RE: [PATCH net-next V3] net: phy: microchip_t1: SQI support for LAN887x

[Tarun.Alle]

Hi Andrew,
Thank you for the review comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Wednesday, September 11, 2024 10:18 PM
> To: Tarun Alle - I68638 <Tarun.Alle@xxxxxxxxxxxxx>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; hkallweit1@xxxxxxxxx;
> linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next V3] net: phy: microchip_t1: SQI support for
> LAN887x
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +     /* Keep inliers and discard outliers */
> > +     for (int i = ARRAY_SIZE(rawtable) / 5;
> > +          i < ARRAY_SIZE(rawtable) / 5 * 4; i++)
> > +             sqiavg += rawtable[i];
> > +
> > +     /* Get SQI average */
> > +     sqiavg /= 120;
> 
> 120?
> 
> Isn't that ARRAY_SIZE(rawtable) / 5 * 4 - ARRAY_SIZE(rawtable) / 5
> 
> Please think about the comments being given. I said you should not assume 200,
> but use ARRAY_SIZE, so it is possible to change the size of the array and not get
> buffer overruns etc. So you need to review all the code.
> 

My apologies I realized the issue just after sending the patch. 

> Better still, change it to 50 and make sure you get sensible values from it. The
> accuracy won't be as good, but i would expect it to be still about right. But with
> the current code, i guess you get 7 no matter what the actual quality is.
> 

To be consistent and accurate with compliance tests, 200 samples are used.
If customer wants accurate SQI value, we need to use suggested sample count 
as per the compliance reports. Otherwise, single (any other value) sample can also
be used which will give ~ +/-1 SQI value (which may not be guaranteed always).


> This is a general principle in C code, and coding in general. Don't scatter the same
> knowledge repeatedly everywhere, because it makes it error prone to change.
> You have to find and change them all, rather than just one central value.
> 

Going forward I will take care of constants and repetitive calculations.

>     Andrew
> 
> ---
> pw-bot: cr

Thanks,
Tarun Alle.