[v1] net: phy: phy_caps: Don't skip better duplex macth on non-exact match

[PATCH net] net: phy: phy_caps: Don't skip better duplex macth on non-exact match

Posted by Maxime Chevallier 8 months, 1 week ago

When performing a non-exact phy_caps lookup, we are looking for a
supported mode that matches as closely as possible the passed speed/duplex.

Blamed patch broke that logic by returning a match too early in case
the caller asks for half-duplex, as a full-duplex linkmode may match
first, and returned as a non-exact match without even trying to mach on
half-duplex modes.

Reported-by: Jijie Shao <shaojijie@huawei.com>
Closes: https://lore.kernel.org/netdev/20250603102500.4ec743cf@fedora/T/#m22ed60ca635c67dc7d9cbb47e8995b2beb5c1576
Fixes: fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_caps.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

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);
 
-- 
2.49.0

Re: [PATCH net] net: phy: phy_caps: Don't skip better duplex macth on non-exact match

Posted by Paolo Abeni 8 months, 1 week ago

On 6/3/25 10:35 AM, Maxime Chevallier wrote:
> When performing a non-exact phy_caps lookup, we are looking for a
> supported mode that matches as closely as possible the passed speed/duplex.
> 
> Blamed patch broke that logic by returning a match too early in case
> the caller asks for half-duplex, as a full-duplex linkmode may match
> first, and returned as a non-exact match without even trying to mach on
> half-duplex modes.
> 
> Reported-by: Jijie Shao <shaojijie@huawei.com>
> Closes: https://lore.kernel.org/netdev/20250603102500.4ec743cf@fedora/T/#m22ed60ca635c67dc7d9cbb47e8995b2beb5c1576
> Fixes: fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/phy_caps.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> 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;

If I read correctly, when user asks for half-duplex, this can still
return a non exact matching full duplex cap, even when there is non
exact matching half-duplex cap available.

I'm wondering if the latter would be preferable, or at least if the
current behaviour should be explicitly called out in the function
documentation.

/P

Re: [PATCH net] net: phy: phy_caps: Don't skip better duplex macth on non-exact match

Posted by Maxime Chevallier 8 months ago

On Thu, 5 Jun 2025 12:24:54 +0200
Paolo Abeni <pabeni@redhat.com> wrote:

> On 6/3/25 10:35 AM, Maxime Chevallier wrote:
> > When performing a non-exact phy_caps lookup, we are looking for a
> > supported mode that matches as closely as possible the passed speed/duplex.
> > 
> > Blamed patch broke that logic by returning a match too early in case
> > the caller asks for half-duplex, as a full-duplex linkmode may match
> > first, and returned as a non-exact match without even trying to mach on
> > half-duplex modes.
> > 
> > Reported-by: Jijie Shao <shaojijie@huawei.com>
> > Closes: https://lore.kernel.org/netdev/20250603102500.4ec743cf@fedora/T/#m22ed60ca635c67dc7d9cbb47e8995b2beb5c1576
> > Fixes: fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex")
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/phy/phy_caps.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > 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;  
> 
> If I read correctly, when user asks for half-duplex, this can still
> return a non exact matching full duplex cap, even when there is non
> exact matching half-duplex cap available.

That would be only if there's no half-duplex match at the requested
speed, but yes indeed.

> 
> I'm wondering if the latter would be preferable, or at least if the
> current behaviour should be explicitly called out in the function
> documentation.

Looking at the previous way of doing this, we looked at the following
array in descending order : 

[...]
	/* 1G */
	PHY_SETTING(   1000, FULL,   1000baseT_Full		),
	PHY_SETTING(   1000, HALF,   1000baseT_Half		),
	PHY_SETTING(   1000, FULL,   1000baseT1_Full		),
	PHY_SETTING(   1000, FULL,   1000baseX_Full		),
	PHY_SETTING(   1000, FULL,   1000baseKX_Full		),
	/* 100M */
	PHY_SETTING(    100, FULL,    100baseT_Full		),
	PHY_SETTING(    100, FULL,    100baseT1_Full		),
	PHY_SETTING(    100, HALF,    100baseT_Half		),
	PHY_SETTING(    100, HALF,    100baseFX_Half		),
	PHY_SETTING(    100, FULL,    100baseFX_Full		),
[...]

The matching logic was pretty much the same, walk that (and and'ing
with the passed supported modes), note the partial matches, return
either an exact or non-exact match.

None of the logic actually cared about the duplex for non-exact
matches, only the speed. I think we would have faced the same behaviour.

In reality, the case you're mentioning would be a device that supports
1000/Full, 100/Full and 100/Half, user asks for 1000/Half, and 100/Full
would be reported.

That's unlikely to exist, but I'll document it as I've been surprised
in the past with setups that shouldn't exist that actually do :)

All of this really makes me want to add some test scripts to cover all
these corner-case behaviours and test for future regressions.
Hopefully when I get a bit more bandwidth I'll be finally able to
finish the netdevsim PHY support...

