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

[Birger Koblitz]

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