> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Friday, August 9, 2024 5:26 PM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: [email protected]; Nguyen, Anthony L
> <[email protected]>; [email protected]; Kubalewski,
> Arkadiusz <[email protected]>
> Subject: Re: [PATCH iwl-next v1] i40e: Add Energy Efficient Ethernet
> ability for X710 Base-T/KR/KX cards
>
> 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.
>
The value is got from FW which dictates endianness.
Const is const, explicit coding style helps understanding and compiler
optimizations.
> > + 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))
>
Ok
> > + 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.
>
But it fixes kernel coding style which checkpatch.pl may complain about.
> >
> > /* Get initial PHY capabilities */
> > status = i40e_aq_get_phy_capabilities(hw, false, true, &phy_cfg,
> NULL);
> > if (status)
> > return -EAGAIN;
> >
> > /* Check whether NIC configuration is compatible with Energy
> Efficient
> > * Ethernet (EEE) mode.
> > */
> > if (phy_cfg.eee_capability == 0)
> > return -EOPNOTSUPP;
> >
> > + i40e_eee_capability_to_kedata_supported(phy_cfg.eee_capability,
> > +edata->supported);
>
> Please line-wrap: Networking still prefers code to be 80 columns wide
> or less.
>
As you wish.
> > + linkmode_copy(edata->lp_advertised, edata->supported);
> > +
> > /* Get current configuration */
> > status = i40e_aq_get_phy_capabilities(hw, false, false,
> &phy_cfg, NULL);
> > if (status)
> > return -EAGAIN;
> >
> > + linkmode_zero(edata->advertised);
> > + if (phy_cfg.eee_capability)
> > + linkmode_copy(edata->advertised, edata->supported);
> > edata->eee_enabled = !!phy_cfg.eee_capability;
> > edata->tx_lpi_enabled = pf->stats.tx_lpi_status;
> >
> > edata->eee_active = pf->stats.tx_lpi_status &&
> > pf->stats.rx_lpi_status;
> >
> > return 0;
> > }
> >
> > static int i40e_is_eee_param_supported(struct net_device *netdev,
> > struct ethtool_keee *edata) {
> > struct i40e_netdev_priv *np = netdev_priv(netdev);
> > struct i40e_vsi *vsi = np->vsi;
> > struct i40e_pf *pf = vsi->back;
> > struct i40e_ethtool_not_used {
> > - u32 value;
> > + int value;
>
> It is unclear to me that this type change is really related to the
> subject of this patch. If not, please drop it. But if so, it seems to
> me that bool would be the appropriate type.
>
> > const char *name;
> > } param[] = {
> > - {edata->tx_lpi_timer, "tx-timer"},
> > + {!!(edata->advertised[0] & ~edata->supported[0]),
> "advertise"},
> > + {!!edata->tx_lpi_timer, "tx-timer"},
> > {edata->tx_lpi_enabled != pf->stats.tx_lpi_status, "tx-
> lpi"}
> > };
> > int i;
> > @@ -5710,7 +5737,7 @@ static int i40e_set_eee(struct net_device
> *netdev, struct ethtool_keee *edata)
> > struct i40e_pf *pf = vsi->back;
> > struct i40e_hw *hw = &pf->hw;
> > __le16 eee_capability;
> > - int status = 0;
> > + int status;
>
> This change seems unrelated to the subject of this patch.
> If so, please remove.
>
But it fixes kernel coding style which checkpatch.pl may complain about.
> >
> > /* Deny parameters we don't support */
> > if (i40e_is_eee_param_supported(netdev, edata)) @@ -5754,7
> +5781,7
> > @@ static int i40e_set_eee(struct net_device *netdev, struct
> ethtool_keee *edata)
> > config.eeer |=
> cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);
> > } else {
> > config.eee_capability = 0;
> > - config.eeer &=
> cpu_to_le32(~I40E_PRTPM_EEER_TX_LPI_EN_MASK);
> > + config.eeer &=
> ~cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);
>
> Ditto.
>
> > }
> >
> > /* Apply modified PHY configuration */ diff --git
> > a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index cbcfada..55bbf0f 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -7263,6 +7263,22 @@ static int i40e_init_pf_dcb(struct i40e_pf
> *pf)
> > return err;
> > }
> > #endif /* CONFIG_I40E_DCB */
> > +static void i40e_print_link_message_eee(struct i40e_vsi *vsi,
> struct ethtool_keee *kedata,
> > + const char *speed, const char *fc) {
> > + if (vsi->netdev->ethtool_ops->get_eee)
> > + vsi->netdev->ethtool_ops->get_eee(vsi->netdev, kedata);
> > +
> > + if (!linkmode_empty(kedata->supported))
> > + netdev_info(vsi->netdev,
> > + "NIC Link is Up, %sbps Full Duplex, Flow
> Control: %s, EEE: %s\n",
> > + speed, fc,
> > + kedata->eee_enabled ? "Enabled" : "Disabled");
> > + else
> > + netdev_info(vsi->netdev,
> > + "NIC Link is Up, %sbps Full Duplex, Flow
> Control: %s\n",
> > + speed, fc);
> > +}
> >
> > /**
> > * i40e_print_link_message - print link up or down @@ -7395,9
> > +7411,12 @@ void i40e_print_link_message(struct i40e_vsi *vsi, bool
> isup)
> > "NIC Link is Up, %sbps Full Duplex, Requested
> FEC: %s, Negotiated FEC: %s, Autoneg: %s, Flow Control: %s\n",
> > speed, req_fec, fec, an, fc);
> > } else {
> > - netdev_info(vsi->netdev,
> > - "NIC Link is Up, %sbps Full Duplex, Flow
> Control: %s\n",
> > - speed, fc);
> > + struct ethtool_keee kedata;
> > +
> > + linkmode_zero(kedata.supported);
> > + kedata.eee_enabled = false;
>
> Can the declaration of ethtool_keee be moved into
> i40e_print_link_message_eee()? I suspect that would lead to a cleaner
> implementation.
>
Ok
> > +
> > + i40e_print_link_message_eee(vsi, &kedata, speed, fc);
> > }
> >
> > }