Thanks,

Maxime

> /P
>

Re: [PATCH net] net: phy: phy_caps: Don't skip better duplex macth on non-exact match

Posted by Russell King (Oracle) 8 months ago

On Fri, Jun 06, 2025 at 10:19:23AM +0200, Maxime Chevallier wrote:
> In reality, the case you're mentioning would be a device that supports
> 1000/Full, 100/Full and 100/Half, user asks for 1000/Half, and 100/Full
> would be reported.
> 
> That's unlikely to exist, but I'll document it as I've been surprised
> in the past with setups that shouldn't exist that actually do :)

That's not unlikely. Many MACs do not support 1000/Half but do support
100/Half and 10/Half.

Also... you're wrong about the result.

If phy_lookup_setting() is called with speed = 1000, duplex = half,
and we have a MAC that supports 1000/Full, 100/Half, 100/Full, then:

- We iterate down through the phy_settings[] to:
	PHY_SETTING(   1000, FULL,   1000baseT_Full             ),

	if (p->speed == speed && p->duplex == duplex) {
  the first condition is true, the second is false.
	} else if (!exact) {
  this is true.
		if (!match && p->speed <= speed)
  match is NULL, and p->speed == speed, so this is a candidate:
			match = p;
  We continue.
- The next entry that will be tested is:
	PHY_SETTING(    100, FULL,    100baseT_Full             ),

	if (p->speed == speed && p->duplex == duplex) {
  the first condition is false.
	} else if (!exact) {
  this is true.
		if (!match && p->speed <= speed)
  this is now false because match is set.
  We continue.
- The next entry that will be tested is:
	PHY_SETTING(    100, HALF,    100baseT_Half             ),

	if (p->speed == speed && p->duplex == duplex) {
  the first condition is false.
	} else if (!exact) {
  this is true.
		if (!match && p->speed <= speed)
  this is now false because match is set.
  We continue.

- We eventually get to the end of the list.
	if (!match && !exact)
  this is false.

- We return the entry for 1000/Full, which is exactly the behaviour I
  was after when I wrote this matching.

If you're version doesn't come out with a matching speed, then I'm
afraid it's still broken.

-- 
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: phy: phy_caps: Don't skip better duplex macth on non-exact match

Posted by Maxime Chevallier 8 months ago

Hi,

On Fri, 6 Jun 2025 09:30:59 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Fri, Jun 06, 2025 at 10:19:23AM +0200, Maxime Chevallier wrote:
> > In reality, the case you're mentioning would be a device that supports
> > 1000/Full, 100/Full and 100/Half, user asks for 1000/Half, and 100/Full
> > would be reported.

> If you're version doesn't come out with a matching speed, then I'm
> afraid it's still broken.

My analysis is wrong, but the new result is right. The patch we're
discussing is correct I think, we do report a matching speed (tested on
an espressobin with manual masking of the linkmodes)

I'll update the doc though.

Maxime

Re: [PATCH net] net: phy: phy_caps: Don't skip better duplex macth on non-exact match

Posted by Larysa Zaremba 8 months, 1 week ago

On Tue, Jun 03, 2025 at 10:35:40AM +0200, Maxime Chevallier wrote:
> When performing a non-exact phy_caps lookup, we are looking for a
> supported mode that matches as closely as possible the passed speed/duplex.
> 
> Blamed patch broke that logic by returning a match too early in case
> the caller asks for half-duplex, as a full-duplex linkmode may match
> first, and returned as a non-exact match without even trying to mach on
> half-duplex modes.
> 
> Reported-by: Jijie Shao <shaojijie@huawei.com>
> Closes: https://lore.kernel.org/netdev/20250603102500.4ec743cf@fedora/T/#m22ed60ca635c67dc7d9cbb47e8995b2beb5c1576
> Fixes: fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>

> ---
>  drivers/net/phy/phy_caps.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> 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);
>  
> -- 
> 2.49.0
> 
>

Re: [PATCH net] net: phy: phy_caps: Don't skip better duplex macth on non-exact match

Posted by Jijie Shao 8 months, 1 week ago

on 2025/6/3 16:35, Maxime Chevallier wrote:
> When performing a non-exact phy_caps lookup, we are looking for a
> supported mode that matches as closely as possible the passed speed/duplex.
>
> Blamed patch broke that logic by returning a match too early in case
> the caller asks for half-duplex, as a full-duplex linkmode may match
> first, and returned as a non-exact match without even trying to mach on
> half-duplex modes.
>
> Reported-by: Jijie Shao <shaojijie@huawei.com>
> Closes: https://lore.kernel.org/netdev/20250603102500.4ec743cf@fedora/T/#m22ed60ca635c67dc7d9cbb47e8995b2beb5c1576
> Fixes: fc81e257d19f ("net: phy: phy_caps: Allow looking-up link caps based on speed and duplex")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Tested-by: Jijie Shao <shaojijie@huawei.com>

> ---
>   drivers/net/phy/phy_caps.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
>
> 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);
>