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 > >
