From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shijith Thotton Subject: Re: [PATCH v2 36/46] net/liquidio: add API to set MTU Date: Thu, 23 Mar 2017 10:32:01 +0530 Message-ID: <20170323050159.GA2350@hone> References: <1487669225-30091-1-git-send-email-shijith.thotton@caviumnetworks.com> <1488454371-3342-1-git-send-email-shijith.thotton@caviumnetworks.com> <1488454371-3342-37-git-send-email-shijith.thotton@caviumnetworks.com> <9a8d31ce-8590-25f3-eab8-6a34e4a645a2@intel.com> <20170321125321.GA13113@localhost.localdomain> <5f7890d7-4714-9ceb-50b2-b548903fee9d@intel.com> <20170321135602.GA13505@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, Jerin Jacob , Derek Chickles , Venkat Koppula , Srisivasubramanian S , Mallesham Jatharakonda To: Ferruh Yigit Return-path: Received: from NAM03-BY2-obe.outbound.protection.outlook.com (mail-by2nam03on0062.outbound.protection.outlook.com [104.47.42.62]) by dpdk.org (Postfix) with ESMTP id 4B0F71094 for ; Thu, 23 Mar 2017 06:02:27 +0100 (CET) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Mar 21, 2017 at 02:09:44PM +0000, Ferruh Yigit wrote: > On 3/21/2017 1:56 PM, Shijith Thotton wrote: > > On Tue, Mar 21, 2017 at 01:01:58PM +0000, Ferruh Yigit wrote: > >> On 3/21/2017 12:53 PM, Shijith Thotton wrote: > >>> On Tue, Mar 21, 2017 at 12:24:49PM +0000, Ferruh Yigit wrote: > >>>> On 3/2/2017 11:32 AM, Shijith Thotton wrote: > >>>>> Signed-off-by: Shijith Thotton > >>>>> Signed-off-by: Jerin Jacob > >>>>> Signed-off-by: Derek Chickles > >>>>> Signed-off-by: Venkat Koppula > >>>>> Signed-off-by: Srisivasubramanian S > >>>>> Signed-off-by: Mallesham Jatharakonda > >>>> > >>>> <...> > >>>> > >>>>> > >>>>> static int > >>>>> +lio_dev_change_vf_mtu(struct rte_eth_dev *eth_dev, uint16_t new_mtu) > >>>>> +{ > >>>>> + struct lio_device *lio_dev = LIO_DEV(eth_dev); > >>>>> + > >>>>> + PMD_INIT_FUNC_TRACE(); > >>>>> + > >>>>> + if (!lio_dev->intf_open) { > >>>>> + lio_dev_err(lio_dev, "Port %d down, can't change MTU\n", > >>>>> + lio_dev->port_id); > >>>>> + return -EINVAL; > >>>>> + } > >>>>> + > >>>>> + /* Limit the MTU to make sure the ethernet packets are between > >>>>> + * ETHER_MIN_MTU bytes and PF's MTU > >>>>> + */ > >>>>> + if ((new_mtu < ETHER_MIN_MTU) || > >>>>> + (new_mtu > lio_dev->linfo.link.s.mtu)) { > >>>>> + lio_dev_err(lio_dev, "Invalid MTU: %d\n", new_mtu); > >>>>> + lio_dev_err(lio_dev, "Valid range %d and %d\n", > >>>>> + ETHER_MIN_MTU, lio_dev->linfo.link.s.mtu); > >>>>> + return -EINVAL; > >>>>> + } > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>> > >>>> Is this really sets the MTU? > >>>> "new_mtu" seems not used, except limit check, an lio_send_ctrl_pkt() > >>>> required perhaps? > >>> > >>> It won't set MTU for hardware and is possible only by PF. So > >>> lio_send_ctrl_pkt is not required. VF MTU is limited by PF MTU and is > >>> mentioned under limitations in driver documentation. Here we are > >>> allowing upper layer to set MTU up to the value configured by PF. > >> > >> I see, but lio_dev_change_vf_mtu() does not set anything at all. If it > >> is not modifying anything at all, why you claim "MTU update" supported? > > > > We allow update for the upper layer till the value supported by PF even > > though there are no real MTU update happening at hardware level. > > Thought it is ok to have it mentioned under limitation. > > Two options are: > > 1. mark support as partial. > > 2. remove this patch and support. > > > > Please suggest which one is better. > > If I get it right, it is not possible to set VF MTU, so I would suggest > removing MTU update support. OK. > > But you may want to keep the patch for MTU validation, and change > function name according. Will change set to validate. > > > > >> > >> And following logic seems wrong for this case: > >> > >> ... > >> if (lio_dev->linfo.link.s.mtu != mtu) { > >> ret = lio_dev_change_vf_mtu(eth_dev, mtu); > >> ... > >> > >> Should this functions set lio_dev->linfo.link.s.mtu at least, perhaps? > > > > lio_dev->linfo.link.s.mtu represents the MTU supported by the device and > > it gets updated if there is a change by PF. > > So, "lio_dev->linfo.link.s.mtu" is device MTU set by PF, "mtu" is VF > eth_dev configured value. > If they are not same, lio_dev_change_vf_mtu() does check if "mtu" is in > valid range and return success or failure, right? > So, this is just configuration validation, nothing changed/set here. > > > > >> > > <...> > > >