| Message ID | 20240218-keee-u32-cleanup-v4-3-71f13b7c3e60@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | drivers: net: Convert EEE handling to use linkmode bitmaps | expand |
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 <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
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;
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(-)