Re: [PATCH net-next v3 3/6] net: dsa: motorcomm: Split MIB module

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] MIB polling work is permanently disabled by
  `yt921x_dsa_port_teardown()`…

Pre-existing issues:
- [Medium] Unsynchronized access to the cached 64-bit MIB counters: at
  this commit `yt921x_mib_read()` (drivers/net/dsa/motorcomm/mib.c)…
- [Medium] Non-atomic read of the 64-bit MIB counter register pairs: for
  descriptors with `size == 2` (RX_GOOD_BYTES 0x3c/0x40, RX_BAD_BYTES…

--

net: dsa: motorcomm: Split MIB module

This moves the yt921x MIB descriptor table, register definitions, stats
structures and all of the ethtool/stats64 callbacks out of chip.c and
chip.h into new mib.c and mib.h files, in preparation for yt922x using a
different MIB scheme. The change is described as pure code motion.

> diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
> new file mode 100644
> index 0000000000000..b1296f224cf69
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/mib.c
> @@ -0,0 +1,408 @@

[ ... ]

> +/* Read and handle overflow of 32bit MIBs. MIB buffer must be zeroed before. */
> +static int yt921x_mib_read(struct yt921x_priv *priv, int port)
> +{

[ ... ]

> +	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> +		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> +		u32 reg = YT921X_MIBn_DATA0(port) + desc->offset;
> +		u64 *valp = &((u64 *)mib)[i];
> +		u32 val0;
> +		u64 val;
> +
> +		res = yt921x_reg_read(priv, reg, &val0);
> +		if (res)
> +			break;
> +
> +		if (desc->size <= 1) {
> +			u64 old_val = *valp;
> +
> +			val = (old_val & ~(u64)U32_MAX) | val0;
> +			if (val < old_val)
> +				val += 1ull << 32;
> +		} else {
> +			u32 val1;
> +
> +			res = yt921x_reg_read(priv, reg + 4, &val1);
> +			if (res)
> +				break;
> +			val = ((u64)val1 << 32) | val0;
> +		}

[Severity: Medium]
This is a pre-existing issue carried over by the move, not something this
patch introduces, but can the size == 2 path assemble a torn value?

The two halves come from two independent SMI/MDIO transactions:

	res = yt921x_reg_read(priv, reg, &val0);
	...
	res = yt921x_reg_read(priv, reg + 4, &val1);
	...
	val = ((u64)val1 << 32) | val0;

If the low word wraps between the two reads, the combined value is off by
2^32.  Unlike the size <= 1 branch there is no wraparound correction here,
so the affected counters (RX_GOOD_BYTES, RX_BAD_BYTES, TX_GOOD_BYTES) can
step backwards by about 4 GiB, and stats64 rx_bytes/tx_bytes can underflow
once ETH_FCS_LEN * packets is subtracted.

At 1 Gbps the low word of a byte counter wraps roughly every 34 seconds,
while each register access over MDIO takes tens of microseconds, so the
window is reachable in practice.

For traceability: a later commit in this series, "net: dsa: motorcomm: Use
safe 64-bit counter reader", replaces this open-coded pair read with
yt921x_counter_read() in smi.c, which reads lo, then re-reads hi/lo and
accepts the value only when lo >= old_lo (bounded to 16 attempts,
-ETIMEDOUT otherwise).

> +
> +		WRITE_ONCE(*valp, val);
> +	}
> +
> +	pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte +
> +			mib->rx_128_255byte + mib->rx_256_511byte +
> +			mib->rx_512_1023byte + mib->rx_1024_1518byte +
> +			mib->rx_jumbo;
> +	pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte +
> +			mib->tx_128_255byte + mib->tx_256_511byte +
> +			mib->tx_512_1023byte + mib->tx_1024_1518byte +
> +			mib->tx_jumbo;

[ ... ]

> +void yt921x_mib_poll(struct work_struct *work)
> +{
> +	struct yt921x_mib *pm = container_of_const(work, struct yt921x_mib,
> +						   work.work);

[ ... ]

> +	schedule_delayed_work(&pm->work, delay);
> +}

[Severity: Medium]
Now that yt921x_mib_poll() lives here, is the work item's disable count
ever balanced for a re-used struct yt921x_mib?

In chip.c, yt921x_dsa_port_teardown() does:

	if (pm)
		disable_delayed_work_sync(&pm->work);

which is __cancel_work_sync(&dwork->work, WORK_CANCEL_DELAYED |
WORK_CANCEL_DISABLE), so the disable count stays in work->data until a
matching enable_delayed_work().

On a later setup, yt921x_dsa_port_setup() only re-initialises the work when
the allocation is fresh:

	if (!pm && (BIT(port) & (priv->info->internal_mask |
		    priv->info->external_mask))) {
		pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL);
		...
		INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll);
	}

