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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

Please use a subject line of "net: phylink: " for phylink patches, not
"net: phy: " which is for phylib.

On Wed, Feb 26, 2025 at 11:09:24AM +0100, Maxime Chevallier 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().
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> V1 -> V2: - fixed pl->link_config.lp_advertising setting
> 
>  drivers/net/phy/phylink.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 6c67d5c9b787..63fbf3d8708a 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -805,9 +805,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;
> @@ -879,8 +880,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);

What's this for? Surely phy_caps_lookup() should not return a link mode
that wasn't in phy_caps_lookup()'s 3rd argument.

Otherwise...

>  	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
>  	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
> @@ -889,9 +892,10 @@ 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->link_config.lp_advertising, match);
>  	} else {
>  		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
>  			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
> @@ -1879,21 +1883,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);

... this is wrong since this is basically doing the same as the above
parsing.

I'm not sure why you're generating different code for what is
essentially the same thing.

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