Re: [PATCH net-next] net: lan966x: Fix lan966x_ifh_get

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 04/14/2023 19:00, Alexander Lobakin wrote:
> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Fri, 14 Apr 2023 10:20:47 +0200

Hi Olek,

> 
> >>From time to time, it was observed that the nanosecond part of the
> > received timestamp, which is extracted from the IFH, it was actually
> > bigger than 1 second. So then when actually calculating the full
> > received timestamp, based on the nanosecond part from IFH and the second
> > part which is read from HW, it was actually wrong.
> >
> > The issue seems to be inside the function lan966x_ifh_get, which
> > extracts information from an IFH(which is an byte array) and returns the
> > value in a u64. When extracting the timestamp value from the IFH, which
> > starts at bit 192 and have the size of 32 bits, then if the most
> > significant bit was set in the timestamp, then this bit was extended
> > then the return value became 0xffffffff... . To fix this, make sure to
> > clear all the other bits before returning the value.
> 
> Ooooh, I remember I was having the same issue with sign extension :s
> Pls see below.
> 
> >
> > Fixes: fd7627833ddf ("net: lan966x: Stop using packing library")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index 80e2ea7e6ce8a..508e494dcc342 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -608,6 +608,7 @@ static u64 lan966x_ifh_get(u8 *ifh, size_t pos, size_t length)
> >                       val |= (1 << i);
> 
> Alternatively, you can change that to (pick one that you like the most):
> 
>                         val |= 1ULL << i;
>                         // or
>                         val |= BIT_ULL(i);
> 
> The thing is that constants without any postfix (U, UL etc.) are treated
> as signed longs, that's why `1 << 31` becomes 0xffffffff80000000. 1U /
> 1UL / 1ULL don't.
> 
> Adding unsigned postfix may also make it better for 32-bit systems, as
> `1 << i` there is 32-bit value, so `1 << 48` may go wrong and/or even
> trigger compilers.

Thanks for suggestion and the explanation, it was really helpful.
I will update this in the next version.

> 
> >       }
> >
> > +     val &= GENMASK(length, 0);
> >       return val;
> >  }
> >
> 
> (now blah not directly related to the fix)

I think this change regarding the improvement of the lan966x_ifh_get
should not be in the next version of this patch, as there are 2
different things. But I would still like to know how to do this!
> 
> I'm wondering a bit if lan966x_ifh_get() can be improved in general to
> work with words rather than bits. You read one byte per each bit each
> iteration there.

Actually, I am not reading 1 byte per each bit iteration. I am reading 1
byte first time when entering in the loop or each time when the bit
iteration (j variable) is modulo 8.

> For example, byte arrays could be casted to __be{32,64} and you'd get
> native byteorder for 32/64 bits via one __be*_to_cpu*() call.

> 
> Thanks,
> Olek

-- 
/Horatiu