Re: [PATCH iwl-next v1] i40e: Add Energy Efficient Ethernet ability for X710 Base-T/KR/KX cards

[Simon Horman <horms@kernel.org>]

On Mon, Aug 12, 2024 at 10:09:37AM +0200, Przemek Kitszel wrote:
> On 8/9/24 17:25, Simon Horman wrote:
> > On Thu, Aug 08, 2024 at 01:22:17PM +0200, Aleksandr Loktionov wrote:
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > index 1d0d2e5..cd7509f 100644
> > > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > > @@ -5641,50 +5641,77 @@ static int i40e_get_module_eeprom(struct net_device *netdev,
> > >   	return 0;
> > >   }
> > > +static void i40e_eee_capability_to_kedata_supported(__le16  eee_capability_,
> > > +						    unsigned long *supported)
> > > +{
> > > +	const int eee_capability = le16_to_cpu(eee_capability_);
> > 
> > Hi Aleksandr,
> > 
> > Maybe u16 would be an appropriate type for eee_capability.
> > Also, using const seems excessive here.
> > 
> > > +	static const int lut[] = {
> > > +		ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > > +		ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> > > +		ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> > > +		ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
> > > +		ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
> > > +		ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
> > > +		ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
> > > +	};
> > > +
> > > +	linkmode_zero(supported);
> > > +	for (unsigned int i = ARRAY_SIZE(lut); i--; )
> > > +		if (eee_capability & (1 << (i + 1)))
> > 
> > Perhaps:
> > 
> > 		if (eee_capability & BIT(i + 1))
> 
> I would avoid any operations with @i other than using it as index:
> lut[i]. We have link mode bits in the table, why to compute that again?
> 
> > 
> > > +			linkmode_set_bit(lut[i], supported);
> > > +}
> > > +
> > >   static int i40e_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
> > >   {
> > >   	struct i40e_netdev_priv *np = netdev_priv(netdev);
> > >   	struct i40e_aq_get_phy_abilities_resp phy_cfg;
> > >   	struct i40e_vsi *vsi = np->vsi;
> > >   	struct i40e_pf *pf = vsi->back;
> > >   	struct i40e_hw *hw = &pf->hw;
> > > -	int status = 0;
> > > +	int status;
> > 
> > This change seems unrelated to the subject of this patch.
> > If so, please remove.
> 
> Hmm, it's remotely related, trivial, and makes code better;
> I intentionally said nothing about this during our internal review ;)

Ok, I would vote for it being a separate patch.
But I won't push this one any further.