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: <af78280b-68a5-47f4-986e-667cc704f8da@lunn.ch>
Date: Fri, 6 Sep 2024 14:57:35 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Tarun.Alle@...rochip.com
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

> > How long does this take?
> > 
> 
> ~76ms

That is faster than i expected. You have a pretty efficient MDIO bus
implementation.

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

Which won't get to run because the driver is actively doing SQI. There
is no preemption here, this code will run to completion, and then
phylib will deal with any interrupts for link down, or do its once per
second poll to check the link status.

With this only taking 76ms, what is the likelihood of link down and
link up again within 76ms? For a 1000BaseT PHY, they don't report link
down for 1 second, and it takes another 1 second to perform autoneg
before the link is up again. Now this is an automotive PHY, so the
timing is different. What does the data sheet say about how fast it
detects and reports link down and up?

	Andrew