From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 61D858E6A for ; Mon, 16 May 2016 14:27:11 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1b2He8-0005Dx-HM; Mon, 16 May 2016 14:29:17 +0200 To: Beilei Xing , jingjing.wu@intel.com References: <1461813555-24095-1-git-send-email-beilei.xing@intel.com> <1463127333-10007-1-git-send-email-beilei.xing@intel.com> Cc: dev@dpdk.org, Julien Meunier , Thomas Monjalon From: Olivier Matz Message-ID: <5739BC96.5010509@6wind.com> Date: Mon, 16 May 2016 14:27:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <1463127333-10007-1-git-send-email-beilei.xing@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 May 2016 12:27:11 -0000 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 > --- > 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