Re: [PATCH net] net: phylink: add missing supported link modes for the fixed-link

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sat, Nov 15, 2025 at 09:36:44PM +0100, Andrew Lunn wrote:
> On Fri, Nov 14, 2025 at 01:28:08PM +0800, Wei Fang wrote:
> > Pause, Asym_Pause and Autoneg bits are not set when pl->supported is
> > initialized, so these link modes will not work for the fixed-link.
> > 
> > Fixes: de7d3f87be3c ("net: phylink: Use phy_caps_lookup for fixed-link configuration")
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  drivers/net/phy/phylink.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 9d7799ea1c17..918244308215 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -637,6 +637,9 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> >  
> >  static void phylink_fill_fixedlink_supported(unsigned long *supported)
> >  {
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, supported);
> 
> Do these make sense? There is no PHY, so there is no autoneg? So why
> is autoneg in supported?
> 
> You can force pause at the MAC with ethtool:
> 
> ethtool -A|--pause devname [autoneg on|off] [rx on|off] [tx on|off]

No, not for fixed links. (I have a patch which enables this, but we
mutually agreed not to push it into mainline - I think you've forgotten
that discussion.)

> Maybe you should explain what problem you are seeing?

As explained in this thread, it's the lack of pause. Not having the
pause bits in the supported mask means that phylink can't evaluate
the pause modes to use, because even if they are present in DT,
they get cleared because they're cleared in the supported mask.

At least Pause and Asym_Pause need to be set.

This is because the fixed-link pause specification is in terms of
Pause and Asym_Pause. Having one set of these bits is meaningless in
terms of "should we transmit pause frames" and "should we receive
pause frames" which is what hardware wants to know. It would've
been better had the DT binding defined pause in terms of tx/rx not
the Pause/Asym_Pause bits.

However, with that definition, the only way to give these bits any
sane meaning is to treat them as the capabilities of a virtual link
partner, use the Pause and Asym_Pause capabilities of the local MAC,
and evaluate them according to the 802.3 rules.

However, as we end up masking off the local MAC's Pause and Asym_Pause
bits, this results in that evaluation deciding that pause is
unsupported. This is a *regression* that needs fixing, caused by the
blamed commit.

There is no question that this needs fixing.

The question is whether Autoneg should be set or not. As the
advertising and lp_advertising bitmasks have to be non-empty, and the
swphy reports aneg capable, aneg complete, and AN enabled, then for
consistency with that state, Autoneg should be set. This is how it was
prior to the blamed commit.

So, the patch is the correct approach. The only thing that is missing
is the detailed explanation about the problem the regression is
causing. We have too many patches that fix regressions without
explaining what the effect of the regression was... which is bad when
we look back at the history.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!