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

[netdev-bot+sashiko@kernel.org]

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().

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index ad5fe1ccb1d71..95041a2fb0ecc 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -549,9 +549,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
>  {
>  	struct yt921x_port *pp = &priv->ports[port];
>  	struct device *dev = to_device(priv);
> -	struct yt921x_mib_stats *mib = &pp->mib;
> +	struct yt921x_mib *pm = pp->mib;
> +	struct yt921x_mib_stats *mib;
>  	int res = 0;
>  
> +	mib = &pm->stats;
> +
>  	/* Reading of yt921x_port::mib is not protected by a lock and it's vain
>  	 * to keep its consistency, since we have to read registers one by one
>  	 * and there is no way to make a snapshot of MIB stats.

[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.

[ ... ]

> @@ -786,7 +810,12 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  {
>  	struct yt921x_priv *priv = to_yt921x_priv(ds);
>  	struct yt921x_port *pp = &priv->ports[port];
> -	struct yt921x_mib_stats *mib = &pp->mib;
> +	struct yt921x_mib *pm = pp->mib;
> +	struct yt921x_mib_stats *mib;
> +
> +	if (!pm)
> +		return;
> +	mib = &pm->stats;
>  
>  	stats->rx_length_errors = mib->rx_undersize_errors +
>  				  mib->rx_fragment_errors;
> @@ -802,8 +831,8 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  	/* stats->tx_heartbeat_errors */
>  	stats->tx_window_errors = mib->tx_late_collisions;
>  
> -	stats->rx_packets = pp->rx_frames;
> -	stats->tx_packets = pp->tx_frames;
> +	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: 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.

[ ... ]

> @@ -3988,7 +4026,9 @@ yt921x_phylink_mac_link_up(struct phylink_config *config,
>  		dev_err(dp->ds->dev, "Failed to %s port %d: %i\n", "bring up",
>  			port, res);
>  
> -	schedule_delayed_work(&priv->ports[port].mib_read, 0);
> +	pm = priv->ports[port].mib;
> +	if (pm)
> +		schedule_delayed_work(&pm->work, 0);
>  }
>  
>  static void
> @@ -4108,11 +4148,35 @@ yt921x_dsa_get_tag_protocol(struct dsa_switch *ds, int port,
>  	return DSA_TAG_PROTO_YT921X;
>  }
>  
> +static void yt921x_dsa_port_teardown(struct dsa_switch *ds, int port)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct yt921x_port *pp = &priv->ports[port];
> +	struct yt921x_mib *pm = pp->mib;
> +
> +	if (pm)
> +		disable_delayed_work_sync(&pm->work);
> +}
> +
>  static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port)
>  {
>  	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct yt921x_port *pp = &priv->ports[port];
> +	struct device *dev = to_device(priv);
> +	struct yt921x_mib *pm = pp->mib;
>  	int res;
>  
> +	if (!pm && (BIT(port) & (priv->info->internal_mask |
> +		    priv->info->external_mask))) {
> +		pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL);
> +		if (!pm)
> +			return -ENOMEM;
> +		pp->mib = pm;
> +
> +		pm->port = pp;
> +		INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll);
> +	}
> +

[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.

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
> index 805febb486838..5bf2d3409f4bb 100644
> --- a/drivers/net/dsa/motorcomm/chip.h
> +++ b/drivers/net/dsa/motorcomm/chip.h
> @@ -913,6 +913,15 @@ struct yt921x_mib_stats {
>  	u64 tx_oam;
>  };
>  
> +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