Re: [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs

[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: 0 · Low: 1

- [Low] Stale comment after the rename:
  `drivers/net/dsa/motorcomm/chip.c:41` still reads "/* Must agree with…

--

net: dsa: motorcomm: Rename MIB stuffs

This renames yt921x_read_mib() to yt921x_mib_read(), yt921x_poll_mib() to
yt921x_mib_poll(), and struct yt921x_mib to struct yt921x_mib_stats, in
preparation for a larger MIB refactor.  No functional change is intended.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index d663af010f431..ad5fe1ccb1d71 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -545,11 +545,11 @@ yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp)
>  }
>  
>  /* Read and handle overflow of 32bit MIBs. MIB buffer must be zeroed before. */
> -static int yt921x_read_mib(struct yt921x_priv *priv, int port)
> +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 *mib = &pp->mib;
> +	struct yt921x_mib_stats *mib = &pp->mib;
>  	int res = 0;

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
> index 83cd454955ddb..805febb486838 100644
> --- a/drivers/net/dsa/motorcomm/chip.h
> +++ b/drivers/net/dsa/motorcomm/chip.h
> @@ -861,7 +861,7 @@ enum yt921x_fdb_entry_status {
>  #define yt921x_port_is_internal(port) ((port) < 8)
>  #define yt921x_port_is_external(port) ((port) == 8 || (port) == 9)
>  
> -struct yt921x_mib {
> +struct yt921x_mib_stats {
>  	u64 rx_broadcast;
>  	u64 rx_pause;
>  	u64 rx_multicast;

[Severity: Low]
Should the comment above yt921x_mib_descs[] in chip.c be renamed too?  It
still points at the old type name:

drivers/net/dsa/motorcomm/chip.c
	/* Must agree with yt921x_mib
	 *
	 * Unstructured fields (name != NULL) will appear in get_ethtool_stats(),
	 * ...
	 */
	static const struct yt921x_mib_desc yt921x_mib_descs[] = {

After this commit there is no struct yt921x_mib left in the tree, so the
comment names a type that no longer exists.

This comment is also the only thing documenting the invariant that
yt921x_mib_read() relies on:

	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];

The descriptor table order and count must match the u64 field order and
count of the (now renamed) struct, and there is no static_assert() or
BUILD_BUG_ON() tying ARRAY_SIZE(yt921x_mib_descs) to
sizeof(struct yt921x_mib_stats) / sizeof(u64).  The invariant itself still
holds here (39 descriptors, 39 u64 fields, same order), so this is only a
comment-versus-definition mismatch.

Worth noting for the rest of the series: by the end of the series the same
comment survives in drivers/net/dsa/motorcomm/mib.c, but struct yt921x_mib
has been reintroduced there as a different container type (work, syncp,
stats, data[]).  At that point the comment points at a struct that the
table must not agree with, since the table has to match
struct yt921x_mib_stats.  Would it make sense to update the comment in this
patch, and possibly add a compile-time assertion on the entry count?

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