| Message ID | 20210126221035.658124-8-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 329a3678ec69962aa67c91397efbd46d36635f91 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Intel Wired LAN Driver Updates 2021-01-26 | expand |
On 26.01.2021 23:10, Tony Nguyen wrote: > From: Corinna Vinschen <vinschen@redhat.com> > > 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 explicitly. > > - 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. > > Fixes: 8c5ad0dae93c ("igc: Add ethtool support") > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++----- > 1 file changed, 18 insertions(+), 6 deletions(-) > Would switching to phylib be a mid-term option for you? This could save quite some code and you'd get things like proper 2.5Gbps handling out of the box. Or is there anything that prevents using phylib?
On Wed, 2021-01-27 at 08:04 +0100, Heiner Kallweit wrote: > On 26.01.2021 23:10, Tony Nguyen wrote: > > From: Corinna Vinschen <vinschen@redhat.com> > > > > 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 explicitly. > > > > - 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. > > > > Fixes: 8c5ad0dae93c ("igc: Add ethtool support") > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > --- > > drivers/net/ethernet/intel/igc/igc_ethtool.c | 24 +++++++++++++++- > > ---- > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > Would switching to phylib be a mid-term option for you? > This could save quite some code and you'd get things like proper > 2.5Gbps > handling out of the box. Or is there anything that prevents using > phylib? Phylib is something we can look into though we have some device specific quirks that we would need to see how it could be handled. Since this is fixing a current problem and any transition to phylib would not be immediate, I think this patch should be accepted while we investigate phylib. Thanks, Tony
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 61d331ce38cd..831f2f09de5f 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) explicitly. + */ + 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;