1. Patchew
  2. linux
  3. [PATCH net-next 00/13] net: phy: Rework linkmodes handling in a dedicated file
  4. View patch

[PATCH net-next 12/13] net: phy: phylink: Use phy_caps_lookup for fixed-link configuration

Maxime Chevallier posted 13 patches 11 months, 3 weeks ago
There is a newer version of this series
[PATCH net-next 12/13] net: phy: phylink: Use phy_caps_lookup for fixed-link configuration
Posted by Maxime Chevallier 11 months, 3 weeks ago 
When phylink creates a fixed-link configuration, it finds a matching
linkmode to set as the advertised, lp_advertising and supported modes
based on the speed and duplex of the fixed link.

Use the newly introduced phy_caps_lookup to get these modes instead of
phy_lookup_settings(). This has the side effect that the matched
settings and configured linkmodes may now contain several linkmodes (the
intersection of supported linkmodes from the phylink settings and the
linkmodes that match speed/duplex) instead of the one from
phy_lookup_settings().

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phylink.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index da7b159702c5..a0b225cd62f5 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -504,9 +504,10 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
 static int phylink_parse_fixedlink(struct phylink *pl,
 				   const struct fwnode_handle *fwnode)
 {
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(match) = { 0, };
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+	const struct link_capabilities *c;
 	struct fwnode_handle *fixed_node;
-	const struct phy_setting *s;
 	struct gpio_desc *desc;
 	u32 speed;
 	int ret;
@@ -578,8 +579,10 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 	linkmode_copy(pl->link_config.advertising, pl->supported);
 	phylink_validate(pl, pl->supported, &pl->link_config);
 
-	s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
-			       pl->supported, true);
+	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
+			    pl->supported, true);
+	if (c)
+		linkmode_and(match, pl->supported, c->linkmodes);
 
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
@@ -588,9 +591,9 @@ static int phylink_parse_fixedlink(struct phylink *pl,
 
 	phylink_set(pl->supported, MII);
 
-	if (s) {
-		__set_bit(s->bit, pl->supported);
-		__set_bit(s->bit, pl->link_config.lp_advertising);
+	if (c) {
+		linkmode_or(pl->supported, pl->supported, match);
+		linkmode_or(pl->link_config.lp_advertising, pl->supported, match);
 	} else {
 		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
 			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
@@ -1578,21 +1581,21 @@ static int phylink_register_sfp(struct phylink *pl,
 int phylink_set_fixed_link(struct phylink *pl,
 			   const struct phylink_link_state *state)
 {
-	const struct phy_setting *s;
+	const struct link_capabilities *c;
 	unsigned long *adv;
 
 	if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
 	    !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
 		return -EINVAL;
 
-	s = phy_lookup_setting(state->speed, state->duplex,
-			       pl->supported, true);
-	if (!s)
+	c = phy_caps_lookup(state->speed, state->duplex,
+			    pl->supported, true);
+	if (!c)
 		return -EINVAL;
 
 	adv = pl->link_config.advertising;
 	linkmode_zero(adv);
-	linkmode_set_bit(s->bit, adv);
+	linkmode_and(adv, pl->supported, c->linkmodes);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, adv);
 
 	pl->link_config.speed = state->speed;
-- 
2.48.1
Re: [PATCH net-next 12/13] net: phy: phylink: Use phy_caps_lookup for fixed-link configuration
Posted by Kory Maincent 11 months, 2 weeks ago 
On Sat, 22 Feb 2025 15:27:24 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> When phylink creates a fixed-link configuration, it finds a matching
> linkmode to set as the advertised, lp_advertising and supported modes
> based on the speed and duplex of the fixed link.
> 
> Use the newly introduced phy_caps_lookup to get these modes instead of
> phy_lookup_settings(). This has the side effect that the matched
> settings and configured linkmodes may now contain several linkmodes (the
> intersection of supported linkmodes from the phylink settings and the
> linkmodes that match speed/duplex) instead of the one from
> phy_lookup_settings().

...

>  
>  	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
>  	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> @@ -588,9 +591,9 @@ static int phylink_parse_fixedlink(struct phylink *pl,
>  
>  	phylink_set(pl->supported, MII);
>  
> -	if (s) {
> -		__set_bit(s->bit, pl->supported);
> -		__set_bit(s->bit, pl->link_config.lp_advertising);
> +	if (c) {
> +		linkmode_or(pl->supported, pl->supported, match);
> +		linkmode_or(pl->link_config.lp_advertising,
> pl->supported, match);

You are doing the OR twice. You should use linkmode_copy() instead.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Re: [PATCH net-next 12/13] net: phy: phylink: Use phy_caps_lookup for fixed-link configuration
Posted by Russell King (Oracle) 11 months, 2 weeks ago 
On Mon, Feb 24, 2025 at 02:44:31PM +0100, Kory Maincent wrote:
> On Sat, 22 Feb 2025 15:27:24 +0100
> Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> 
> > When phylink creates a fixed-link configuration, it finds a matching
> > linkmode to set as the advertised, lp_advertising and supported modes
> > based on the speed and duplex of the fixed link.
> > 
> > Use the newly introduced phy_caps_lookup to get these modes instead of
> > phy_lookup_settings(). This has the side effect that the matched
> > settings and configured linkmodes may now contain several linkmodes (the
> > intersection of supported linkmodes from the phylink settings and the
> > linkmodes that match speed/duplex) instead of the one from
> > phy_lookup_settings().
> 
> ...
> 
> >  
> >  	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
> >  	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> > @@ -588,9 +591,9 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> >  
> >  	phylink_set(pl->supported, MII);
> >  
> > -	if (s) {
> > -		__set_bit(s->bit, pl->supported);
> > -		__set_bit(s->bit, pl->link_config.lp_advertising);
> > +	if (c) {
> > +		linkmode_or(pl->supported, pl->supported, match);
> > +		linkmode_or(pl->link_config.lp_advertising,
> > pl->supported, match);
> 
> You are doing the OR twice. You should use linkmode_copy() instead.

No, we don't want to copy pl->supported to
pl->link_config.lp_advertising. We just want to set the linkmode bit
that corresponds to the speed/duplex in each mask.

That will result in e.g. the pause mode bits will be overwritten despite
being appropriately set in the advertising mask in the code above this.

-- 
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-next 12/13] net: phy: phylink: Use phy_caps_lookup for fixed-link configuration
Posted by Kory Maincent 11 months, 2 weeks ago 
On Mon, 24 Feb 2025 13:53:28 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Feb 24, 2025 at 02:44:31PM +0100, Kory Maincent wrote:
> > On Sat, 22 Feb 2025 15:27:24 +0100
> > Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> >   
> > > When phylink creates a fixed-link configuration, it finds a matching
> > > linkmode to set as the advertised, lp_advertising and supported modes
> > > based on the speed and duplex of the fixed link.
> > > 
> > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > phy_lookup_settings(). This has the side effect that the matched
> > > settings and configured linkmodes may now contain several linkmodes (the
> > > intersection of supported linkmodes from the phylink settings and the
> > > linkmodes that match speed/duplex) instead of the one from
> > > phy_lookup_settings().  
> > 
> > ...
> >   
> > >  
> > >  	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
> > >  	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> > > @@ -588,9 +591,9 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> > >  
> > >  	phylink_set(pl->supported, MII);
> > >  
> > > -	if (s) {
> > > -		__set_bit(s->bit, pl->supported);
> > > -		__set_bit(s->bit, pl->link_config.lp_advertising);
> > > +	if (c) {
> > > +		linkmode_or(pl->supported, pl->supported, match);
> > > +		linkmode_or(pl->link_config.lp_advertising,
> > > pl->supported, match);  
> > 
> > You are doing the OR twice. You should use linkmode_copy() instead.  
> 
> No, we don't want to copy pl->supported to
> pl->link_config.lp_advertising. We just want to set the linkmode bit
> that corresponds to the speed/duplex in each mask.
> 
> That will result in e.g. the pause mode bits will be overwritten despite
> being appropriately set in the advertising mask in the code above this.

Ok, so the right thing should be this:
linkmode_or(pl->link_config.lp_advertising, pl->link_config.lp_advertising,
	    match)

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Re: [PATCH net-next 12/13] net: phy: phylink: Use phy_caps_lookup for fixed-link configuration
Posted by Maxime Chevallier 11 months, 2 weeks ago 
On Mon, 24 Feb 2025 15:04:40 +0100
Kory Maincent <kory.maincent@bootlin.com> wrote:

> On Mon, 24 Feb 2025 13:53:28 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Mon, Feb 24, 2025 at 02:44:31PM +0100, Kory Maincent wrote:  
> > > On Sat, 22 Feb 2025 15:27:24 +0100
> > > Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
> > >     
> > > > When phylink creates a fixed-link configuration, it finds a matching
> > > > linkmode to set as the advertised, lp_advertising and supported modes
> > > > based on the speed and duplex of the fixed link.
> > > > 
> > > > Use the newly introduced phy_caps_lookup to get these modes instead of
> > > > phy_lookup_settings(). This has the side effect that the matched
> > > > settings and configured linkmodes may now contain several linkmodes (the
> > > > intersection of supported linkmodes from the phylink settings and the
> > > > linkmodes that match speed/duplex) instead of the one from
> > > > phy_lookup_settings().    
> > > 
> > > ...
> > >     
> > > >  
> > > >  	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
> > > >  	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> > > > @@ -588,9 +591,9 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> > > >  
> > > >  	phylink_set(pl->supported, MII);
> > > >  
> > > > -	if (s) {
> > > > -		__set_bit(s->bit, pl->supported);
> > > > -		__set_bit(s->bit, pl->link_config.lp_advertising);
> > > > +	if (c) {
> > > > +		linkmode_or(pl->supported, pl->supported, match);
> > > > +		linkmode_or(pl->link_config.lp_advertising,
> > > > pl->supported, match);    
> > > 
> > > You are doing the OR twice. You should use linkmode_copy() instead.    
> > 
> > No, we don't want to copy pl->supported to
> > pl->link_config.lp_advertising. We just want to set the linkmode bit
> > that corresponds to the speed/duplex in each mask.
> > 
> > That will result in e.g. the pause mode bits will be overwritten despite
> > being appropriately set in the advertising mask in the code above this.  
> 
> Ok, so the right thing should be this:
> linkmode_or(pl->link_config.lp_advertising, pl->link_config.lp_advertising,
> 	    match)

That looks right indeed, I'll address that for the next iteration.

Thanks for reviewing,

Maxime

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.