Re: [PATCH v3] i40e: configure MTU

[Olivier Matz <olivier.matz@6wind.com>]

Hi Beilei,

On 05/13/2016 10:15 AM, Beilei Xing wrote:
> This patch enables configuring MTU for i40e.
> Since changing MTU needs to reconfigure queue, stop port first
> before configuring MTU.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v3 changes:
>  Add frame size with extra I40E_VLAN_TAG_SIZE.
>  Delete i40e_dev_rx_init(pf) cause it will be called when port starts.
> 
> v2 changes:
>  If mtu is not within the allowed range, return -EINVAL instead of -EBUSY.
>  Delete rxq reconfigure cause rxq reconfigure will be finished in
>  i40e_dev_rx_init.
>    
>  drivers/net/i40e/i40e_ethdev.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> [...]
> +static int
> +i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	struct rte_eth_dev_data *dev_data = pf->dev_data;
> +	uint32_t frame_size = mtu + ETHER_HDR_LEN
> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> +	int ret = 0;
> +
> +	/* check if mtu is within the allowed range */
> +	if ((mtu < ETHER_MIN_MTU) || (frame_size > I40E_FRAME_SIZE_MAX))
> +		return -EINVAL;
> +
> +	/* mtu setting is forbidden if port is start */
> +	if (dev_data->dev_started) {
> +		PMD_DRV_LOG(ERR,
> +			    "port %d must be stopped before configuration\n",
> +			    dev_data->port_id);
> +		return -ENOTSUP;
> +	}

I'm not convinced that ENOTSUP is the proper return value here.
It is usually returned when a function is not implemented, which
is not the case here: the function is implemented but is forbidden
because the port is running.

I saw that Julien commented on your v1 that the return value should
be one of:
 - (0) if successful.
 - (-ENOTSUP) if operation is not supported.
 - (-ENODEV) if *port_id* invalid.
 - (-EINVAL) if *mtu* invalid.

But I think your initial value (-EBUSY) was fine. Maybe it should be
added in the API instead, with the following description:
  (-EBUSY) if the operation is not allowed when the port is running

This would allow the application to take its dispositions to stop the
port and restart it with the proper jumbo_frame argument.

+CC Thomas which maintains ethdev API.


Regards,
Olivier