netdev - Re: [net PATCH 2/2] net: phylink: Set advertising based on phy_lookup_setting in ksettings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0UfyhFXWRAsW_i3GRQmY-RprruU7gXb8f=-J_5kvRQEMBA@mail.gmail.com>
Date: Wed, 2 Apr 2025 15:34:45 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, andrew@...n.ch, hkallweit1@...il.com, 
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	maxime.chevallier@...tlin.com
Subject: Re: [net PATCH 2/2] net: phylink: Set advertising based on
 phy_lookup_setting in ksettings_set

On Wed, Apr 2, 2025 at 11:02 AM Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> On Tue, Apr 01, 2025 at 02:30:13PM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@...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@...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.