pp->mib is a devm allocation that survives a DSA tree teardown/setup cycle,
so pm is non-NULL the second time and neither INIT_DELAYED_WORK() nor
enable_delayed_work() runs.

The teardown callback is reached on ordinary port teardown, not just device
removal: dsa_port_devlink_teardown() calls ds->ops->port_teardown, and
dsa_switch_remove() tears down the whole tree, so a surviving switch's
ports get torn down while its pp->mib stays alive.  A later
dsa_tree_setup() then calls port_setup again on that switch.

After that, the schedule_delayed_work(&pm->work, 0) in
yt921x_phylink_mac_link_up() is silently dropped by
clear_pending_if_disabled(), so MIB polling never resumes for the port and
the 32-bit wraparound accumulation in yt921x_mib_read() stops happening.

Would cancel_delayed_work_sync() in port_teardown, or a matching
enable_delayed_work() in yt921x_dsa_port_setup(), be more appropriate for a
re-usable object?

[ ... ]

> +void
> +yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
> +		       struct rtnl_link_stats64 *stats)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct yt921x_port *pp = &priv->ports[port];
> +	struct yt921x_mib *pm = pp->mib;
> +	struct yt921x_mib_stats *mib;
> +
> +	if (!pm)
> +		return;
> +	mib = &pm->stats;

[ ... ]

> +	stats->rx_packets = pm->rx_frames;
> +	stats->tx_packets = pm->tx_frames;
> +	stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets;
> +	stats->tx_bytes = mib->tx_good_bytes - ETH_FCS_LEN * stats->tx_packets;

[Severity: Medium]
This isn't a bug introduced by this patch, but how are these cached 64-bit
counters synchronized against the poll worker?

yt921x_mib_read() writes each counter with WRITE_ONCE() on a plain u64 and
stores pm->rx_frames / pm->tx_frames with plain assignments while holding
priv->reg_lock.  yt921x_dsa_get_stats64() reads all of them with no lock at
all, and the other callbacks release the lock before copying out, e.g.:

	mutex_lock(&priv->reg_lock);
	yt921x_mib_read(priv, port);
	mutex_unlock(&priv->reg_lock);

	mac_stats->FramesTransmittedOK = pm->tx_frames;

and in yt921x_dsa_get_pause_stats():

	mutex_unlock(&priv->reg_lock);

	pause_stats->tx_pause_frames = mib->tx_pause;

On 32-bit builds a u64 store is two stores, so a reader can observe
mismatched halves, and derived values can mix fields from different
snapshots.  Can rx_bytes underflow when rx_good_bytes comes from an older
snapshot than rx_frames?

For traceability: later commits in this series, "net: dsa: motorcomm: Use
u64_stats_t for MIB stats" and "net: dsa: motorcomm: Fix MIB
synchronization", convert the counters to u64_stats_t, wrap the write-back
in u64_stats_update_begin()/u64_stats_update_end(), add a
u64_stats_fetch_begin()/u64_stats_fetch_retry() loop in
yt921x_dsa_get_stats64(), and move the remaining field copies inside the
reg_lock section.

> diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h
> new file mode 100644
> index 0000000000000..cfad0665baaea
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/mib.h
> @@ -0,0 +1,159 @@

[ ... ]

> +struct yt921x_mib_stats {
> +	u64 rx_broadcast;
> +	u64 rx_pause;

[ ... ]

> +struct yt921x_mib {
> +	struct yt921x_port *port;
> +
> +	struct delayed_work work;
> +	struct yt921x_mib_stats stats;
> +	u64 rx_frames;
> +	u64 tx_frames;
> +};

[ ... ]

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