Re: [PATCH net-next] net: dsa: motorcomm: Do not do the redundant MIB clear

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message's last paragraph gives a reason why the
  removed MIB clear needs no replacement anywhere else, and both parts…

--

net: dsa: motorcomm: Do not do the redundant MIB clear

This patch removes the MIB clear from yt921x_chip_setup(). The commit
message argues it is redundant because a hardware reset happens right
before it. It also argues that nothing else needs to handle MIB state,
because per-port state is always freshly allocated.

> Nor should we take care of it elsewhere: .port_setup() should not clear
> it, and .setup() runs at most once per registration, so per-port state
> is always freshly allocated by .port_setup().

[Severity: Medium]
Is this paragraph accurate? Both of its claims seem to conflict with the
DSA core and with the driver's own port setup code.

On ".setup() runs at most once per registration": in net/dsa/dsa.c,
dsa_switch_remove() calls dsa_tree_teardown() on the whole tree. That
reaches dsa_switch_teardown() for every member switch, which does:

    ds->setup = false;

When the removed switch probes again, the call chain is:

dsa_switch_probe()->dsa_tree_setup()->dsa_switch_setup()
    err = ds->ops->setup(ds);

This runs for every switch in the tree, including a yt921x that never
unregistered.

On "per-port state is always freshly allocated by .port_setup()":
yt921x_dsa_port_setup() only allocates the MIB block when none exists:

    if (!pm && (BIT(port) & yt921x_info_ports_mask(info))) {
        pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL);
        ...
        /* Start disabled, so that the enable below always pairs with
         * either this disable or the one from a previous teardown
         */

yt921x_dsa_port_teardown() deliberately keeps the block:

    /* get_stats64() might not be synchronized with port_teardown(), hold
     * the MIB block till the device teardown
     */

So on a second .setup() within one registration, yt921x_dsa_setup() calls
yt921x_chip_reset(). Per this commit message, that zeroes the hardware
counters. pm->data[] and pm->stats still hold the old accumulated values.

yt921x_mib_read() says "MIB buffer must be zeroed before" and extends the
32-bit counters like this:

    val = (old_val & ~(u64)U32_MAX) | val0;
    if (val < old_val)
        val += 1ull << 32;

After a re-setup, won't the first poll treat the reset counters as a wrap?
That would add about 2^32 to each 32-bit counter. The 64-bit counters would
go backwards to small values. These values then show up in get_stats64,
ethtool -S and the rmon/mac/pause stats.

The removed write to YT921X_MIB_CTRL never cleared the software buffer, so
this patch does not change runtime behaviour here. The concern is that the
commit message says no handling is needed, based on invariants the code does
not appear to guarantee.

Could the commit message be corrected? Alternatively, should
yt921x_dsa_port_setup() clear pm->data and pm->stats when it reuses an
existing block, since every .setup() hard-resets the chip?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010231015.2600207-1-mmyangfl%40gmail.com