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 <andrew@lunn.ch>
---
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.0On 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 <andrew@lunn.ch>
Thanks Andrew,
the nit below notwithstanding this looks good to me.
Reviewed-by: Simon Horman <horms@kernel.org>
> ---
> 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
>
>
> > + 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 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
© 2016 - 2026 Red Hat, Inc.