Reported by the following Smatch static checker warning:
drivers/net/dsa/yt921x.c:702 yt921x_read_mib()
warn: was expecting a 64 bit value instead of '(~0)'
Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/netdev/aPsjYKQMzpY0nSXm@stanley.mountain/
Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
drivers/net/dsa/yt921x.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
index 0b3df732c0d1..5e4e8093ba16 100644
--- a/drivers/net/dsa/yt921x.c
+++ b/drivers/net/dsa/yt921x.c
@@ -682,21 +682,22 @@ static int yt921x_read_mib(struct yt921x_priv *priv, int port)
const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
u32 reg = YT921X_MIBn_DATA0(port) + desc->offset;
u64 *valp = &((u64 *)mib)[i];
- u64 val = *valp;
u32 val0;
- u32 val1;
+ u64 val;
res = yt921x_reg_read(priv, reg, &val0);
if (res)
break;
if (desc->size <= 1) {
- if (val < (u32)val)
- /* overflow */
- val += (u64)U32_MAX + 1;
- val &= ~U32_MAX;
- val |= val0;
+ 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;
--
2.51.0
This is an AI-generated review of your patch.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: yt921x: Use u64_stats_t for MIB stats
This patch converts MIB statistics from plain u64 to u64_stats_t and adds
u64_stats_sync protection for atomic statistics access on 32-bit systems.
> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index 7b8c1549a0fb..3d87944830f6 100644
[ ... ]
> @@ -762,22 +779,27 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
> struct yt921x_priv *priv = to_yt921x_priv(ds);
> struct yt921x_port *pp = &priv->ports[port];
> struct yt921x_mib *mib = &pp->mib;
> + unsigned int start;
> size_t j;
>
> mutex_lock(&priv->reg_lock);
> yt921x_read_mib(priv, port);
> mutex_unlock(&priv->reg_lock);
>
> - j = 0;
> - for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> - const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> + do {
> + start = u64_stats_fetch_begin(&pp->syncp);
This uses pp->syncp via u64_stats_fetch_begin(), but the syncp field does
not appear to be initialized with u64_stats_init() anywhere.
Looking at yt921x_mdio_probe(), the port initialization loop initializes
pp->index and pp->mib_read but not pp->syncp:
for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
struct yt921x_port *pp = &priv->ports[i];
pp->index = i;
INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
}
On 32-bit systems, u64_stats_init() calls seqcount_init() which sets up
the lockdep class. Without this initialization, CONFIG_DEBUG_LOCK_ALLOC
builds may produce lockdep warnings.
For comparison, the xrs700x driver at drivers/net/dsa/xrs700x/xrs700x.c
initializes its syncp field with u64_stats_init(&p->syncp).
Should u64_stats_init(&pp->syncp) be added to the port initialization loop
in yt921x_mdio_probe()?
[ ... ]
> diff --git a/drivers/net/dsa/yt921x.h b/drivers/net/dsa/yt921x.h
> index 61bb0ab3b09a..17fee83952c2 100644
> --- a/drivers/net/dsa/yt921x.h
> +++ b/drivers/net/dsa/yt921x.h
[ ... ]
> @@ -533,9 +535,13 @@ struct yt921x_port {
> bool isolated;
>
> struct delayed_work mib_read;
> + struct u64_stats_sync syncp;
> struct yt921x_mib mib;
> - u64 rx_frames;
> - u64 tx_frames;
> + u64_stats_t rx_frames;
> + u64_stats_t tx_frames;
> +
> + /* only used by read routine to avoid huge allocations on the stack */
> + struct yt921x_mib mib_new;
> };
--
pw-bot: cr
© 2016 - 2026 Red Hat, Inc.