linux-kernel - RE: [PATCH net-next] net: phy: microchip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DM4PR11MB623922B7FE567372AB617CA88B9E2@DM4PR11MB6239.namprd11.prod.outlook.com>
Date: Fri, 6 Sep 2024 05:45:48 +0000
From: <Tarun.Alle@...rochip.com>
To: <andrew@...n.ch>
CC: <Arun.Ramadoss@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
	<hkallweit1@...il.com>, <linux@...linux.org.uk>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next] net: phy: microchip_t1: SQI support for LAN887x

Hi Andrew,
Thanks for the review comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Thursday, September 5, 2024 6:09 PM
> To: Tarun Alle - I68638 <Tarun.Alle@...rochip.com>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@...rochip.com>; UNGLinuxDriver
> <UNGLinuxDriver@...rochip.com>; hkallweit1@...il.com;
> linux@...linux.org.uk; davem@...emloft.net; edumazet@...gle.com;
> kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH net-next] net: phy: microchip_t1: SQI support for LAN887x
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +     /* Get 200 SQI raw readings */
> > +     for (int i = 0; i < 200; i++) {
> 
> Please replace all the hard coded 200 with ARRAY_SIZE(rawtable). That makes it
> easier to tune the size of the table without causing buffer overrun bugs.
> 

Will fix in next version.

> > +             rc = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                                LAN887X_POKE_PEEK_100,
> > +                                LAN887X_POKE_PEEK_100_EN);
> > +             if (rc < 0)
> > +                     return rc;
> > +
> > +             rc = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> > +                               LAN887X_SQI_MSE_100);
> > +             if (rc < 0)
> > +                     return rc;
> > +
> > +             rawtable[i] = rc;
> > +             rc = genphy_c45_read_link(phydev);
> > +             if (rc < 0)
> > +                     return rc;
> > +
> > +             if (!phydev->link)
> > +                     return -ENETDOWN;
> > +     }
> 
> How long does this take?
> 

~76ms

> genphy_c45_read_link() takes a few MDIO transaction, plus the two you see
> here. So maybe 1000 MDIO bus transactions? Which could be
> 3000-4000 if it needs to use C45 over C22.
> 
> Do you have any data on the accuracy, with say 10, 20, 40, 80, 160 samples?
>

Here number of samples are suggested by our compliance test data.
There is an APP Note regarding SQI samples and calculations.
No, the number of samples are only 200 as any other count was not
consistent in terms of accuracy.
 
> Can the genphy_c45_read_link() be moved out of the loop? If the link is lost, is the
> sample totally random, or does it have a well defined value? Looking at the link
> status every iteration, rather than before and after collecting the samples, you are
> trying to protect against the link going down and back up again. If it is taking a
> couple of seconds to collect all the samples, i suppose that is possible, but if its
> 50ms, do you really have to worry?
> 


Sampling data is random. If the link is down at any point during
the data sampling we are discarding the entire set.
If we check the link status before and after the data collection, there could
be an invalidate SQI derivation in very worst-case scenario.

Just to improve instead of register read can I change it to use phydev->link variable?
This link variable is update by PHY state machine.

> > +static int lan887x_get_sqi(struct phy_device *phydev) {
> > +     int rc, val;
> > +
> > +     if (phydev->speed != SPEED_1000 &&
> > +         phydev->speed != SPEED_100) {
> > +             return -EINVAL;
> > +     }
> 
> Can that happen? Does the PHY support SPEED_10? Or are you trying to protect
> against SPEED_UNKOWN because the link is down? ENETDOWN might be more
> appropriate that EINVAL.
>

LAN887x supports SPEED_100 and SPEED_1000 only. This condition is to 
address the unknow speed. Will fix the error code in next version.
 
>         Andrew

Thanks,
Tarun Alle.