From mboxrd@z Thu Jan  1 00:00:00 1970
From: Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [PATCH v3] i40e: configure MTU
Date: Mon, 16 May 2016 14:27:02 +0200
Message-ID: <5739BC96.5010509@6wind.com>
References: <1461813555-24095-1-git-send-email-beilei.xing@intel.com>
 <1463127333-10007-1-git-send-email-beilei.xing@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Cc: dev@dpdk.org, Julien Meunier <julien.meunier@6wind.com>,
 Thomas Monjalon <thomas.monjalon@6wind.com>
To: Beilei Xing <beilei.xing@intel.com>, jingjing.wu@intel.com
Return-path: <dev-bounces@dpdk.org>
Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67])
 by dpdk.org (Postfix) with ESMTP id 61D858E6A
 for <dev@dpdk.org>; Mon, 16 May 2016 14:27:11 +0200 (CEST)
In-Reply-To: <1463127333-10007-1-git-send-email-beilei.xing@intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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