| Message ID | E1q4eLm-00Ayxk-GZ@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 03c44a21d0333a2f469680bc9caaf96a9bfaea87 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phylink: actually fix ksettings_set() ethtool call | expand |
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 01 Jun 2023 10:12:06 +0100 you wrote: > Raju Lakkaraju reported that the below commit caused a regression > with Lan743x drivers and a 2.5G SFP. Sadly, this is because the commit > was utterly wrong. Let's fix this properly by not moving the > linkmode_and(), but instead copying the link ksettings and then > modifying the advertising mask before passing the modified link > ksettings to phylib. > > [...] Here is the summary with links: - [net] net: phylink: actually fix ksettings_set() ethtool call https://git.kernel.org/netdev/net/c/03c44a21d033 You are awesome, thank you!
The 06/01/2023 10:12, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Raju Lakkaraju reported that the below commit caused a regression > with Lan743x drivers and a 2.5G SFP. Sadly, this is because the commit > was utterly wrong. Let's fix this properly by not moving the > linkmode_and(), but instead copying the link ksettings and then > modifying the advertising mask before passing the modified link > ksettings to phylib. > > Fixes: df0acdc59b09 ("net: phylink: fix ksettings_set() ethtool call") > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > Raju, > > Given the number of cockups I've made with this so far, it would be a > really good idea if you can explicitly test this patch and provide a > tested-by. Also it would be good to have a reported-by as well. Tested this patch with 1G Speed Cu SFP (Axcen Photonics - AXGT-R1T4-05I1) with different speeds (1G/100M/10M bps) changes.. Working as expected. Tested-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> Reported-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com> Thanks, Raju > > Thanks. > > drivers/net/phy/phylink.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index e237949deee6..b4831110003c 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -2225,11 +2225,13 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, > > ASSERT_RTNL(); > > - /* Mask out unsupported advertisements */ > - linkmode_and(config.advertising, kset->link_modes.advertising, > - pl->supported); > - > if (pl->phydev) { > + struct ethtool_link_ksettings phy_kset = *kset; > + > + linkmode_and(phy_kset.link_modes.advertising, > + phy_kset.link_modes.advertising, > + pl->supported); > + > /* We can rely on phylib for this update; we also do not need > * to update the pl->link_config settings: > * - the configuration returned via ksettings_get() will come > @@ -2248,10 +2250,13 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, > * the presence of a PHY, this should not be changed as that > * should be determined from the media side advertisement. > */ > - return phy_ethtool_ksettings_set(pl->phydev, kset); > + return phy_ethtool_ksettings_set(pl->phydev, &phy_kset); > } > > config = pl->link_config; > + /* Mask out unsupported advertisements */ > + linkmode_and(config.advertising, kset->link_modes.advertising, > + pl->supported); > > /* FIXME: should we reject autoneg if phy/mac does not support it? */ > switch (kset->base.autoneg) { > -- > 2.30.2 >
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index e237949deee6..b4831110003c 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -2225,11 +2225,13 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, ASSERT_RTNL(); - /* Mask out unsupported advertisements */ - linkmode_and(config.advertising, kset->link_modes.advertising, - pl->supported); - if (pl->phydev) { + struct ethtool_link_ksettings phy_kset = *kset; + + linkmode_and(phy_kset.link_modes.advertising, + phy_kset.link_modes.advertising, + pl->supported); + /* We can rely on phylib for this update; we also do not need * to update the pl->link_config settings: * - the configuration returned via ksettings_get() will come @@ -2248,10 +2250,13 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, * the presence of a PHY, this should not be changed as that * should be determined from the media side advertisement. */ - return phy_ethtool_ksettings_set(pl->phydev, kset); + return phy_ethtool_ksettings_set(pl->phydev, &phy_kset); } config = pl->link_config; + /* Mask out unsupported advertisements */ + linkmode_and(config.advertising, kset->link_modes.advertising, + pl->supported); /* FIXME: should we reject autoneg if phy/mac does not support it? */ switch (kset->base.autoneg) {
Raju Lakkaraju reported that the below commit caused a regression with Lan743x drivers and a 2.5G SFP. Sadly, this is because the commit was utterly wrong. Let's fix this properly by not moving the linkmode_and(), but instead copying the link ksettings and then modifying the advertising mask before passing the modified link ksettings to phylib. Fixes: df0acdc59b09 ("net: phylink: fix ksettings_set() ethtool call") Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- Raju, Given the number of cockups I've made with this so far, it would be a really good idea if you can explicitly test this patch and provide a tested-by. Also it would be good to have a reported-by as well. Thanks. drivers/net/phy/phylink.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)