Re: [PATCH] PHY: Fix no autoneg corner case

[Heiner Kallweit <hkallweit1@gmail.com>]

On 29.11.2024 07:17, Krzysztof Hałasa wrote:
> Andrew,
> 
> thanks for your response.
> 
> Andrew Lunn <andrew@lunn.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.