Received-SPF: pass (google.com: domain of linux-kernel+bounces-73036-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 20 Feb 2024 12:44:05 +0000
From: Simon Horman <horms@kernel.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Ariel Elior <aelior@marvell.com>,
	Manish Chopra <manishc@marvell.com>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	intel-wired-lan@lists.osuosl.org
Subject: Re: [PATCH net-next v4 3/9] net: qlogic: qede: Use linkmode helpers
 for EEE
Message-ID: <20240220124405.GB40273@kernel.org>
References: <20240218-keee-u32-cleanup-v4-0-71f13b7c3e60@lunn.ch>
 <20240218-keee-u32-cleanup-v4-3-71f13b7c3e60@lunn.ch>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240218-keee-u32-cleanup-v4-3-71f13b7c3e60@lunn.ch>

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(&params, 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
> 
>