linux-kernel - Re: [PATCH] PHY: Fix no autoneg corner case

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 for Android: free password hash cracker in your pocket

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

Message-ID: <m3serah8ch.fsf@t19.piap.pl>
Date: Fri, 29 Nov 2024 07:17:34 +0100
From: Krzysztof Hałasa <khalasa@...p.pl>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>,  Oliver Neukum <oneukum@...e.com>,
  Andrew Lunn <andrew+netdev@...n.ch>,  "David S. Miller"
 <davem@...emloft.net>,  Eric Dumazet <edumazet@...gle.com>,  Jakub
 Kicinski <kuba@...nel.org>,  Paolo Abeni <pabeni@...hat.com>,
  linux-usb@...r.kernel.org,  linux-kernel@...r.kernel.org,  Jose Ignacio
 Tornos Martinez <jtornosm@...hat.com>,  Ming Lei <ming.lei@...hat.com>
Subject: Re: [PATCH] PHY: Fix no autoneg corner case

Andrew,

thanks for your response.

Andrew Lunn <andrew@...n.ch> writes:

>> It seems the phy/phylink code assumes the PHY starts with autoneg
>> enabled (if supported). This is simply an incorrect assumption.
>
> This is sounding like a driver bug. When phy_start() is called it
> kicks off the PHY state machine. That should result in
> phy_config_aneg() being called. That function is badly named, since it
> is used both for autoneg and forced setting. The purpose of that call
> is to configure the PHY to the configuration stored in
> phydev->advertise, etc. So if the PHY by hardware defaults has autoneg
> disabled, but the configuration in phydev says it should be enabled,
> calling phy_config_aneg() should actually enabled autoneg.

But... how would the driver know if autoneg is to be enabled or not?

In the USB ASIX case, the Ethernet driver could dig this info up from
the chip EEPROM. Not sure if I like this way, though. Complicated, and
it's not needed in this case I think.

> I would say there are two different issues here.
>
> 1) It seems like we are not configuring the hardware to match phydev.
> 2) We are overwriting how the bootloader etc configured the hardware.
>
> 2) is always hard, because how do we know the PHY is not messed up
> from a previous boot/crash cycle etc. In general, a driver should try
> to put the hardware into a well known state. If we have a clear use
> case for this, we can consider how to implement it.

Well, I think if someone set the PHY previously, and then the machine
rebooted (without actually changing PHY config), then perhaps the
settings are better than any defaults anyway. Though I guess it will be
configured in the init scripts again soon.

It's not something easily messed up by a crash. But yes, there is a risk
the config was wrong, set by mistake or something.

BTW USB adapters will almost always reconfig PHY on boot, because they
are powered from USB bus.

In this case, with ASIX USB adapter (internal PHY ax88796b /
ax88796b_rust), the MAC + PHY will be configured by hardware on USB
power up. So we _know_ the settings are better than any hardcoded
defaults.

Maybe the specific ASIX PHY code should handle this.

Nevertheless, the inconsistency between phy/phylink/etc. and the actual
hardware PHY is there.
I guess I will have a look at this again shortly.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa