Re: [PATCH net-next v3 2/6] net: dsa: motorcomm: Split MIB buffers

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

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

- [Medium] Asymmetric use of the counted workqueue disable API: the new
  .port_teardown callback yt921x_dsa_port_teardown()…
- [Low] The descriptor-table comment above yt921x_mib_descs[] says 'Must
  agree with yt921x_mib', but after this patch `struct yt921x_mib`…

Pre-existing issues:
- [Medium] The software MIB shadow is not re-zeroed when the hardware MIB
  counters are cleared: yt921x_chip_setup() issues YT921X_MIB_CTRL_CLEAN…
- [Low] Unsynchronized 64-bit statistics: the MIB counters are written by
  yt921x_mib_read() from the poll worker under priv->reg_lock (plain…

--

net: dsa: motorcomm: Split MIB buffers

This moves the per-port MIB statistics, the derived rx/tx frame counters
and the MIB poll delayed_work out of struct yt921x_port into a new
struct yt921x_mib, allocated with devm_kzalloc() from .port_setup() only
for internal/external ports.  A new .port_teardown() callback replaces
the disable loop that used to live in yt921x_mdio_remove().

[Severity: Low]
Two comments look stale after the split.

The header comment above yt921x_mib_descs[] says "Must agree with
yt921x_mib", but struct yt921x_mib is now the wrapper holding the port
back pointer, the delayed work and the two derived counters.  The
descriptor offsets are applied to the flat counter layout instead:

	mib = &pm->stats;
	...
		u64 *valp = &((u64 *)mib)[i];

Should that comment name yt921x_mib_stats now?

The comment kept here still says "Reading of yt921x_port::mib is not
protected by a lock", although that member is only a pointer after this
patch and the lockless data lives in yt921x_mib::stats, rx_frames and
tx_frames.  This second half is removed later in the series by "net: dsa:
motorcomm: Fix MIB synchronization", but the yt921x_mib_descs comment
still reads "Must agree with yt921x_mib" at the end of the series.

[ ... ]

[Severity: Low]
This isn't a bug introduced by this patch, but can these u64 reads tear on
32-bit?  yt921x_mib_read() stores pm->rx_frames and pm->tx_frames with
plain assignments from the poll worker, and yt921x_dsa_get_stats64() reads
them with neither priv->reg_lock nor a u64_stats_sync retry loop, so a
reader can observe halves from two different updates.  The subtraction

	stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets;

can then underflow.

The baseline had the same lockless read of pp->rx_frames/pp->tx_frames, and
later patches in this series ("net: dsa: motorcomm: Use u64_stats_t for MIB
stats" and "net: dsa: motorcomm: Fix MIB synchronization") wrap the writer
in u64_stats_update_begin()/end() and the reader in
u64_stats_fetch_begin()/u64_stats_fetch_retry(), so this is only noted for
completeness.

[ ... ]

[Severity: Medium]
Is the disable/enable pair here asymmetric?  yt921x_dsa_port_teardown()
calls disable_delayed_work_sync(&pm->work), but yt921x_dsa_port_setup() has
no matching enable_delayed_work(), and the "if (!pm && ...)" gate means an
already allocated pm (and its work item, including the disable counter) is
reused untouched.

The disable count is a plain counter in the work data, documented in
kernel/workqueue.c as:

 * Disable @work by incrementing its disable count and cancel it if currently
 * pending. As long as the disable count is non-zero, any attempt to queue @work
 * will fail and return %false.

with work_offqd_disable()/work_offqd_enable() incrementing/decrementing it
and enable_delayed_work() being the only decrement.  So after a single
teardown/setup cycle on a still-live switch:

yt921x_phylink_mac_link_up()
	schedule_delayed_work(&pm->work, 0);	/* returns false, forever */

yt921x_mib_poll() never runs again and the cached counters read by
yt921x_dsa_get_stats64() and the ethtool callbacks stay frozen, including
the 32-bit wrap accumulation done in yt921x_mib_read().

Two paths where DSA runs port_setup again for the same priv:

net/dsa/dsa.c:dsa_tree_setup_ports() {
	...
			err = dsa_port_setup(dp);
			if (err) {
				err = dsa_port_setup_as_unused(dp);
	...
}

dsa_port_setup() already ran the driver's port_teardown on its error path
(dsa_port_devlink_setup() calls dsa_port_devlink_teardown() when
devlink_port_register() fails), and then port_setup is called again.

In a multi-switch tree, dsa_switch_remove() -> dsa_tree_teardown() tears
down the ports of the surviving switches too, and a later re-probe runs
dsa_tree_setup() again, calling port_setup() for a port whose pp->mib is
already non-NULL.

Should port_setup() call enable_delayed_work(&pm->work) when it reuses an
existing pm?  As written, each cycle also bumps the disable depth
monotonically until it hits WORK_OFFQ_DISABLE_BITS and WARN_ONCE fires.

[Severity: Medium]
This is a pre-existing issue, but should the shadow counters be re-zeroed
here rather than only on first allocation?  yt921x_chip_setup() clears the
hardware MIB on every .setup():

	ctrl = YT921X_MIB_CTRL_CLEAN | YT921X_MIB_CTRL_ALL_PORT;
	res = yt921x_reg_write(priv, YT921X_MIB_CTRL, ctrl);

while pm->stats is zeroed only by the devm_kzalloc() inside the
"if (!pm && ...)" branch.  dsa_switch_teardown() clears ds->setup, so a
second dsa_tree_setup() can call ds->ops->setup again for the same priv (for
example when a sibling switch in the tree is removed and re-probed).  The
retained shadow then holds pre-clean values while the hardware restarts at
zero, and the first read applies the wrap correction:

drivers/net/dsa/motorcomm/chip.c:yt921x_mib_read() {
	...
			val = (old_val & ~(u64)U32_MAX) | val0;
			if (val < old_val)
				val += 1ull << 32;
	...
}

adding a spurious 2^32 to every affected counter.  The baseline had the same
behaviour with the shadow embedded in priv.

[ ... ]

[ ... ]

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