Re: [net PATCH 2/2] net: phylink: Set advertising based on phy_lookup_setting in ksettings_set

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

On Tue, Apr 01, 2025 at 02:30:13PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> While testing a driver that supports mulitple speeds on the same SFP module
> I noticed I wasn't able to change them when I was not using
> autonegotiation. I would attempt to update the speed, but it had no effect.
> 
> A bit of digging led me to the fact that we weren't updating the advertised
> link mask and as a result the interface wasn't being updated when I
> requested an updated speed. This change makes it so that we apply the speed
> from the phy settings to the config.advertised following a behavior similar
> to what we already do when setting up a fixed-link.
> 
> Fixes: ea269a6f7207 ("net: phylink: Update SFP selected interface on advertising changes")
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> ---
>  drivers/net/phy/phylink.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 380e51c5bdaa..f561a803e5ce 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2763,6 +2763,7 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>  
>  		config.speed = c->speed;
>  		config.duplex = c->duplex;
> +		linkmode_and(config.advertising, c->linkmodes, pl->supported);

I had thought that ethtool provided an appropriate advertising mask
when aneg is disabled, but it just preserves the old mask, which seems
to be the intended behaviour (if one looks at phylib, that's also what
happens there.) We should not deviate from that with a user API.

So, I would like to change how this works somewhat to avoid a user
visible change. Also, interface mode changing on AUTONEG_DISABLED was
never intended to work. Indeed, mvneta and mvpp2 don't support
AUTONEG_DISABLED for 1000BASE-X nor 2500BASE-X which is where this
interface switching was implemented (for switching between these two.)

I've already got rid of the phylink_sfp_select_interface() usage when
a module is inserted (see phylink_sfp_config_optical(), where we base
the interface selection off interface support masks there rather than
advertisements - it used to be advertisements.)

We now have phylink_interface_max_speed() which gives the speed of
the interface, which gives us the possibility of doing something
like this for the AUTONEG_DISABLE state:

	phy_interface_and(interfaces, pl->config->supported_interfaces,
			  pl->sfp_interfaces);

	best_speed = SPEED_UNKNOWN;
	best_interface = PHY_INTERFACE_MODE_NA;

	for_each_set_bit(interface, interfaces, __ETHTOOL_LINK_MODE_MASK_NBITS) {
		max_speed = phylink_interface_max_speed(interface);
		if (max_speed < config.speed)
			continue;
		if (max_speed == config.speed)
			return interface;
		if (best_speed == SPEED_UNKNOWN ||
		    max_speed < best_speed) {
			best_speed = max_speed;
			best_interface = interface;
		}
	}

	return best_interface;

to select the interface from aneg-disabled state.

Do you think that would work for you?

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