Message-ID: <20250530095608.63c42399@fedora.home>
Date: Fri, 30 May 2025 09:56:08 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Jijie Shao <shaojijie@...wei.com>
Cc: <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>, <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 Thu, 29 May 2025 17:36:11 +0800
Jijie Shao <shaojijie@...wei.com> wrote:

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

The !exact match logic in the rework is wrong indeed, sorry...

For non-exact matches we return a non-exact match too early without
giving the chance for a potentially exact half-duplex match...

As for the fix, can you try this out :

diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index 703321689726..d80f6a37edf1 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -195,7 +195,7 @@ 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;
+	const struct link_capabilities *lcap, *match = NULL, *last = NULL;
 
 	for_each_link_caps_desc_speed(lcap) {
 		if (linkmode_intersects(lcap->linkmodes, supported)) {
@@ -204,16 +204,19 @@ phy_caps_lookup(int speed, unsigned int duplex, const unsigned long *supported,
 			if (lcap->speed == speed && lcap->duplex == duplex) {
 				return lcap;
 			} else if (!exact) {
-				if (lcap->speed <= speed)
-					return lcap;
+				if (!match && lcap->speed <= speed)
+					match = lcap;
+
+				if (lcap->speed < speed)
+					break;
 			}
 		}
 	}
 
-	if (!exact)
-		return last;
+	if (!match && !exact)
+		match = last;
 
-	return NULL;
+	return match;
 }
 EXPORT_SYMBOL_GPL(phy_caps_lookup);

Powered by blists - more mailing lists