> -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Yong Wang > Sent: Thursday, June 16, 2016 6:52 PM > To: Olivier Matz; Xing, Beilei; Wu, Jingjing > Cc: dev at dpdk.org; Julien Meunier; Thomas Monjalon > Subject: Re: [dpdk-dev] [PATCH v3] i40e: configure MTU > > On 6/16/16, 10:40 AM, "dev on behalf of Yong Wang" <dev-bounces at dpdk.org > on behalf of yongwang at vmware.com> wrote: > > >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 > > Correction: this should read as mtu_set. > > >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.
Other PMDs do allow to change mtu without stopping/reconfiguration the port (under certain conditions of course). As I remember that's why that function was introduced at the first place. It is a bit strange that i40e doesn't allow that, might be the author can comment why it is necessary. Again the whole i40e_dev_mtu_set() looks a bit strange to me - I can't see where the actual change of HW state is happening. Konstantin > > > >>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 > >
