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

On Thu, Dec 09, 2021 at 05:56:17PM +0000, Vladimir Oltean wrote:

On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:

quoted

quoted

I think the problem is that we also need to track the operstate of the
master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
You can see that this is exactly the line after which the timeouts disappear:

[    7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

I didn't really want to go there, because now I'm not sure how to
synthesize the information for the switch drivers to consume it.
Anyway I've prepared a v2 patchset and I'll send it out very soon.

Wonder if we should leave the driver decide when it's ready by parsing
the different state? (And change
the up ops to something like a generic change?)

There isn't just one state to track, which is precisely the problem that
I had to deal with for v2. The master is operational during the time
frame between NETDEV_UP and NETDEV_GOING_DOWN, intersected with the
interval during which netif_oper_up(master) is true. So in the simple
state propagation approach, DSA would need to provide at least two ops
to switches, one for admin state and the other for oper state. And the
switch driver would need to AND the two and keep state by itself.
Letting the driver make the decision would have been acceptable to me if
we could have 3 ops and a common implementation, something like this:

static void qca8k_master_state_change(struct dsa_switch *ds,
				      const struct dsa_master *master)
{
	bool operational = (master->flags & IFF_UP) && netif_oper_up(master);
}

	.master_admin_state_change	= qca8k_master_state_change,
	.master_oper_state_change	= qca8k_master_state_change,

but the problem is that during NETDEV_GOING_DOWN, master->flags & IFF_UP
is still true, so this wouldn't work. And replacing the NETDEV_GOING_DOWN
notifier with the NETDEV_DOWN one would solve that problem, but it would
no longer guarantee that the switch can disable this feature without
timeouts before the master is down - because now it _is_ down.

Ok will have to test v2 and check if this is also fixed.

-- 
	Ansuel