Re: [PATCH net-next v3 13/13] ax88796b: Add support for AX88772D, AX88179A and AX88279

On 31/07/2026 18:40, Andrew Lunn wrote:
> On Fri, Jul 31, 2026 at 06:14:50PM +0200, Birger Koblitz wrote:
>> On 24/07/2026 19:18, Andrew Lunn wrote:
>>>> +static int asix_ax88279_read_status(struct phy_device *phydev)
>>>> +{
>>>
>>>> +	val = phy_read(phydev, MII_ADVERTISE);
>>>> +	if (val < 0)
>>>> +		return val;
>>>> +
>>>> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>>> +			 phydev->advertising, val & AX_ADVERTISE_2500);
>>>
>>> This looks wrong. A PHY driver should configure the hardware to
>>> advertise what is in phydev->advertising. The PHY driver itself should
>>> never change this value, especially in _read_status(). It is up to
>>> user space to decide on what is advertised, and the phylib core will
>>> sanitise the value to ensure it is within the PHYs capabilities.
>>>
>> This looks wrong, because the PHY uses the wrong bit for configuring
>> advertising 2500_FD. Since phylink does not know about this behaviour
>> the PHY driver needs to decipher this additional bit and then set it
>> in advertising based on the value in the PHY register. But maybe
>> I miss something?
> 
> We have two bitmaps.
> 
> phydev->supported lists all the modes the device supports. Generally,
> that is filled out during probe, when genphy_read_abilities() or
> genphy_c45_pma_read_abilities() is called, which looks at register
> values which indicate what the PHY actually supports. If those
> registers are wrong, because it breaks the standard, you can provide a
> .get_features() callback in the PHY driver. That typically calls
> genphy_read_abilities() and/or genphy_c45_pma_read_abilities(), and
> them fixes up what they have discovered to fit what the device really
> does.
> 
> phylink will also mask phydev->supported with what the MAC actually
> supports. So the PHY might indicate it supported 1G Half, but the MAC
> does not, and so it will be removed.
> 
> phydev->supported is then copied into phydev->advertised. The user can
> modify phydev->advertised, but phydev->supported is fixed.
> 
> When the link is configured up, the .config_aneg() driver method is
> called. It needs to push the bits in phydev->advertised into the
> hardware, so it actually advertises those link modes.
> 
> If the hardware is broken and puts the bits in the wrong place, you
> cannot use the helper. So the drive needs to do the interpretation and
> write the correct bits in the registers. Depending on how it is
> broken, you might be able to call genphy_config_aneg() to do
> 10/100/1G, and then do 2.5G manually in the driver.
> 
> The PHY driver should consider phydev->advertised read only.
Thanks for the explanation, Andrew! I think I finally got it.
I was basically doing a short-circuit between read_status() and
config_aneg() using .advertising in order to get the correct
result with the broken use of the 2.5GBit advertising bit.
I will fix both config_aneg() and read_status() in the next
set of patches and stop writing to .advertising in the PHY driver.

Birger