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 <[email protected]>
---
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/[email protected]
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/[email protected]
Changes in v2:
- igb: Fix type 100BaseT to 1000BaseT.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
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,
--
Andrew Lunn <[email protected]>
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 <[email protected]>
---
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;
ax88179_ethtool_set_eee(dev, &eee_data);
/* Restart autoneg */
--
2.43.0
Make use of the existing linkmode helpers for bit manipulation of EEE
advertise, support and link partner support. The aim is to drop the
restricted _u32 variants in the near future.
Signed-off-by: Andrew Lunn <[email protected]>
---
drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 60 ++++++++++++++++---------
1 file changed, 38 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index dfa15619fd78..ae3ebf0cf999 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -1789,18 +1789,26 @@ static int qede_get_eee(struct net_device *dev, struct ethtool_keee *edata)
return -EOPNOTSUPP;
}
- if (current_link.eee.adv_caps & QED_EEE_1G_ADV)
- edata->advertised_u32 = ADVERTISED_1000baseT_Full;
- if (current_link.eee.adv_caps & QED_EEE_10G_ADV)
- edata->advertised_u32 |= ADVERTISED_10000baseT_Full;
- if (current_link.sup_caps & QED_EEE_1G_ADV)
- edata->supported_u32 = ADVERTISED_1000baseT_Full;
- if (current_link.sup_caps & QED_EEE_10G_ADV)
- edata->supported_u32 |= ADVERTISED_10000baseT_Full;
- if (current_link.eee.lp_adv_caps & QED_EEE_1G_ADV)
- edata->lp_advertised_u32 = ADVERTISED_1000baseT_Full;
- if (current_link.eee.lp_adv_caps & QED_EEE_10G_ADV)
- edata->lp_advertised_u32 |= ADVERTISED_10000baseT_Full;
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ edata->advertised,
+ current_link.eee.adv_caps & QED_EEE_1G_ADV);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ edata->advertised,
+ current_link.eee.adv_caps & QED_EEE_10G_ADV);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ edata->supported,
+ current_link.sup_caps & QED_EEE_1G_ADV);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ edata->supported,
+ current_link.sup_caps & QED_EEE_10G_ADV);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ edata->lp_advertised,
+ current_link.eee.lp_adv_caps & QED_EEE_1G_ADV);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ edata->lp_advertised,
+ current_link.eee.lp_adv_caps & QED_EEE_10G_ADV);
edata->tx_lpi_timer = current_link.eee.tx_lpi_timer;
edata->eee_enabled = current_link.eee.enable;
@@ -1812,9 +1820,12 @@ static int qede_get_eee(struct net_device *dev, struct ethtool_keee *edata)
static int qede_set_eee(struct net_device *dev, struct ethtool_keee *edata)
{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = {};
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(tmp) = {};
struct qede_dev *edev = netdev_priv(dev);
struct qed_link_output current_link;
struct qed_link_params params;
+ bool unsupp;
if (!edev->ops->common->can_link_change(edev->cdev)) {
DP_INFO(edev, "Link settings are not allowed to be changed\n");
@@ -1832,21 +1843,26 @@ static int qede_set_eee(struct net_device *dev, struct ethtool_keee *edata)
memset(¶ms, 0, sizeof(params));
params.override_flags |= QED_LINK_OVERRIDE_EEE_CONFIG;
- if (!(edata->advertised_u32 & (ADVERTISED_1000baseT_Full |
- ADVERTISED_10000baseT_Full)) ||
- ((edata->advertised_u32 & (ADVERTISED_1000baseT_Full |
- ADVERTISED_10000baseT_Full)) !=
- edata->advertised_u32)) {
+ linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ supported);
+
+ unsupp = linkmode_andnot(tmp, edata->advertised, supported);
+ if (unsupp) {
DP_VERBOSE(edev, QED_MSG_DEBUG,
- "Invalid advertised capabilities %d\n",
- edata->advertised_u32);
+ "Invalid advertised capabilities %*pb\n",
+ __ETHTOOL_LINK_MODE_MASK_NBITS, edata->advertised);
return -EINVAL;
}
- if (edata->advertised_u32 & ADVERTISED_1000baseT_Full)
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ edata->advertised))
params.eee.adv_caps = QED_EEE_1G_ADV;
- if (edata->advertised_u32 & ADVERTISED_10000baseT_Full)
- params.eee.adv_caps |= QED_EEE_10G_ADV;
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ edata->advertised))
+ params.eee.adv_caps = QED_EEE_10G_ADV;
+
params.eee.enable = edata->eee_enabled;
params.eee.tx_lpi_enable = edata->tx_lpi_enabled;
params.eee.tx_lpi_timer = edata->tx_lpi_timer;
--
2.43.0
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 <[email protected]>
---
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
index b1e7338a4ed1..a0879f125146 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -3403,30 +3403,31 @@ static int ixgbe_get_module_eeprom(struct net_device *dev,
static const struct {
ixgbe_link_speed mac_speed;
- u32 supported;
+ u32 link_mode;
} ixgbe_ls_map[] = {
- { IXGBE_LINK_SPEED_10_FULL, SUPPORTED_10baseT_Full },
- { IXGBE_LINK_SPEED_100_FULL, SUPPORTED_100baseT_Full },
- { IXGBE_LINK_SPEED_1GB_FULL, SUPPORTED_1000baseT_Full },
- { IXGBE_LINK_SPEED_2_5GB_FULL, SUPPORTED_2500baseX_Full },
- { IXGBE_LINK_SPEED_10GB_FULL, SUPPORTED_10000baseT_Full },
+ { IXGBE_LINK_SPEED_10_FULL, ETHTOOL_LINK_MODE_10baseT_Full_BIT },
+ { IXGBE_LINK_SPEED_100_FULL, ETHTOOL_LINK_MODE_100baseT_Full_BIT },
+ { IXGBE_LINK_SPEED_1GB_FULL, ETHTOOL_LINK_MODE_1000baseT_Full_BIT },
+ { IXGBE_LINK_SPEED_2_5GB_FULL, ETHTOOL_LINK_MODE_2500baseX_Full_BIT },
+ { IXGBE_LINK_SPEED_10GB_FULL, ETHTOOL_LINK_MODE_10000baseT_Full_BIT },
};
static const struct {
u32 lp_advertised;
- u32 mac_speed;
+ u32 link_mode;
} ixgbe_lp_map[] = {
- { FW_PHY_ACT_UD_2_100M_TX_EEE, SUPPORTED_100baseT_Full },
- { FW_PHY_ACT_UD_2_1G_T_EEE, SUPPORTED_1000baseT_Full },
- { FW_PHY_ACT_UD_2_10G_T_EEE, SUPPORTED_10000baseT_Full },
- { FW_PHY_ACT_UD_2_1G_KX_EEE, SUPPORTED_1000baseKX_Full },
- { FW_PHY_ACT_UD_2_10G_KX4_EEE, SUPPORTED_10000baseKX4_Full },
- { FW_PHY_ACT_UD_2_10G_KR_EEE, SUPPORTED_10000baseKR_Full},
+ { FW_PHY_ACT_UD_2_100M_TX_EEE, ETHTOOL_LINK_MODE_100baseT_Full_BIT },
+ { FW_PHY_ACT_UD_2_1G_T_EEE, ETHTOOL_LINK_MODE_1000baseT_Full_BIT },
+ { FW_PHY_ACT_UD_2_10G_T_EEE, ETHTOOL_LINK_MODE_10000baseT_Full_BIT },
+ { FW_PHY_ACT_UD_2_1G_KX_EEE, ETHTOOL_LINK_MODE_1000baseKX_Full_BIT },
+ { FW_PHY_ACT_UD_2_10G_KX4_EEE, ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT },
+ { FW_PHY_ACT_UD_2_10G_KR_EEE, ETHTOOL_LINK_MODE_10000baseKR_Full_BIT},
};
static int
ixgbe_get_eee_fw(struct ixgbe_adapter *adapter, struct ethtool_keee *edata)
{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(common);
u32 info[FW_PHY_ACT_DATA_COUNT] = { 0 };
struct ixgbe_hw *hw = &adapter->hw;
int rc;
@@ -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);
}
- 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;
}
@@ -3504,7 +3506,7 @@ static int ixgbe_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
return -EINVAL;
}
- if (eee_data.advertised_u32 != edata->advertised_u32) {
+ if (!linkmode_equal(eee_data.advertised, edata->advertised)) {
e_err(drv,
"Setting EEE advertised speeds is not supported\n");
return -EINVAL;
--
2.43.0
Energy Efficient Ethernet should always be negotiated with the link
peer. Don't include SUPPORTED_Autoneg in the results of get_eee() for
supported, advertised or lp_advertised, since it is
assumed. Additionally, ethtool(1) ignores the set bit, and no other
driver sets this.
Reviewed-by: Jacob Keller <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
---
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 7 +------
drivers/net/ethernet/intel/igc/igc_ethtool.c | 4 ----
2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 1b5473358e1a..42e7e6cdaa6d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -5664,16 +5664,12 @@ static int i40e_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
if (phy_cfg.eee_capability == 0)
return -EOPNOTSUPP;
- edata->supported_u32 = SUPPORTED_Autoneg;
- edata->lp_advertised_u32 = edata->supported_u32;
-
/* Get current configuration */
status = i40e_aq_get_phy_capabilities(hw, false, false, &phy_cfg, NULL);
if (status)
return -EAGAIN;
- edata->advertised_u32 = phy_cfg.eee_capability ? SUPPORTED_Autoneg : 0U;
- edata->eee_enabled = !!edata->advertised_u32;
+ edata->eee_enabled = !!phy_cfg.eee_capability;
edata->tx_lpi_enabled = pf->stats.tx_lpi_status;
edata->eee_active = pf->stats.tx_lpi_status && pf->stats.rx_lpi_status;
@@ -5691,7 +5687,6 @@ static int i40e_is_eee_param_supported(struct net_device *netdev,
u32 value;
const char *name;
} param[] = {
- {edata->advertised_u32 & ~SUPPORTED_Autoneg, "advertise"},
{edata->tx_lpi_timer, "tx-timer"},
{edata->tx_lpi_enabled != pf->stats.tx_lpi_status, "tx-lpi"}
};
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 47c797dd2cd9..ac92d10a3e97 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1634,7 +1634,6 @@ static int igc_ethtool_get_eee(struct net_device *netdev,
mmd_eee_adv_to_ethtool_adv_t(adapter->eee_advert);
*edata = adapter->eee;
- edata->supported_u32 = SUPPORTED_Autoneg;
eeer = rd32(IGC_EEER);
@@ -1647,9 +1646,6 @@ static int igc_ethtool_get_eee(struct net_device *netdev,
edata->eee_enabled = hw->dev_spec._base.eee_enable;
- edata->advertised_u32 = SUPPORTED_Autoneg;
- edata->lp_advertised_u32 = SUPPORTED_Autoneg;
-
/* Report correct negotiated EEE status for devices that
* wrongly report EEE at half-duplex
*/
--
2.43.0
All MAC drivers have been converted to use the link mode members of
keee. So remove the _u32 values, and the code in the ethtool core to
convert the legacy _u32 values to link modes.
Signed-off-by: Andrew Lunn <[email protected]>
---
include/linux/ethtool.h | 3 ---
net/ethtool/eee.c | 31 ++++---------------------------
net/ethtool/ioctl.c | 29 ++++++++++-------------------
3 files changed, 14 insertions(+), 49 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index b90c33607594..9901e563f706 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -226,9 +226,6 @@ struct ethtool_keee {
__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
__ETHTOOL_DECLARE_LINK_MODE_MASK(advertised);
__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertised);
- u32 supported_u32;
- u32 advertised_u32;
- u32 lp_advertised_u32;
u32 tx_lpi_timer;
bool tx_lpi_enabled;
bool eee_active;
diff --git a/net/ethtool/eee.c b/net/ethtool/eee.c
index db6faa18fe41..bf398973eb8a 100644
--- a/net/ethtool/eee.c
+++ b/net/ethtool/eee.c
@@ -4,9 +4,6 @@
#include "common.h"
#include "bitset.h"
-#define EEE_MODES_COUNT \
- (sizeof_field(struct ethtool_keee, supported_u32) * BITS_PER_BYTE)
-
struct eee_req_info {
struct ethnl_req_info base;
};
@@ -41,15 +38,6 @@ static int eee_prepare_data(const struct ethnl_req_info *req_base,
ret = dev->ethtool_ops->get_eee(dev, eee);
ethnl_ops_complete(dev);
- if (!ret && !ethtool_eee_use_linkmodes(eee)) {
- ethtool_convert_legacy_u32_to_link_mode(eee->supported,
- eee->supported_u32);
- ethtool_convert_legacy_u32_to_link_mode(eee->advertised,
- eee->advertised_u32);
- ethtool_convert_legacy_u32_to_link_mode(eee->lp_advertised,
- eee->lp_advertised_u32);
- }
-
return ret;
}
@@ -62,11 +50,6 @@ static int eee_reply_size(const struct ethnl_req_info *req_base,
int len = 0;
int ret;
- BUILD_BUG_ON(sizeof(eee->advertised_u32) * BITS_PER_BYTE !=
- EEE_MODES_COUNT);
- BUILD_BUG_ON(sizeof(eee->lp_advertised_u32) * BITS_PER_BYTE !=
- EEE_MODES_COUNT);
-
/* MODES_OURS */
ret = ethnl_bitset_size(eee->advertised, eee->supported,
__ETHTOOL_LINK_MODE_MASK_NBITS,
@@ -154,16 +137,10 @@ ethnl_set_eee(struct ethnl_req_info *req_info, struct genl_info *info)
if (ret < 0)
return ret;
- if (ethtool_eee_use_linkmodes(&eee)) {
- ret = ethnl_update_bitset(eee.advertised,
- __ETHTOOL_LINK_MODE_MASK_NBITS,
- tb[ETHTOOL_A_EEE_MODES_OURS],
- link_mode_names, info->extack, &mod);
- } else {
- ret = ethnl_update_bitset32(&eee.advertised_u32, EEE_MODES_COUNT,
- tb[ETHTOOL_A_EEE_MODES_OURS],
- link_mode_names, info->extack, &mod);
- }
+ ret = ethnl_update_bitset(eee.advertised,
+ __ETHTOOL_LINK_MODE_MASK_NBITS,
+ tb[ETHTOOL_A_EEE_MODES_OURS],
+ link_mode_names, info->extack, &mod);
if (ret < 0)
return ret;
ethnl_update_bool(&eee.eee_enabled, tb[ETHTOOL_A_EEE_ENABLED], &mod);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 1763e8b697e1..5464f237d8dd 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1513,9 +1513,6 @@ static void eee_to_keee(struct ethtool_keee *keee,
{
memset(keee, 0, sizeof(*keee));
- keee->supported_u32 = eee->supported;
- keee->advertised_u32 = eee->advertised;
- keee->lp_advertised_u32 = eee->lp_advertised;
keee->eee_active = eee->eee_active;
keee->eee_enabled = eee->eee_enabled;
keee->tx_lpi_enabled = eee->tx_lpi_enabled;
@@ -1532,6 +1529,8 @@ static void eee_to_keee(struct ethtool_keee *keee,
static void keee_to_eee(struct ethtool_eee *eee,
const struct ethtool_keee *keee)
{
+ bool overflow;
+
memset(eee, 0, sizeof(*eee));
eee->eee_active = keee->eee_active;
@@ -1539,22 +1538,14 @@ static void keee_to_eee(struct ethtool_eee *eee,
eee->tx_lpi_enabled = keee->tx_lpi_enabled;
eee->tx_lpi_timer = keee->tx_lpi_timer;
- if (ethtool_eee_use_linkmodes(keee)) {
- bool overflow;
-
- overflow = !ethtool_convert_link_mode_to_legacy_u32(&eee->supported,
- keee->supported);
- ethtool_convert_link_mode_to_legacy_u32(&eee->advertised,
- keee->advertised);
- ethtool_convert_link_mode_to_legacy_u32(&eee->lp_advertised,
- keee->lp_advertised);
- if (overflow)
- pr_warn("Ethtool ioctl interface doesn't support passing EEE linkmodes beyond bit 32\n");
- } else {
- eee->supported = keee->supported_u32;
- eee->advertised = keee->advertised_u32;
- eee->lp_advertised = keee->lp_advertised_u32;
- }
+ overflow = !ethtool_convert_link_mode_to_legacy_u32(&eee->supported,
+ keee->supported);
+ ethtool_convert_link_mode_to_legacy_u32(&eee->advertised,
+ keee->advertised);
+ ethtool_convert_link_mode_to_legacy_u32(&eee->lp_advertised,
+ keee->lp_advertised);
+ if (overflow)
+ pr_warn("Ethtool ioctl interface doesn't support passing EEE linkmodes beyond bit 32\n");
}
static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)
--
2.43.0
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 <[email protected]>
---
drivers/net/ethernet/intel/igc/igc_ethtool.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index ac92d10a3e97..1a64f1ca6ca8 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1630,8 +1630,8 @@ static int igc_ethtool_get_eee(struct net_device *netdev,
u32 eeer;
if (hw->dev_spec._base.eee_enable)
- edata->advertised_u32 =
- mmd_eee_adv_to_ethtool_adv_t(adapter->eee_advert);
+ mii_eee_cap1_mod_linkmode_t(edata->advertised,
+ adapter->eee_advert);
*edata = adapter->eee;
@@ -1653,7 +1653,7 @@ static int igc_ethtool_get_eee(struct net_device *netdev,
edata->eee_enabled = false;
edata->eee_active = false;
edata->tx_lpi_enabled = false;
- edata->advertised_u32 &= ~edata->advertised_u32;
+ linkmode_zero(edata->advertised);
}
return 0;
@@ -1695,7 +1695,8 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
return -EINVAL;
}
- adapter->eee_advert = ethtool_adv_to_mmd_eee_adv_t(edata->advertised_u32);
+ adapter->eee_advert = linkmode_to_mii_eee_cap1_t(edata->advertised);
+
if (hw->dev_spec._base.eee_enable != edata->eee_enabled) {
hw->dev_spec._base.eee_enable = edata->eee_enabled;
adapter->flags |= IGC_FLAG_EEE;
--
2.43.0
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 <[email protected]>
---
drivers/net/ethernet/intel/e1000e/ethtool.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index ff243ae71b78..dc553c51d79a 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -2223,16 +2223,16 @@ static int e1000e_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
ret_val = e1000_read_emi_reg_locked(hw, cap_addr, &phy_data);
if (ret_val)
goto release;
- edata->supported_u32 = mmd_eee_cap_to_ethtool_sup_t(phy_data);
+ mii_eee_cap1_mod_linkmode_t(edata->supported, phy_data);
/* EEE Advertised */
- edata->advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(adapter->eee_advert);
+ mii_eee_cap1_mod_linkmode_t(edata->advertised, adapter->eee_advert);
/* EEE Link Partner Advertised */
ret_val = e1000_read_emi_reg_locked(hw, lpa_addr, &phy_data);
if (ret_val)
goto release;
- edata->lp_advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(phy_data);
+ mii_eee_cap1_mod_linkmode_t(edata->lp_advertised, phy_data);
/* EEE PCS Status */
ret_val = e1000_read_emi_reg_locked(hw, pcs_stat_addr, &phy_data);
@@ -2265,6 +2265,8 @@ static int e1000e_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
static int e1000e_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = {};
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(tmp) = {};
struct e1000_hw *hw = &adapter->hw;
struct ethtool_keee eee_curr;
s32 ret_val;
@@ -2283,12 +2285,17 @@ static int e1000e_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
return -EINVAL;
}
- if (edata->advertised_u32 & ~(ADVERTISE_100_FULL | ADVERTISE_1000_FULL)) {
+ linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ supported);
+
+ if (linkmode_andnot(tmp, edata->advertised, supported)) {
e_err("EEE advertisement supports only 100TX and/or 1000T full-duplex\n");
return -EINVAL;
}
- adapter->eee_advert = ethtool_adv_to_mmd_eee_adv_t(edata->advertised_u32);
+ adapter->eee_advert = linkmode_to_mii_eee_cap1_t(edata->advertised);
hw->dev_spec.ich8lan.eee_disable = !edata->eee_enabled;
--
2.43.0
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 <[email protected]>
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 35 ++++++++++++++++++----------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index b87b23d2151c..99977a22b843 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -3038,11 +3038,13 @@ static int igb_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
(hw->phy.media_type != e1000_media_type_copper))
return -EOPNOTSUPP;
- edata->supported_u32 = (SUPPORTED_1000baseT_Full |
- SUPPORTED_100baseT_Full);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ edata->supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ edata->supported);
if (!hw->dev_spec._82575.eee_disable)
- edata->advertised_u32 =
- mmd_eee_adv_to_ethtool_adv_t(adapter->eee_advert);
+ mii_eee_cap1_mod_linkmode_t(edata->advertised,
+ adapter->eee_advert);
/* The IPCNFG and EEER registers are not supported on I354. */
if (hw->mac.type == e1000_i354) {
@@ -3068,7 +3070,7 @@ static int igb_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
if (ret_val)
return -ENODATA;
- edata->lp_advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(phy_data);
+ mii_eee_cap1_mod_linkmode_t(edata->lp_advertised, phy_data);
break;
case e1000_i354:
case e1000_i210:
@@ -3079,7 +3081,7 @@ static int igb_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
if (ret_val)
return -ENODATA;
- edata->lp_advertised_u32 = mmd_eee_adv_to_ethtool_adv_t(phy_data);
+ mii_eee_cap1_mod_linkmode_t(edata->lp_advertised, phy_data);
break;
default:
@@ -3099,7 +3101,7 @@ static int igb_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
edata->eee_enabled = false;
edata->eee_active = false;
edata->tx_lpi_enabled = false;
- edata->advertised_u32 &= ~edata->advertised_u32;
+ linkmode_zero(edata->advertised);
}
return 0;
@@ -3109,6 +3111,8 @@ static int igb_set_eee(struct net_device *netdev,
struct ethtool_keee *edata)
{
struct igb_adapter *adapter = netdev_priv(netdev);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = {};
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(tmp) = {};
struct e1000_hw *hw = &adapter->hw;
struct ethtool_keee eee_curr;
bool adv1g_eee = true, adv100m_eee = true;
@@ -3138,14 +3142,21 @@ static int igb_set_eee(struct net_device *netdev,
return -EINVAL;
}
- if (!edata->advertised_u32 || (edata->advertised_u32 &
- ~(ADVERTISE_100_FULL | ADVERTISE_1000_FULL))) {
+ linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ supported);
+ if (linkmode_andnot(tmp, edata->advertised, supported)) {
dev_err(&adapter->pdev->dev,
"EEE Advertisement supports only 100Tx and/or 100T full duplex\n");
return -EINVAL;
}
- adv100m_eee = !!(edata->advertised_u32 & ADVERTISE_100_FULL);
- adv1g_eee = !!(edata->advertised_u32 & ADVERTISE_1000_FULL);
+ adv100m_eee = linkmode_test_bit(
+ ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ edata->advertised);
+ adv1g_eee = linkmode_test_bit(
+ ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ edata->advertised);
} else if (!edata->eee_enabled) {
dev_err(&adapter->pdev->dev,
@@ -3153,7 +3164,7 @@ static int igb_set_eee(struct net_device *netdev,
return -EINVAL;
}
- adapter->eee_advert = ethtool_adv_to_mmd_eee_adv_t(edata->advertised_u32);
+ adapter->eee_advert = linkmode_to_mii_eee_cap1_t(edata->advertised);
if (hw->dev_spec._82575.eee_disable != !edata->eee_enabled) {
hw->dev_spec._82575.eee_disable = !edata->eee_enabled;
adapter->flags |= IGB_FLAG_EEE;
--
2.43.0
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 <[email protected]>
> ---
> 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: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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Sun, Feb 18, 2024 at 11:07:04AM -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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Sun, Feb 18, 2024 at 11:07:06AM -0600, Andrew Lunn wrote:
> All MAC drivers have been converted to use the link mode members of
> keee. So remove the _u32 values, and the code in the ethtool core to
> convert the legacy _u32 values to link modes.
>
> Signed-off-by: Andrew Lunn <[email protected]>
> ---
> include/linux/ethtool.h | 3 ---
> net/ethtool/eee.c | 31 ++++---------------------------
> net/ethtool/ioctl.c | 29 ++++++++++-------------------
> 3 files changed, 14 insertions(+), 49 deletions(-)
Nice :)
Reviewed-by: Simon Horman <[email protected]>
..
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 <[email protected]>
> ---
> 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:00AM -0600, Andrew Lunn wrote:
> Make use of the existing linkmode helpers for bit manipulation of EEE
> advertise, support and link partner support. The aim is to drop the
> restricted _u32 variants in the near future.
>
> Signed-off-by: Andrew Lunn <[email protected]>
Thanks Andrew,
the nit below notwithstanding this looks good to me.
Reviewed-by: Simon Horman <[email protected]>
> ---
> drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 60 ++++++++++++++++---------
> 1 file changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
..
> @@ -1812,9 +1820,12 @@ static int qede_get_eee(struct net_device *dev, struct ethtool_keee *edata)
>
> static int qede_set_eee(struct net_device *dev, struct ethtool_keee *edata)
> {
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = {};
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(tmp) = {};
> struct qede_dev *edev = netdev_priv(dev);
> struct qed_link_output current_link;
> struct qed_link_params params;
> + bool unsupp;
>
> if (!edev->ops->common->can_link_change(edev->cdev)) {
> DP_INFO(edev, "Link settings are not allowed to be changed\n");
> @@ -1832,21 +1843,26 @@ static int qede_set_eee(struct net_device *dev, struct ethtool_keee *edata)
> memset(¶ms, 0, sizeof(params));
> params.override_flags |= QED_LINK_OVERRIDE_EEE_CONFIG;
>
> - if (!(edata->advertised_u32 & (ADVERTISED_1000baseT_Full |
> - ADVERTISED_10000baseT_Full)) ||
> - ((edata->advertised_u32 & (ADVERTISED_1000baseT_Full |
> - ADVERTISED_10000baseT_Full)) !=
> - edata->advertised_u32)) {
> + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> + supported);
> + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> + supported);
> +
> + unsupp = linkmode_andnot(tmp, edata->advertised, supported);
nit: Given the types involved, I might have written this as:
unsupp = !!linkmode_andnot(tmp, edata->advertised, supported);
> + if (unsupp) {
> DP_VERBOSE(edev, QED_MSG_DEBUG,
> - "Invalid advertised capabilities %d\n",
> - edata->advertised_u32);
> + "Invalid advertised capabilities %*pb\n",
> + __ETHTOOL_LINK_MODE_MASK_NBITS, edata->advertised);
> return -EINVAL;
> }
>
> - if (edata->advertised_u32 & ADVERTISED_1000baseT_Full)
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> + edata->advertised))
> params.eee.adv_caps = QED_EEE_1G_ADV;
> - if (edata->advertised_u32 & ADVERTISED_10000baseT_Full)
> - params.eee.adv_caps |= QED_EEE_10G_ADV;
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> + edata->advertised))
> + params.eee.adv_caps = QED_EEE_10G_ADV;
> +
> params.eee.enable = edata->eee_enabled;
> params.eee.tx_lpi_enable = edata->tx_lpi_enabled;
> params.eee.tx_lpi_timer = edata->tx_lpi_timer;
>
> --
> 2.43.0
>
>
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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
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 <[email protected]>
> > ---
> > 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
> > + unsupp = linkmode_andnot(tmp, edata->advertised, supported);
>
> nit: Given the types involved, I might have written this as:
>
> unsupp = !!linkmode_andnot(tmp, edata->advertised, supported);
linkmode_andnot() calls bitmap_andnot():
static inline bool bitmap_andnot(unsigned long *dst, const unsigned long *src1,
const unsigned long *src2, unsigned int nbits)
It already returns a bool, so there is no need to force an int to bool
conversion using !!.
Andrew
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 <[email protected]>
> > ---
> > 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
On Tue, Feb 20, 2024 at 03:45:28PM +0100, Andrew Lunn wrote:
> > > + unsupp = linkmode_andnot(tmp, edata->advertised, supported);
> >
> > nit: Given the types involved, I might have written this as:
> >
> > unsupp = !!linkmode_andnot(tmp, edata->advertised, supported);
>
> linkmode_andnot() calls bitmap_andnot():
>
> static inline bool bitmap_andnot(unsigned long *dst, const unsigned long *src1,
> const unsigned long *src2, unsigned int nbits)
>
> It already returns a bool, so there is no need to force an int to bool
> conversion using !!.
Good point, sorry for missing that.
I assume there is a reason that the return type of
linkmode_andnot is not bool.
On Wed, Feb 21, 2024 at 10:28:51AM +0000, Simon Horman wrote:
> On Tue, Feb 20, 2024 at 03:45:28PM +0100, Andrew Lunn wrote:
> > > > + unsupp = linkmode_andnot(tmp, edata->advertised, supported);
> > >
> > > nit: Given the types involved, I might have written this as:
> > >
> > > unsupp = !!linkmode_andnot(tmp, edata->advertised, supported);
> >
> > linkmode_andnot() calls bitmap_andnot():
> >
> > static inline bool bitmap_andnot(unsigned long *dst, const unsigned long *src1,
> > const unsigned long *src2, unsigned int nbits)
> >
> > It already returns a bool, so there is no need to force an int to bool
> > conversion using !!.
>
> Good point, sorry for missing that.
> I assume there is a reason that the return type of
> linkmode_andnot is not bool.
Either i got it wrong when i added the wrapper, or bitmap_andnot() has
changed since then?
It probably can be changed to a bool.
Andrew