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

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

Posted by Wei Fang 2 months, 3 weeks ago

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);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
-- 
2.34.1

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

Posted by Andrew Lunn 2 months, 3 weeks ago

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]

with autoneg off, rx and tx are directly applied to the MAC. But to do
that, are these bits needed in supported?

Maybe you should explain what problem you are seeing?

	Andrew

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

Posted by Russell King (Oracle) 2 months, 3 weeks ago

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!

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

Posted by Andrew Lunn 2 months, 3 weeks ago

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

Not too surprising, given the number of discussions...

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

Agreed.

I often like Vladimirs patches. Two pages of commit message for a one
line change.

    Andrew

---
pw-bot: cr

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

Posted by Maxime Chevallier 2 months, 3 weeks ago


On 14/11/2025 06:28, 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>

I agree with the fact that these used to be set, and it's no longer the
case since the blamed commit. My bad

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
>  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);
>  	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
>  	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
>  	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);

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

Posted by Russell King (Oracle) 2 months, 3 weeks ago

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.

What problem does this cause?

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