[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240812160613.GB44433@kernel.org>
Date: Mon, 12 Aug 2024 17:06:13 +0100
From: Simon Horman <horms@...nel.org>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: Aleksandr Loktionov <aleksandr.loktionov@...el.com>,
intel-wired-lan@...ts.osuosl.org, anthony.l.nguyen@...el.com,
netdev@...r.kernel.org,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
Subject: Re: [PATCH iwl-next v1] i40e: Add Energy Efficient Ethernet ability
for X710 Base-T/KR/KX cards
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.
Powered by blists - more mailing lists