From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id E6C613B5 for ; Tue, 21 Mar 2017 15:09:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490105388; x=1521641388; h=subject:to:references:cc:from:message-id:date: mime-version:in-reply-to:content-transfer-encoding; bh=DuiTP23AqV11fEFZFkj5qQaWurAen8NHHVaVWu4YIvE=; b=EmKi+xFaVGL2lgF4q/oH9FBZ91GoRANh0nAGHLWSse0HQzODT4AOXkLu e3cyjr+LV1rVntHyeQphCIqDi7mp8A==; Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Mar 2017 07:09:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,198,1486454400"; d="scan'208";a="79411896" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.122]) ([10.237.220.122]) by fmsmga006.fm.intel.com with ESMTP; 21 Mar 2017 07:09:45 -0700 To: Shijith Thotton 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> Cc: dev@dpdk.org, Jerin Jacob , Derek Chickles , Venkat Koppula , Srisivasubramanian S , Mallesham Jatharakonda From: Ferruh Yigit Message-ID: Date: Tue, 21 Mar 2017 14:09:44 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170321135602.GA13505@localhost.localdomain> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 36/46] net/liquidio: add API to set MTU X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Mar 2017 14:09:48 -0000 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. But you may want to keep the patch for MTU validation, and change function name according. > >> >> 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. > >> > <...> >