Re: [PATCH net-next v4 2/3] net: dsa: qca8k: serialize CPU MAC pause during MTU changes

[Yongzhao Chen <yongzhao.derek@gmail.com>]

Hi Christian,

Thanks for the detailed review.

> I'm not entirely sure we need to use the reg mutex here since each port
> have their own register and is independent... a dedicated mutex should be
> considered for the task...

As you suggested, the next revision adds a dedicated per-switch
port_status_lock instead of reusing reg_mutex, which stays with the
FDB/VLAN operations. It is held across the whole MTU sequence and by all
PORT_STATUS writers.

The lock is needed because the MTU sequence can interleave with
phylink's MAC link-up/down callbacks, which run from the phylink resolve
work without RTNL. Per-access register locking cannot protect the whole
read/pause/change/restore sequence.

> I would use __qca8k_port_set.. and add tag to enforce that the mutex should be
> locked here.

Done: the helper is now __qca8k_port_set_status() with
lockdep_assert_held().

> Can port be enabled concurrently and corrupt the port enable map? Can you
> check with AI if this case is possible? If yes then this might be a good
> idea to make a separate prereq patch introducing a dedicated mutex for
> port status and protect it accordingly. (might also be worth for net)

The DSA core calls port_enable() and port_disable() under RTNL, so the
updates to port_enabled_map are already serialized. I did not find a
path where two updates can race, so I don't think a separate net fix is
needed.

> In the context of internal PHY CPU port port 0 and port 6 won't be
> connected... Should we check that and create a mask of the cpu port right
> from the start?

The mask is now (BIT(0) | BIT(6) | dsa_cpu_ports(ds)), intersected with
port_enabled_map. I kept pausing ports 0 and 6 whenever they are
enabled, as the current code does. With an internal CPU port they can
still be in use (in my port 5 CPU test, port 6 was a fixed-link user
port), and I have no evidence that changing the frame size is safe with
their MACs running. Is there hardware guidance confirming that enabled
non-CPU ports 0/6 can remain running during the MTU update?

I have also fixed the reverse xmas tree ordering, and the loops now use
for_each_set_bit() on an unsigned long mask.

Deterministic tests built from the extracted kernel functions cover the
MTU/MAC-callback interleavings and fail when the relevant locking is
removed. On a Redmi AX5400 running an OpenWrt Linux 6.18.52 backport
(wired only, lockdep enabled), MTU changes during repeated renegotiation
passed the functional checks but never hit
a stably down link. In a separate test with the port 5 PHY powered down,
the MTU changes succeeded and the port 5 MAC stayed off in every stable
link-down sample.

Thanks,
Yongzhao Chen