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

[Yangfl]

On Mon, Oct 27, 2025 at 8:14 PM Andrew Lunn <andrew@xxxxxxx> 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?