Message-ID: <5bbf1201-e803-40ea-a081-c25919f5e89d@huawei.com>
Date: Thu, 29 May 2025 17:36:11 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>, <davem@...emloft.net>,
	Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet
	<edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Russell King
	<linux@...linux.org.uk>, Heiner Kallweit <hkallweit1@...il.com>
CC: <shaojijie@...wei.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <thomas.petazzoni@...tlin.com>,
	<linux-arm-kernel@...ts.infradead.org>, Christophe Leroy
	<christophe.leroy@...roup.eu>, Herve Codina <herve.codina@...tlin.com>,
	Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean
	<vladimir.oltean@....com>, Köry Maincent
	<kory.maincent@...tlin.com>, Oleksij Rempel <o.rempel@...gutronix.de>, Simon
 Horman <horms@...nel.org>, Romain Gantois <romain.gantois@...tlin.com>,
	"shenjian15@...wei.com" <shenjian15@...wei.com>
Subject: Re: [PATCH net-next v5 07/13] net: phy: phy_caps: Allow looking-up
 link caps based on speed and duplex


on 2025/3/8 1:36, Maxime Chevallier wrote:
> As the link_caps array is efficient for <speed,duplex> lookups,
> implement a function for speed/duplex lookups that matches a given
> mask. This replicates to some extent the phy_lookup_settings()
> behaviour, matching full link_capabilities instead of a single linkmode.
>
> phy.c's phy_santize_settings() and phylink's
> phylink_ethtool_ksettings_set() performs such lookup using the
> phy_settings table, but are only interested in the actual speed/duplex
> that were matched, rathet than the individual linkmode.
>
> Similar to phy_lookup_settings(), the newly introduced phy_caps_lookup()
> will run through the link_caps[] array by descending speed/duplex order.
>
> If the link_capabilities for a given <speed/duplex> tuple intersects the
> passed linkmodes, we consider that a match.
>
> Similar to phy_lookup_settings(), we also allow passing an 'exact'
> boolean, allowing non-exact match. Here, we MUST always match the
> linkmodes mask, but we allow matching on lower speed settings.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> ---
>   drivers/net/phy/phy-caps.h |  4 ++++
>   drivers/net/phy/phy.c      | 32 ++++++--------------------
>   drivers/net/phy/phy_caps.c | 47 ++++++++++++++++++++++++++++++++++++++
>   drivers/net/phy/phylink.c  | 17 +++++++-------
>   4 files changed, 67 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
> index 8833798f141f..aef4b54a8429 100644
> --- a/drivers/net/phy/phy-caps.h
> +++ b/drivers/net/phy/phy-caps.h
> @@ -51,4 +51,8 @@ phy_caps_lookup_by_linkmode(const unsigned long *linkmodes);
>   const struct link_capabilities *
>   phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only);
>   
> +const struct link_capabilities *
> +phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
> +		bool exact);
> +
>   #endif /* __PHY_CAPS_H */
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 8df37d221fba..562acde89224 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -213,25 +213,6 @@ int phy_aneg_done(struct phy_device *phydev)
>   }
>   EXPORT_SYMBOL(phy_aneg_done);
>   
> -/**
> - * phy_find_valid - find a PHY setting that matches the requested parameters
> - * @speed: desired speed
> - * @duplex: desired duplex
> - * @supported: mask of supported link modes
> - *
> - * Locate a supported phy setting that is, in priority order:
> - * - an exact match for the specified speed and duplex mode
> - * - a match for the specified speed, or slower speed
> - * - the slowest supported speed
> - * Returns the matched phy_setting entry, or %NULL if no supported phy
> - * settings were found.
> - */
> -static const struct phy_setting *
> -phy_find_valid(int speed, int duplex, unsigned long *supported)
> -{
> -	return phy_lookup_setting(speed, duplex, supported, false);
> -}
> -
>   /**
>    * phy_supported_speeds - return all speeds currently supported by a phy device
>    * @phy: The phy device to return supported speeds of.
> @@ -274,13 +255,14 @@ EXPORT_SYMBOL(phy_check_valid);
>    */
>   static void phy_sanitize_settings(struct phy_device *phydev)
>   {
> -	const struct phy_setting *setting;
> +	const struct link_capabilities *c;
> +
> +	c = phy_caps_lookup(phydev->speed, phydev->duplex, phydev->supported,
> +			    false);
>   
> -	setting = phy_find_valid(phydev->speed, phydev->duplex,
> -				 phydev->supported);
> -	if (setting) {
> -		phydev->speed = setting->speed;
> -		phydev->duplex = setting->duplex;
> +	if (c) {
> +		phydev->speed = c->speed;
> +		phydev->duplex = c->duplex;
>   	} else {
>   		/* We failed to find anything (no supported speeds?) */
>   		phydev->speed = SPEED_UNKNOWN;
> diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
> index c39f38c12ef2..0366feee2912 100644
> --- a/drivers/net/phy/phy_caps.c
> +++ b/drivers/net/phy/phy_caps.c
> @@ -170,6 +170,53 @@ phy_caps_lookup_by_linkmode_rev(const unsigned long *linkmodes, bool fdx_only)
>   	return NULL;
>   }
>   
> +/**
> + * phy_caps_lookup() - Lookup capabilities by speed/duplex that matches a mask
> + * @speed: Speed to match
> + * @duplex: Duplex to match
> + * @supported: Mask of linkmodes to match
> + * @exact: Perform an exact match or not.
> + *
> + * Lookup a link_capabilities entry that intersect the supported linkmodes mask,
> + * and that matches the passed speed and duplex.
> + *
> + * When @exact is set, an exact match is performed on speed and duplex, meaning
> + * that if the linkmodes for the given speed and duplex intersect the supported
> + * mask, this capability is returned, otherwise we don't have a match and return
> + * NULL.
> + *
> + * When @exact is not set, we return either an exact match, or matching capabilities
> + * at lower speed, or the lowest matching speed, or NULL.
> + *
> + * Returns: a matched link_capabilities according to the above process, NULL
> + *	    otherwise.
> + */
> +const struct link_capabilities *
> +phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
> +		bool exact)
> +{
> +	const struct link_capabilities *lcap, *last = NULL;
> +
> +	for_each_link_caps_desc_speed(lcap) {
> +		if (linkmode_intersects(lcap->linkmodes, supported)) {
> +			last = lcap;
> +			/* exact match on speed and duplex*/
> +			if (lcap->speed == speed && lcap->duplex == duplex) {
> +				return lcap;
> +			} else if (!exact) {
> +				if (lcap->speed <= speed)
> +					return lcap;
> +			}
> +		}
> +	}
> +
> +	if (!exact)
> +		return last;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(phy_caps_lookup);
> +
>   /**
>    * phy_caps_linkmode_max_speed() - Clamp a linkmodes set to a max speed
>    * @max_speed: Speed limit for the linkmode set
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a3f64b6d2d34..cf9f019382ad 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -20,6 +20,7 @@
>   #include <linux/timer.h>
>   #include <linux/workqueue.h>
>   
> +#include "phy-caps.h"
>   #include "sfp.h"
>   #include "swphy.h"
>   
> @@ -2852,8 +2853,8 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   				  const struct ethtool_link_ksettings *kset)
>   {
>   	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
> +	const struct link_capabilities *c;
>   	struct phylink_link_state config;
> -	const struct phy_setting *s;
>   
>   	ASSERT_RTNL();
>   
> @@ -2896,23 +2897,23 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   		/* Autonegotiation disabled, select a suitable speed and
>   		 * duplex.
>   		 */
> -		s = phy_lookup_setting(kset->base.speed, kset->base.duplex,
> -				       pl->supported, false);
> -		if (!s)
> +		c = phy_caps_lookup(kset->base.speed, kset->base.duplex,
> +				    pl->supported, false);
> +		if (!c)
>   			return -EINVAL;



Hi Maxime,  fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex") might have different behavior than the modification.
My case is set 10M Half with disable autoneg both sides and I expect it is
link in 10M Half. But now, it is link in 10M Full，which is not what I
expect.

I used followed command and trace how phy worked.
	ethtool -s eth1 autoneg off speed 10 duplex half
The log is showed as followed:
ethtool-13127	[067]	6164.771853: phy_ethtool_ksettings set: (phy_ethtool ksettings set+0x0/0x200) duplex=0 speed=10
kworker/u322:2-11096	[070]	6164.771853:	_phy_start_aneq: ( _phy_start_aneg+0x0/0xb8) duplex=0 speed=10
kworker/u322:2-11096	[070]	6164.771854:	phy_caps_lookup: (phy_caps_lookup+0x0/0xf0) duplex=0 speed=10
kworker/u322:2-11096	[070]	6164.771855:	phy_config_aneg: (phy_config_aneg+0x0/0x70) duplex=1 speed=10
kworker/u322:2-11096	[070]	6164.771856:	genphy_config_aneg:	(__genphy_config_aneg+0X0/0X270) duplex=1 speed=10

I also try to fixed it and it works. Do you have any idea about it.

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0e762fc3a529..2986c41c42a8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -258,7 +258,7 @@ static void phy_sanitize_settings(struct phy_device *phydev)
         const struct link_capabilities *c;

         c = phy_caps_lookup(phydev->speed, phydev->duplex, phydev->supported,
-                           false);
+                           true);

         if (c) {
                 phydev->speed = c->speed;



>   
>   		/* If we have a fixed link, refuse to change link parameters.
>   		 * If the link parameters match, accept them but do nothing.
>   		 */
>   		if (pl->req_link_an_mode == MLO_AN_FIXED) {
> -			if (s->speed != pl->link_config.speed ||
> -			    s->duplex != pl->link_config.duplex)
> +			if (c->speed != pl->link_config.speed ||
> +			    c->duplex != pl->link_config.duplex)
>   				return -EINVAL;
>   			return 0;
>   		}
>   
> -		config.speed = s->speed;
> -		config.duplex = s->duplex;
> +		config.speed = c->speed;
> +		config.duplex = c->duplex;
>   		break;
>   
>   	case AUTONEG_ENABLE:

Powered by blists - more mailing lists