[dpdk-dev] [PATCH v3] i40e: configure MTU

[Yong Wang]

On 5/16/16, 5:27 AM, "dev on behalf of Olivier Matz" <dev-bounces at dpdk.org on behalf of olivier.matz at 6wind.com> wrote:

>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 at 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

AFAICT, the same check is not done for other drivers that implement
the mac_set op. Wouldn’t it make more sense to have the driver disable
the port, reconfigure and re-enable it in this case, instead of returning
error code?  If the consensus in DPDK is to have the application disable
the port first, we need to enforce this policy across all devices and
clearly document this behavior.

>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