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

On Fri, Dec 10, 2021 at 08:45:43PM +0100, Ansuel Smith wrote:

quoted

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.