| Message ID | 20201117195040.178651-1-vinschen@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | igc: fix link speed advertising | expand |
Ping? It looks like this patch got lost somehow. Without this patch, setting link speed advertising is broken. Thanks, Corinna On Nov 17 20:50, Corinna Vinschen wrote: > Link speed advertising in igc has two problems: > > - When setting the advertisement via ethtool, the link speed is converted > to the legacy 32 bit representation for the intel PHY code. > This inadvertently drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT (being > beyond bit 31). As a result, any call to `ethtool -s ...' drops the > 2500Mbit/s link speed from the PHY settings. Only reloading the driver > alleviates that problem. > > Fix this by converting the ETHTOOL_LINK_MODE_2500baseT_Full_BIT to the > Intel PHY ADVERTISE_2500_FULL bit explicitely. > > - Rather than checking the actual PHY setting, the .get_link_ksettings > function always fills link_modes.advertising with all link speeds > the device is capable of. > > Fix this by checking the PHY autoneg_advertised settings and report > only the actually advertised speeds up to ethtool. > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++----- > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c > index 61d331ce38cd..75cb4ca36bac 100644 > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > @@ -1675,12 +1675,18 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev, > cmd->base.phy_address = hw->phy.addr; > > /* advertising link modes */ > - ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half); > - ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full); > - ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half); > - ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full); > - ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full); > - ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full); > + if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF) > + ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half); > + if (hw->phy.autoneg_advertised & ADVERTISE_10_FULL) > + ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full); > + if (hw->phy.autoneg_advertised & ADVERTISE_100_HALF) > + ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half); > + if (hw->phy.autoneg_advertised & ADVERTISE_100_FULL) > + ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full); > + if (hw->phy.autoneg_advertised & ADVERTISE_1000_FULL) > + ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full); > + if (hw->phy.autoneg_advertised & ADVERTISE_2500_FULL) > + ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full); > > /* set autoneg settings */ > if (hw->mac.autoneg == 1) { > @@ -1792,6 +1798,12 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev, > > ethtool_convert_link_mode_to_legacy_u32(&advertising, > cmd->link_modes.advertising); > + /* Converting to legacy u32 drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT. > + * We have to check this and convert it to ADVERTISE_2500_FULL > + * (aka ETHTOOL_LINK_MODE_2500baseX_Full_BIT) explicitely. > + */ > + if (ethtool_link_ksettings_test_link_mode(cmd, advertising, 2500baseT_Full)) > + advertising |= ADVERTISE_2500_FULL; > > if (cmd->base.autoneg == AUTONEG_ENABLE) { > hw->mac.autoneg = 1; > -- > 2.27.0 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Tue, 26 Jan 2021 11:30:37 +0100 Corinna Vinschen wrote: > Ping? > > It looks like this patch got lost somehow. Without this patch, > setting link speed advertising is broken. Adding Intel maintainers.
On Tue, 2021-01-26 at 12:00 -0800, Jakub Kicinski wrote: > On Tue, 26 Jan 2021 11:30:37 +0100 Corinna Vinschen wrote: > > Ping? > > > > It looks like this patch got lost somehow. Without this patch, > > setting link speed advertising is broken. > > Adding Intel maintainers. I talked to the team and they won't be able to get to testing this patch soon. We have reviewed the code and everything looks correct so I'll submit it in my pull request today. Thanks, Tony
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 61d331ce38cd..75cb4ca36bac 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -1675,12 +1675,18 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev, cmd->base.phy_address = hw->phy.addr; /* advertising link modes */ - ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half); - ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full); - ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half); - ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full); - ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full); - ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full); + if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF) + ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Half); + if (hw->phy.autoneg_advertised & ADVERTISE_10_FULL) + ethtool_link_ksettings_add_link_mode(cmd, advertising, 10baseT_Full); + if (hw->phy.autoneg_advertised & ADVERTISE_100_HALF) + ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Half); + if (hw->phy.autoneg_advertised & ADVERTISE_100_FULL) + ethtool_link_ksettings_add_link_mode(cmd, advertising, 100baseT_Full); + if (hw->phy.autoneg_advertised & ADVERTISE_1000_FULL) + ethtool_link_ksettings_add_link_mode(cmd, advertising, 1000baseT_Full); + if (hw->phy.autoneg_advertised & ADVERTISE_2500_FULL) + ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full); /* set autoneg settings */ if (hw->mac.autoneg == 1) { @@ -1792,6 +1798,12 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev, ethtool_convert_link_mode_to_legacy_u32(&advertising, cmd->link_modes.advertising); + /* Converting to legacy u32 drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT. + * We have to check this and convert it to ADVERTISE_2500_FULL + * (aka ETHTOOL_LINK_MODE_2500baseX_Full_BIT) explicitely. + */ + if (ethtool_link_ksettings_test_link_mode(cmd, advertising, 2500baseT_Full)) + advertising |= ADVERTISE_2500_FULL; if (cmd->base.autoneg == AUTONEG_ENABLE) { hw->mac.autoneg = 1;
Link speed advertising in igc has two problems: - When setting the advertisement via ethtool, the link speed is converted to the legacy 32 bit representation for the intel PHY code. This inadvertently drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT (being beyond bit 31). As a result, any call to `ethtool -s ...' drops the 2500Mbit/s link speed from the PHY settings. Only reloading the driver alleviates that problem. Fix this by converting the ETHTOOL_LINK_MODE_2500baseT_Full_BIT to the Intel PHY ADVERTISE_2500_FULL bit explicitely. - Rather than checking the actual PHY setting, the .get_link_ksettings function always fills link_modes.advertising with all link speeds the device is capable of. Fix this by checking the PHY autoneg_advertised settings and report only the actually advertised speeds up to ethtool. Signed-off-by: Corinna Vinschen <vinschen@redhat.com> --- drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-)