| Message ID | 174354301312.26800.4565150748823347100.stgit@ahduyck-xeon-server.home.arpa (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes for net/phy/phylink.c | expand |
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?
On Wed, Apr 2, 2025 at 11:02 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > 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? That should work. The only case where it might get iffy would be a QSFP-DD cable that supported both NRZ and PAM4. In that case we might get a 50R1 when we are expecting a 50R2. However that is kind of a problem throughout with all the pure speed/duplex checks. The only way to get around that would be to add a new check for lanes to kind of take the place of duplex as we would need to also have the lanes match.
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); break; case AUTONEG_ENABLE: