On Thu, 21 Jan 2021 18:01:28 +0200 Vladimir Oltean wrote: > +/* Since the dsa/tagging sysfs device attribute is per master, the assumption > + * is that all DSA switches within a tree share the same tagger, otherwise > + * they would have formed disjoint trees (different "dsa,member" values). > + */ > +int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst, > + struct net_device *master, > + const struct dsa_device_ops *tag_ops, > + const struct dsa_device_ops *old_tag_ops) > +{ > + struct dsa_notifier_tag_proto_info info; > + struct dsa_port *dp; > + int err; > + > + /* At the moment we don't allow changing the tag protocol under > + * traffic. May revisit in the future. > + */ > + if (master->flags & IFF_UP) > + return -EBUSY;
But you're not holding rtnl_lock at this point, this check is advisory at best. > + list_for_each_entry(dp, &dst->ports, list) { What protects this iteration? All sysfs guarantees you is that struct net_device *master itself will not disappear. Could you explain the locking expectations a bit? > + if (!dsa_is_user_port(dp->ds, dp->index)) > + continue; > + > + if (dp->slave->flags & IFF_UP) > + return -EBUSY; > + } > + > + mutex_lock(&dst->tagger_lock); > + > + info.tag_ops = old_tag_ops; > + err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_DEL, &info); > + if (err) > + return err; > + > + info.tag_ops = tag_ops; > + err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info); > + if (err) > + goto out_unwind_tagger; > + > + mutex_unlock(&dst->tagger_lock); > + > + return 0; > + > +out_unwind_tagger: > + info.tag_ops = old_tag_ops; > + dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info); > + mutex_unlock(&dst->tagger_lock); > + return err; > +}