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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <f870d2c7-cf0a-4e78-80d6-faa490a13820@gmail.com>
Date: Fri, 29 Nov 2024 07:49:32 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Krzysztof Hałasa <khalasa@...p.pl>,
 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

On 29.11.2024 07:17, Krzysztof Hałasa wrote:
> 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?
> 
If autoneg is supported, then phylib defaults to enable it. I don't see
anything wrong with this. BaseT modes from 1000Mbps on require autoneg.
Your original commit message seems to refer to a use case where a certain
operation mode of the PHY doesn't support autoneg. Then the PHY driver
should detect this operation mode and clear the autoneg-supported bit.

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