Re: [net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

On Wed, Dec 08, 2021 at 04:53:41PM +0200, Vladimir Oltean wrote:
> 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.

Anyway just as an info, I implemented also the mib handler and it does
work correctly. Vladimir implementation works just good and it seems
pretty clean with the tagger used only to implement stuff and the driver
to do the ""dirty"" work.

But anyway for this kind of transaction, we really need a way to track
when it's possible to use it. We need to polish that. Just to not mix
things I will had my comments in the other series about tracking master
up.

-- 
	Ansuel