Re: [RFC PATCH v2 net-next 0/4] DSA master state tracking

[Ansuel Smith <ansuelsmth@gmail.com>]

On Fri, Dec 10, 2021 at 07:54:42PM +0000, Vladimir Oltean wrote:
> On Fri, Dec 10, 2021 at 08:45:43PM +0100, Ansuel Smith wrote:
> > > Anyway the reason why I didn't say anything about this is because I
> > > don't yet understand how it is supposed to work. Specifically:
> > > 
> > > rtnl_lock
> > > 
> > > dev_open()
> > > -> __dev_open()
> > >    -> dev->flags |= IFF_UP;
> > >    -> dev_activate()
> > >       -> transition_one_qdisc()
> > > -> call_netdevice_notifiers(NETDEV_UP, dev);
> > > 
> > > rtnl_unlock
> > > 
> > > so the qdisc should have already transitioned by the time NETDEV_UP is
> > > emitted.
> > > 
> > > and since we already require a NETDEV_UP to have occurred, or dev->flags
> > > to contain IFF_UP, I simply don't understand the following
> > > (a) why would the qdisc be noop when we catch NETDEV_UP
> > > (b) who calls netdev_state_change() (or __dev_notify_flags ?!) after the
> > >     qdisc changes on a TX queue? If no one, then I'm not sure how we can
> > >     reliably check for the state of the qdisc if we aren't notified
> > >     about changes to it.
> > 
> > The ipv6 check is just a hint. The real clue was the second
> > NETDEV_CHANGE called by linkwatch_do_dev in link_watch.c
> > That is the one that calls the CHANGE event before the ready stuff.
> > 
> > I had problem tracking this as the change logic is "emit CHANGE when flags
> > change" but netdev_state_change is also called for other reason and one
> > example is dev_activate/dev_deactivate from linkwatch_do_dev.
> > It seems a bit confusing that a generic state change is called even when
> > flags are not changed and because of this is a bit problematic track why
> > the CHANGE event was called.
> > 
> > Wonder if linkwatch_do_dev should be changed and introduce a flag? But
> > that seems problematic if for whatever reason a driver use the CHANGE
> > event to track exactly dev_activate/deactivate.
> 
> Yes, I had my own "aha" moment just minutes before you sent this email
> about linkwatch_do_dev. So indeed that's the source of both the
> dev_activate(), as well as the netdev_state_change() notifier.
> 
> As to my previous question (why would the qdisc be noop when we catch
> NETDEV_UP): the answer is of course in the code as well:
> 
> dev_activate() has:
> 	if (!netif_carrier_ok(dev))
> 		/* Delay activation until next carrier-on event */
> 		return;
> 
> which is then actually picked up from linkwatch_do_dev().
> 
> Let's not change linkwatch_do_dev(), I just wanted to understand why it
> works. Please confirm that it also works for you to make master_admin_up
> depend on qdisc_tx_is_noop() instead of the current ingress_queue check,
> then add a comment stating the mechanism through which we are tracking
> the dev_activate() calls, and then this should be good to go.
> I'd like you to pick up the patches and post them together with your
> driver changes. I can't post the patches on my own since I don't have
> any use for them. I'll leave a few more "review" comments on them in a
> minute.

Ok will do the test, but I'm positive about that.
So the idea is to send a v3 rfc with the depends of the tagger-owned
private data. Add to my series your series with this extra check.
(when I will post v3 feel free to tell me if I messed code credits)

Is the additional bool and function correct or should we merge them and
assume a link up only when we both have the flag and the qdisc?

-- 
	Ansuel