RE: [PATCH] net/i40e: fix max frmame size config at port level

["Zhang, Yuying" <yuying.zhang@intel.com>]

Hi Wenxuan,

> -----Original Message-----
> Date: Thu, 28 Apr 2022 11:12:24 +0000
> From: wenxuanx.wu@intel.com
> To: beilei.xing@intel.com,	dev@dpdk.org
> Cc: wenxuanx.wu@intel.com,	yidingx.zhou@intel.com,
> 	stable@dpdk.org
> Subject: [PATCH] net/i40e: fix max frmame size config at port level

Please add version of patch and correct the spelling error in the title.

> Message-ID: <20220428111224.57705-1-wenxuanx.wu@intel.com>
> 
> From: Wenxuan Wu <wenxuanx.wu@intel.com>
> 
> Previously, max frame size can only be set when link is up, and the wait
> time is 1 sec. But for nic media type of I40E_10G_BASET would consume
> longer
> time which is too short to up would result in error.

Please split this sentence into two since two subjects are different and omitted by you.
BTW, it should be less than 75 characters per line. Fix the code style warning please.

> 
> Acctually, max frame size of media type I40E_MEDIA_TYPE_BASET can be set
> regardless of link status.
> 
> This patch omit the status check of 10G_MEDIA_TYPE_BASET.
> 
> Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 755786dc10..5762cd526a 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -12102,23 +12102,21 @@ i40e_set_mac_max_frame(struct rte_eth_dev
> *dev, uint16_t size)
>  	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
>  	uint32_t rep_cnt = MAX_REPEAT_TIME;
>  	struct rte_eth_link link;
> -	enum i40e_status_code status;
> +	enum i40e_status_code status = I40E_ERR_DEVICE_NOT_SUPPORTED;
> 
>  	do {
>  		update_link_reg(hw, &link);
>  		if (link.link_status)
>  			break;
> -
>  		rte_delay_ms(CHECK_INTERVAL);
>  	} while (--rep_cnt);

It dones't need to wait for 1s without link status check.

> 
> -	if (link.link_status) {
> +	/* Only I40E_MEDIA_TYPE_FIBER link up should be guaranteed */

Your code mismatches with your comment.

> +	if (hw->phy.media_type == I40E_MEDIA_TYPE_BASET ||
> link.link_status)
>  		status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false,
> NULL);
> -		if (status != I40E_SUCCESS)
> -			PMD_DRV_LOG(ERR, "Failed to set max frame size at
> port level");
> -	} else {
> -		PMD_DRV_LOG(ERR, "Set max frame size at port level not
> applicable on link down");
> -	}
> +
> +	if (status != I40E_SUCCESS)
> +		PMD_DRV_LOG(ERR, "Failed to set max frame size at port
> level");
>  }
> 
>  RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
> --
> 2.25.1

Best regards,
Yuying