Re: [PATCH net-next v2 1/2] net: dsa: yt921x: Fix MIB overflow wraparound routine

[Andrew Lunn <andrew@lunn.ch>]

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