[Intel-wired-lan] [PATCH net-next v1] i40e: Fix set max_tx_rate when it is lower than 1 Mbps

[Tony Nguyen]

On 8/16/2022 2:43 AM, Andrii Staikov wrote:
> Value max_tx_rate is converted from bytes to Mbps.
> When max_tx_rate was set to lower than 125000 bytes (1 Mbps)
> it was cut to 0 because of this conversion.

It would be nice to include what the visible behavior of this is.

> Add a check that if max_tx_rate is lower than 1 Mbps
> set this to minimum usable value of 50 Mbps.
> Add defined constants.
> 
> bats=yes
> 
> Title: i40e: Fix set max_tx_rate when it is lower than 1 Mbps
> Change-type: DefectResolution
> HSDES-Number: 16013332829
> HSDES-Tenant: server_platf_lan.bug

These should not included.

Since you use 'Fix', I presume this should go to net (with Fixes:), 
otherwise, it would be preferable to avoid using 'Fix' for patches going 
to net-next.

> Signed-off-by: Michal Jaron <michalx.jaron at intel.com>
> Signed-off-by: Andrii Staikov <andrii.staikov at intel.com>

I believe Michal is the author? In which case, you should modify the 
patch author to reflect that.

> ---
>   drivers/net/ethernet/intel/i40e/i40e_main.c | 32 +++++++++++++++++----
>   1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 10c1e1ea83a1..e3d9804aeb25 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -5908,6 +5908,26 @@ static int i40e_get_link_speed(struct i40e_vsi *vsi)
>   	}
>   }
>   
> +/**
> + * i40e_bw_bytes_to_mbits - Convert max_tx_rate from bytes to mbits
> + * @vsi: Pointer to vsi structure
> + * @max_tx_rate: max TX rate in bytes to be converted into Mbits
> + *
> + * Helper function to convert units before send to set BW limit
> + **/
> +static u64 i40e_bw_bytes_to_mbits(struct i40e_vsi *vsi, u64 max_tx_rate)
> +{
> +	if (max_tx_rate < I40E_BW_MBPS_DIVISOR) {
> +		dev_warn(&vsi->back->pdev->dev,
> +			 "Setting max tx rate to minimum usable value of 50Mbps.\n");
> +		max_tx_rate = I40E_BW_CREDIT_DIVISOR;

Should the print use the define vs magic number?

> +	} else {
> +		do_div(max_tx_rate, I40E_BW_MBPS_DIVISOR);
> +	}
> +
> +	return max_tx_rate;
> +}
> +
>   /**
>    * i40e_set_bw_limit - setup BW limit for Tx traffic based on max_tx_rate
>    * @vsi: VSI to be configured
> @@ -5930,10 +5950,10 @@ int i40e_set_bw_limit(struct i40e_vsi *vsi, u16 seid, u64 max_tx_rate)
>   			max_tx_rate, seid);
>   		return -EINVAL;
>   	}
> -	if (max_tx_rate && max_tx_rate < 50) {
> +	if (max_tx_rate && max_tx_rate < I40E_BW_CREDIT_DIVISOR) {
>   		dev_warn(&pf->pdev->dev,
>   			 "Setting max tx rate to minimum usable value of 50Mbps.\n");
> -		max_tx_rate = 50;
> +		max_tx_rate = I40E_BW_CREDIT_DIVISOR;

Same comment here.

>   	}
>   
>   	/* Tx rate credits are in values of 50Mbps, 0 is disabled */
> @@ -8224,9 +8244,9 @@ static int i40e_setup_tc(struct net_device *netdev, void *type_data)
>   
>   	if (i40e_is_tc_mqprio_enabled(pf)) {
>   		if (vsi->mqprio_qopt.max_rate[0]) {
> -			u64 max_tx_rate = vsi->mqprio_qopt.max_rate[0];
> +			u64 max_tx_rate = i40e_bw_bytes_to_mbits(vsi,
> +						  vsi->mqprio_qopt.max_rate[0]);
>   
> -			do_div(max_tx_rate, I40E_BW_MBPS_DIVISOR);
>   			ret = i40e_set_bw_limit(vsi, vsi->seid, max_tx_rate);
>   			if (!ret) {
>   				u64 credits = max_tx_rate;
> @@ -10971,10 +10991,10 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
>   	}
>   
>   	if (vsi->mqprio_qopt.max_rate[0]) {
> -		u64 max_tx_rate = vsi->mqprio_qopt.max_rate[0];
> +		u64 max_tx_rate = i40e_bw_bytes_to_mbits(vsi,
> +						  vsi->mqprio_qopt.max_rate[0]);
>   		u64 credits = 0;
>   
> -		do_div(max_tx_rate, I40E_BW_MBPS_DIVISOR);
>   		ret = i40e_set_bw_limit(vsi, vsi->seid, max_tx_rate);
>   		if (ret)
>   			goto end_unlock;