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)'
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 ab762ffc4661..97a7eeb4ea15 100644
--- a/drivers/net/dsa/yt921x.c
+++ b/drivers/net/dsa/yt921x.c
@@ -687,21 +687,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;
+ u64 val;
u32 val0;
- u32 val1;
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.0On Sun, Oct 26, 2025 at 01:13:10AM +0800, David Yang wrote:
> 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)'
>
> 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 ab762ffc4661..97a7eeb4ea15 100644
> --- a/drivers/net/dsa/yt921x.c
> +++ b/drivers/net/dsa/yt921x.c
> @@ -687,21 +687,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;
> + u64 val;
> u32 val0;
> - u32 val1;
>
> 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;
> +
What David suggested, https://lore.kernel.org/all/20251024132117.43f39504@pumpkin/ was
if (desc->size <= 1) {
u64 old_val = *valp;
val = upper32_bits(old_val) | val0;
if (val < old_val)
val += 1ull << 32;
}
I believe there is a minor typo here, it should be upper_32_bits(),
but what you implemented is not really what David suggested.
Andrew
On Mon, Oct 27, 2025 at 10:30 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Oct 26, 2025 at 01:13:10AM +0800, David Yang wrote:
> > 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)'
> >
> > 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 ab762ffc4661..97a7eeb4ea15 100644
> > --- a/drivers/net/dsa/yt921x.c
> > +++ b/drivers/net/dsa/yt921x.c
> > @@ -687,21 +687,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;
> > + u64 val;
> > u32 val0;
> > - u32 val1;
> >
> > 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;
> > +
>
> What David suggested, https://lore.kernel.org/all/20251024132117.43f39504@pumpkin/ was
>
> if (desc->size <= 1) {
> u64 old_val = *valp;
> val = upper32_bits(old_val) | val0;
> if (val < old_val)
> val += 1ull << 32;
> }
>
> I believe there is a minor typo here, it should be upper_32_bits(),
> but what you implemented is not really what David suggested.
>
> Andrew
I didn't find the definition for upper32_bits, so...
> > > diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> > > index ab762ffc4661..97a7eeb4ea15 100644
> > > --- a/drivers/net/dsa/yt921x.c
> > > +++ b/drivers/net/dsa/yt921x.c
> > > @@ -687,21 +687,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;
> > > + u64 val;
> > > u32 val0;
> > > - u32 val1;
> > >
> > > 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;
> > > +
> >
> > What David suggested, https://lore.kernel.org/all/20251024132117.43f39504@pumpkin/ was
> >
> > if (desc->size <= 1) {
> > u64 old_val = *valp;
> > val = upper32_bits(old_val) | val0;
> > if (val < old_val)
> > val += 1ull << 32;
> > }
> >
> > I believe there is a minor typo here, it should be upper_32_bits(),
> > but what you implemented is not really what David suggested.
> >
> > Andrew
>
> I didn't find the definition for upper32_bits, so...
You should of asked, or searched a bit harder, because what you
changed it to is different.
/**
* upper_32_bits - return bits 32-63 of a number
* @n: the number we're accessing
*
* A basic shift-right of a 64- or 32-bit quantity. Use this to suppress
* the "right shift count >= width of type" warning when that quantity is
* 32-bits.
*/
#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
I don't see any shifting in your version.
And then i have to ask, which is correct?
How have you been testing this code? If this is TX bytes, for a 1G
link, it will overflow 32 bits in about 34 seconds. So a simple iperf
test could be used. If its TX packets, 64 byte packets could be done
in 5 hours.
Andrew
On Mon, Oct 27, 2025 at 8:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > > diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> > > > index ab762ffc4661..97a7eeb4ea15 100644
> > > > --- a/drivers/net/dsa/yt921x.c
> > > > +++ b/drivers/net/dsa/yt921x.c
> > > > @@ -687,21 +687,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;
> > > > + u64 val;
> > > > u32 val0;
> > > > - u32 val1;
> > > >
> > > > 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;
> > > > +
> > >
> > > What David suggested, https://lore.kernel.org/all/20251024132117.43f39504@pumpkin/ was
> > >
> > > if (desc->size <= 1) {
> > > u64 old_val = *valp;
> > > val = upper32_bits(old_val) | val0;
> > > if (val < old_val)
> > > val += 1ull << 32;
> > > }
> > >
> > > I believe there is a minor typo here, it should be upper_32_bits(),
> > > but what you implemented is not really what David suggested.
> > >
> > > Andrew
> >
> > I didn't find the definition for upper32_bits, so...
>
> You should of asked, or searched a bit harder, because what you
> changed it to is different.
>
> /**
> * upper_32_bits - return bits 32-63 of a number
> * @n: the number we're accessing
> *
> * A basic shift-right of a 64- or 32-bit quantity. Use this to suppress
> * the "right shift count >= width of type" warning when that quantity is
> * 32-bits.
> */
> #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
>
> I don't see any shifting in your version.
>
> And then i have to ask, which is correct?
>
> How have you been testing this code? If this is TX bytes, for a 1G
> link, it will overflow 32 bits in about 34 seconds. So a simple iperf
> test could be used. If its TX packets, 64 byte packets could be done
> in 5 hours.
>
> Andrew
I used ping to check whether the statistics match expected values and
didn't realize iperf, I'll check that later.
> but what you implemented is not really what David suggested.
Shifting is clearly wrong here and I think they got upper_32_bits()
wrong too, but should or shouldn't I give credit to them
(Suggested-by), if I took most of, but not exactly all of their ideas?
© 2016 - 2026 Red Hat, Inc.