Hi Florian,
On Wed, Jan 20, 2021 at 07:53:41PM -0800, Florian Fainelli wrote:
> > +static int dsa_switch_tag_proto_del(struct dsa_switch *ds,
> > + struct dsa_notifier_tag_proto_info *info)
> > +{
> > + int err = 0, port;
> > +
> > + for (port = 0; port < ds->num_ports; port++) {
> > + if (!dsa_switch_tag_proto_match(ds, port, info))
> > + continue;
> > +
> > + /* Check early if we can replace it, so we don't delete it
> > + * for nothing and leave the switch dangling.
> > + */
> > + if (!ds->ops->set_tag_protocol) {
> > + err = -EOPNOTSUPP;
> > + break;
> > + }
>
> This can be moved out of the loop.
Thanks a lot for reviewing.
Yes, you are right, this can be moved out. It is a left-over from using
dsa_broadcast in the previous version. It is not the only left-over: the
info->tree_index is now redundant because we are notifying within a
single DSA switch tree.
> > +
> > + /* The delete method is optional, just the setter
> > + * is mandatory
> > + */
> > + if (ds->ops->del_tag_protocol)
> > + ds->ops->del_tag_protocol(ds, port,
> > + info->tag_ops->proto);
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static int dsa_switch_tag_proto_set(struct dsa_switch *ds,
> > + struct dsa_notifier_tag_proto_info *info)
> > +{
> > + bool proto_changed = false;
> > + int port, err;
> > +
> > + for (port = 0; port < ds->num_ports; port++) {
> > + struct dsa_port *cpu_dp = dsa_to_port(ds, port);
> > +
> > + if (!dsa_switch_tag_proto_match(ds, port, info))
> > + continue;
> > +
> > + err = ds->ops->set_tag_protocol(ds, cpu_dp->index,
> > + info->tag_ops->proto);
> > + if (err)
> > + return err;
>
> Don't you need to test for ds->ops->set_tag_protocol to be implemented
> before calling it? Similar comment to earlier, can we do an early check
> for the operation being supported outside of the loop?
My assumption was that I had already added the check in tag_proto_del.
There are no code paths that call one but not the other. I can add the
check here too.
There's one more thing I would change: the dsa_switch_tag_proto_match.
Right now I am matching only on the CPU port, but if I take a look at
the mv88e6xxx driver:
static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
{
if (dsa_is_dsa_port(chip->ds, port))
return mv88e6xxx_set_port_mode_dsa(chip, port);
if (dsa_is_user_port(chip->ds, port))
return mv88e6xxx_set_port_mode_normal(chip, port);
/* Setup CPU port mode depending on its supported tag format */
if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA)
return mv88e6xxx_set_port_mode_dsa(chip, port);
if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
return mv88e6xxx_set_port_mode_edsa(chip, port);
return -EINVAL;
}
DSA links call the same function as CPU ports configured for
DSA_TAG_PROTO_DSA. So to cater to Marvell switches too, and to ease a
potential conversion to this API, I could add dsa_is_dsa_port to the
matching function too.
