On Sun, 24 Jan 2021 10:20:39 +0200 Vladimir Oltean wrote:
> On Fri, Jan 22, 2021 at 08:52:16PM -0800, Jakub Kicinski wrote:
> > On Thu, 21 Jan 2021 18:01:28 +0200 Vladimir Oltean wrote:
> > > + 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?
>
> The dsa_group sysfs is removed in:
>
> dsa_unregister_switch
> -> mutex_lock(&dsa2_mutex)
> -> dsa_switch_remove
> -> dsa_tree_teardown
> -> dsa_tree_teardown_master
> -> dsa_master_teardown
> -> sysfs_remove_group
> There are 2 points here:
> 1. sysfs_remove_group actually waits for a concurrent tagging_store()
> call to finish (at least it does when I put an msleep(10000) inside
> tagging_store).
> 2. After the sysfs_remove_group, dsa_tree_change_tag_proto should never
> be called again.
>
> Next comes:
> -> dsa_tree_teardown
> -> dsa_tree_teardown_switches
> -> dsa_port_teardown
> -> dsa_slave_destroy
> After this, all DSA net devices are unregistered and freed.
>
> Next comes:
> -> dsa_switch_remove
> -> dsa_switch_release_ports
> -> mutex_unlock(&dsa2_mutex)
> where the dst->ports list is finally freed.
>
> So there is no chance that the dst->ports list is modified concurrently
> with tagging_store.
Sounds good, thanks!
