| Message ID | 20240218-keee-u32-cleanup-v4-0-71f13b7c3e60@lunn.ch |
|---|---|
| Headers | show |
| Series | drivers: net: Convert EEE handling to use linkmode bitmaps | expand |
On Sun, Feb 18, 2024 at 11:07:01AM -0600, Andrew Lunn wrote: > Convert the tables to make use of ETHTOOL link mode bits, rather than > the old u32 SUPPORTED speeds. Make use of the linkmode helps to set > bits and compare linkmodes. As a result, the _u32 members of keee are > no longer used, a step towards removing them. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 48 ++++++++++++------------ > 1 file changed, 25 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c ... > @@ -3436,28 +3437,29 @@ ixgbe_get_eee_fw(struct ixgbe_adapter *adapter, struct ethtool_keee *edata) > if (rc) > return rc; > > - edata->lp_advertised_u32 = 0; > for (i = 0; i < ARRAY_SIZE(ixgbe_lp_map); ++i) { > if (info[0] & ixgbe_lp_map[i].lp_advertised) > - edata->lp_advertised_u32 |= ixgbe_lp_map[i].mac_speed; > + linkmode_set_bit(ixgbe_lp_map[i].link_mode, > + edata->lp_advertised); > } > > - edata->supported_u32 = 0; > for (i = 0; i < ARRAY_SIZE(ixgbe_ls_map); ++i) { > if (hw->phy.eee_speeds_supported & ixgbe_ls_map[i].mac_speed) > - edata->supported_u32 |= ixgbe_ls_map[i].supported; > + linkmode_set_bit(ixgbe_lp_map[i].link_mode, > + edata->lp_advertised); Hi Andrew, should this be edata->supported rather than edata->lp_advertised? > } > > - edata->advertised_u32 = 0; > for (i = 0; i < ARRAY_SIZE(ixgbe_ls_map); ++i) { > if (hw->phy.eee_speeds_advertised & ixgbe_ls_map[i].mac_speed) > - edata->advertised_u32 |= ixgbe_ls_map[i].supported; > + linkmode_set_bit(ixgbe_lp_map[i].link_mode, > + edata->advertised); > } > > - edata->eee_enabled = !!edata->advertised_u32; > + edata->eee_enabled = !linkmode_empty(edata->advertised); > edata->tx_lpi_enabled = edata->eee_enabled; > - if (edata->advertised_u32 & edata->lp_advertised_u32) > - edata->eee_active = true; > + > + linkmode_and(common, edata->advertised, edata->lp_advertised); > + edata->eee_active = !linkmode_empty(common); > > return 0; > } ...
On Sun, Feb 18, 2024 at 11:06:59AM -0600, Andrew Lunn wrote: > Make use of the existing linkmode helpers for converting PHY EEE > register values into links modes, now that ethtool_keee uses link > modes, rather than u32 values. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/usb/ax88179_178a.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c > index d6168eaa286f..d4bf9865d87b 100644 > --- a/drivers/net/usb/ax88179_178a.c > +++ b/drivers/net/usb/ax88179_178a.c > @@ -676,21 +676,21 @@ ax88179_ethtool_get_eee(struct usbnet *dev, struct ethtool_keee *data) > MDIO_MMD_PCS); > if (val < 0) > return val; > - data->supported_u32 = mmd_eee_cap_to_ethtool_sup_t(val); > + mii_eee_cap1_mod_linkmode_t(data->supported, val); > > /* Get advertisement EEE */ > val = ax88179_phy_read_mmd_indirect(dev, MDIO_AN_EEE_ADV, > MDIO_MMD_AN); > if (val < 0) > return val; > - data->advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(val); > + mii_eee_cap1_mod_linkmode_t(data->advertised, val); > > /* Get LP advertisement EEE */ > val = ax88179_phy_read_mmd_indirect(dev, MDIO_AN_EEE_LPABLE, > MDIO_MMD_AN); > if (val < 0) > return val; > - data->lp_advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(val); > + mii_eee_cap1_mod_linkmode_t(data->lp_advertised, val); > > return 0; > } > @@ -698,7 +698,7 @@ ax88179_ethtool_get_eee(struct usbnet *dev, struct ethtool_keee *data) > static int > ax88179_ethtool_set_eee(struct usbnet *dev, struct ethtool_keee *data) > { > - u16 tmp16 = ethtool_adv_to_mmd_eee_adv_t(data->advertised_u32); > + u16 tmp16 = linkmode_to_mii_eee_cap1_t(data->advertised); > > return ax88179_phy_write_mmd_indirect(dev, MDIO_AN_EEE_ADV, > MDIO_MMD_AN, tmp16); > @@ -1663,7 +1663,6 @@ static int ax88179_reset(struct usbnet *dev) > ax88179_disable_eee(dev); > > ax88179_ethtool_get_eee(dev, &eee_data); > - eee_data.advertised_u32 = 0; Hi Andrew, could you clarify why advertised no longer needs to be cleared? > ax88179_ethtool_set_eee(dev, &eee_data); > > /* Restart autoneg */ > > -- > 2.43.0 > >
On Sun, Feb 18, 2024 at 11:07:03AM -0600, Andrew Lunn wrote: > Make use of the existing linkmode helpers for converting PHY EEE > register values into links modes, now that ethtool_keee uses link > modes, rather than u32 values. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Reviewed-by: Simon Horman <horms@kernel.org>
On Sun, Feb 18, 2024 at 11:07:05AM -0600, Andrew Lunn wrote: > Make use of the existing linkmode helpers for converting PHY EEE > register values into links modes, now that ethtool_keee uses link > modes, rather than u32 values. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Reviewed-by: Simon Horman <horms@kernel.org>
On Sun, Feb 18, 2024 at 11:06:58AM -0600, Andrew Lunn wrote: > Make use of the existing linkmode helpers for converting PHY EEE > register values into links modes, now that ethtool_keee uses link > modes, rather than u32 values. > > Rework determining if EEE is active to make is similar as to how > phylib decides, and make use of a phylib helper to validate if EEE is > valid in for the current link mode. This then requires that PHYLIB is > selected. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Reviewed-by: Simon Horman <horms@kernel.org>
On Tue, Feb 20, 2024 at 12:39:24PM +0000, Simon Horman wrote: > On Sun, Feb 18, 2024 at 11:06:59AM -0600, Andrew Lunn wrote: > > Make use of the existing linkmode helpers for converting PHY EEE > > register values into links modes, now that ethtool_keee uses link > > modes, rather than u32 values. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/usb/ax88179_178a.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c > > index d6168eaa286f..d4bf9865d87b 100644 > > --- a/drivers/net/usb/ax88179_178a.c > > +++ b/drivers/net/usb/ax88179_178a.c > > @@ -676,21 +676,21 @@ ax88179_ethtool_get_eee(struct usbnet *dev, struct ethtool_keee *data) > > MDIO_MMD_PCS); > > if (val < 0) > > return val; > > - data->supported_u32 = mmd_eee_cap_to_ethtool_sup_t(val); > > + mii_eee_cap1_mod_linkmode_t(data->supported, val); > > > > /* Get advertisement EEE */ > > val = ax88179_phy_read_mmd_indirect(dev, MDIO_AN_EEE_ADV, > > MDIO_MMD_AN); > > if (val < 0) > > return val; > > - data->advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(val); > > + mii_eee_cap1_mod_linkmode_t(data->advertised, val); > > > > /* Get LP advertisement EEE */ > > val = ax88179_phy_read_mmd_indirect(dev, MDIO_AN_EEE_LPABLE, > > MDIO_MMD_AN); > > if (val < 0) > > return val; > > - data->lp_advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(val); > > + mii_eee_cap1_mod_linkmode_t(data->lp_advertised, val); > > > > return 0; > > } > > @@ -698,7 +698,7 @@ ax88179_ethtool_get_eee(struct usbnet *dev, struct ethtool_keee *data) > > static int > > ax88179_ethtool_set_eee(struct usbnet *dev, struct ethtool_keee *data) > > { > > - u16 tmp16 = ethtool_adv_to_mmd_eee_adv_t(data->advertised_u32); > > + u16 tmp16 = linkmode_to_mii_eee_cap1_t(data->advertised); > > > > return ax88179_phy_write_mmd_indirect(dev, MDIO_AN_EEE_ADV, > > MDIO_MMD_AN, tmp16); > > @@ -1663,7 +1663,6 @@ static int ax88179_reset(struct usbnet *dev) > > ax88179_disable_eee(dev); > > > > ax88179_ethtool_get_eee(dev, &eee_data); > > - eee_data.advertised_u32 = 0; > > Hi Andrew, > > could you clarify why advertised no longer needs to be cleared? Ah, that is me being too delete happy. When the ethtool core calls into the driver for eee_get(), it first zeros the structure passed in. Some drivers than again zeroed members, so i deleted them. However, this is not a eee_get() call, eee_data is actually a stack variable. ax88179_ethtool_get_eee() has set it, so it is at least not random junk. But the intention here is to not advertise any EEE link modes until the user calls set_eee(). So this zero'ing is needed. Good catch, thanks. Andrew --- pw-bot: cr
On Tue, Feb 20, 2024 at 12:06:43PM +0000, Simon Horman wrote: > On Sun, Feb 18, 2024 at 11:07:01AM -0600, Andrew Lunn wrote: > > Convert the tables to make use of ETHTOOL link mode bits, rather than > > the old u32 SUPPORTED speeds. Make use of the linkmode helps to set > > bits and compare linkmodes. As a result, the _u32 members of keee are > > no longer used, a step towards removing them. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 48 ++++++++++++------------ > > 1 file changed, 25 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > > ... > > > @@ -3436,28 +3437,29 @@ ixgbe_get_eee_fw(struct ixgbe_adapter *adapter, struct ethtool_keee *edata) > > if (rc) > > return rc; > > > > - edata->lp_advertised_u32 = 0; > > for (i = 0; i < ARRAY_SIZE(ixgbe_lp_map); ++i) { > > if (info[0] & ixgbe_lp_map[i].lp_advertised) > > - edata->lp_advertised_u32 |= ixgbe_lp_map[i].mac_speed; > > + linkmode_set_bit(ixgbe_lp_map[i].link_mode, > > + edata->lp_advertised); > > } > > > > - edata->supported_u32 = 0; > > for (i = 0; i < ARRAY_SIZE(ixgbe_ls_map); ++i) { > > if (hw->phy.eee_speeds_supported & ixgbe_ls_map[i].mac_speed) > > - edata->supported_u32 |= ixgbe_ls_map[i].supported; > > + linkmode_set_bit(ixgbe_lp_map[i].link_mode, > > + edata->lp_advertised); > > Hi Andrew, > > should this be edata->supported rather than edata->lp_advertised? Correct :-( Andrew
EEE has until recently been limited to lower speeds due to the use of the legacy u32 for link speeds. This restriction has been lifted, with the use of linkmode bitmaps, added in the following patches: 1f069de63602 ethtool: add linkmode bitmap support to struct ethtool_keee 1d756ff13da6 ethtool: add suffix _u32 to legacy bitmap members of struct ethtool_keee 285cc15cc555 ethtool: adjust struct ethtool_keee to kernel needs 0b3100bc8fa7 ethtool: switch back from ethtool_keee to ethtool_eee for ioctl d80a52335374 ethtool: replace struct ethtool_eee with a new struct ethtool_keee on kernel side This patchset converts the remaining MAC drivers still using the old _u32 to link modes. A couple of Intel drivers do odd things with EEE, setting the autoneg bit. It is unclear why, no other driver does, ethtool does not display it, and EEE is always negotiated. One patch in this series deletes this code. With all users of the legacy _u32 changed to link modes, the _u32 values are removed from keee, and support for them in the ethtool core is removed. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- Changes in v4: - Add missing conversion in igb - Add missing conversion in r8152 - Add patch to remove now unused _u32 members - Link to v3: https://lore.kernel.org/r/20240217-keee-u32-cleanup-v3-0-fcf6b62a0c7f@lunn.ch Changes in v3: - Add list of commits adding linkmodes to EEE to cover letter - Fix grammar error in cover letter. - Add Reviewed-by from Jacob Keller - Link to v2: https://lore.kernel.org/r/20240214-keee-u32-cleanup-v2-0-4ac534b83d66@lunn.ch Changes in v2: - igb: Fix type 100BaseT to 1000BaseT. - Link to v1: https://lore.kernel.org/r/20240204-keee-u32-cleanup-v1-0-fb6e08329d9a@lunn.ch --- Andrew Lunn (9): net: usb: r8152: Use linkmode helpers for EEE net: usb: ax88179_178a: Use linkmode helpers for EEE net: qlogic: qede: Use linkmode helpers for EEE net: ethernet: ixgbe: Convert EEE to use linkmodes net: intel: i40e/igc: Remove setting Autoneg in EEE capabilities net: intel: e1000e: Use linkmode helpers for EEE net: intel: igb: Use linkmode helpers for EEE net: intel: igc: Use linkmode helpers for EEE net: ethtool: eee: Remove legacy _u32 from keee drivers/net/ethernet/intel/e1000e/ethtool.c | 17 +++++-- drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 7 +-- drivers/net/ethernet/intel/igb/igb_ethtool.c | 35 +++++++++----- drivers/net/ethernet/intel/igc/igc_ethtool.c | 13 ++--- drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 48 ++++++++++--------- drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 60 +++++++++++++++--------- drivers/net/usb/Kconfig | 1 + drivers/net/usb/ax88179_178a.c | 9 ++-- drivers/net/usb/r8152.c | 33 +++++++------ include/linux/ethtool.h | 3 -- net/ethtool/eee.c | 31 ++---------- net/ethtool/ioctl.c | 29 ++++-------- 12 files changed, 139 insertions(+), 147 deletions(-) --- base-commit: a6e0cb150c514efba4aaba4069927de43d80bb59 change-id: 20240204-keee-u32-cleanup-b86d68458d80 Best regards,