Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 8 Dec 2021 16:53:41 +0200 | From | Vladimir Oltean <> | Subject | Re: [net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet |
| |
On Wed, Dec 08, 2021 at 03:33:27PM +0100, Ansuel Smith wrote: > > But there are some problems with offering a "master_going_up/master_going_down" > > set of callbacks. Specifically, we could easily hook into the NETDEV_PRE_UP/ > > NETDEV_GOING_DOWN netdev notifiers and transform these into DSA switch > > API calls. The goal would be for the qca8k tagger to mark the > > Ethernet-based register access method as available/unavailable, and in > > the regmap implementation, to use that or the other. DSA would then also > > be responsible for calling "master_going_up" when the switch ports and > > master are sufficiently initialized that traffic should be possible. > > But that first "master_going_up" notification is in fact the most > > problematic one, because we may not receive a NETDEV_PRE_UP event, > > because the DSA master may already be up when we probe our switch tree. > > This would be a bit finicky to get right. We may, for instance, hold > > rtnl_lock for the entirety of dsa_tree_setup_master(). This will block > > potentially concurrent netdevice notifiers handled by dsa_slave_nb. > > And while holding rtnl_lock() and immediately after each dsa_master_setup(), > > we may check whether master->flags & IFF_UP is true, and if it is, > > synthesize a call to ds->ops->master_going_up(). We also need to do the > > reverse in dsa_tree_teardown_master(). > > Should we care about holding the lock for that much time? Will do some > test hoping the IFF_UP is sufficient to make the Ethernet mdio work.
I'm certainly not smart enough to optimize things, so I'd rather hold the rtnl_lock for as long as I'm comfortable is enough to avoid races. The reason why we must hold rtnl_lock is because during dsa_master_setup(), the value of netdev_uses_dsa(dp->master) changes from false to true. The idea is that if IFF_UP isn't set right now, no problem, release the lock and we'll catch the NETDEV_UP notifier when that will appear. But we want to (a) replay the master up state if it was already up while it wasn't a DSA master (b) avoid a potential race where the master does go up, we receive that notification, but netdev_uses_dsa() doesn't yet return true for it.
The model would be similar to what we have for the NETDEV_GOING_DOWN handler.
Please wait for me to finish the sja1105 conversion. There are some issues I've noticed in your connect/disconnect implementation that I haven't had a chance to comment on, yet. I've tested ocelot-8021q plus the tagging protocol change and these appear fine. I'd like to post the changes I have, to make sure that what works for me works for you, and what works for you works for me. I may also have some patches laying around that track the master up/down state (I needed those for some RFC DSA master change patches). I'll build a mini patch series and post it soon-ish.
| ![\](/images/icornerr.gif) |