From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id DB616695C for ; Thu, 23 Mar 2017 15:02:16 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490277737; x=1521813737; h=subject:to:references:cc:from:message-id:date: mime-version:in-reply-to:content-transfer-encoding; bh=SUp/q2A5/Ft7QY7EhyEi4bvU3rJGcLS6EKJ/wpEVdWM=; b=KnPh9pPZsqUDKNMKb6byYuNG7c58X/tN8yKR9x43hXJlrSmw3fllFeQC ULAhCLG3LePPFlrnaRWyrOhwXr578g==; Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Mar 2017 07:02:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,210,1486454400"; d="scan'208";a="80241858" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.122]) ([10.237.220.122]) by fmsmga006.fm.intel.com with ESMTP; 23 Mar 2017 07:02:14 -0700 To: Jerin Jacob References: <1490019040-6268-1-git-send-email-jerin.jacob@caviumnetworks.com> <20170321143832.swnf7xetxlxc3k5w@localhost.localdomain> <8059571c-9747-ca33-0d61-f453e43afa48@intel.com> <20170322093931.2u5z3n645cnobi63@localhost.localdomain> <526d95ae-fe16-6c54-441c-32368df01c7d@intel.com> <20170323132013.lg4f7k26o5klsgyn@localhost.localdomain> <8f471344-24cd-2727-626b-84cc49c813cf@intel.com> <20170323140026.inooqnbb6dq3jkjy@localhost.localdomain> Cc: dev@dpdk.org, Christian Ehrhardt From: Ferruh Yigit Message-ID: <5655489e-06f9-e5ce-2372-cf45cb80f585@intel.com> Date: Thu, 23 Mar 2017 14:02:14 +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: <20170323140026.inooqnbb6dq3jkjy@localhost.localdomain> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] net/thunderx: sync mailbox definitions with Linux PF driver 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: Thu, 23 Mar 2017 14:02:17 -0000 On 3/23/2017 2:00 PM, Jerin Jacob wrote: > On Thu, Mar 23, 2017 at 01:26:15PM +0000, Ferruh Yigit wrote: >> On 3/23/2017 1:20 PM, Jerin Jacob wrote: >>> On Wed, Mar 22, 2017 at 10:33:14AM +0000, Ferruh Yigit wrote: >>>> On 3/22/2017 9:39 AM, Jerin Jacob wrote: >>>>> On Tue, Mar 21, 2017 at 02:53:29PM +0000, Ferruh Yigit wrote: >>>>>> On 3/21/2017 2:38 PM, Jerin Jacob wrote: >>>>>>> On Tue, Mar 21, 2017 at 02:31:41PM +0000, Ferruh Yigit wrote: >>>>>>>> On 3/20/2017 2:10 PM, Jerin Jacob wrote: >>>>>>>>> - bgx_link_status mbox definition was changed in Linux >>>>>>>>> commit 1cc702591bae ("net: thunderx: Add ethtool support") >>>>>>>>> - NIC_MBOX_MSG_RES_BIT related changes were never part of Linux PF driver >>>>>>>>> >>>>>>>>> Signed-off-by: Jerin Jacob >>>>>>>> >>>>>>>> <...> >>>>>>>> >>>>>>>>> @@ -157,6 +151,7 @@ struct rss_cfg_msg { >>>>>>>>> /* Physical interface link status */ >>>>>>>>> struct bgx_link_status { >>>>>>>>> uint8_t msg; >>>>>>>>> + uint8_t mac_type; >>>>>>>> >>>>>>>> Hi Jerin, >>>>>>>> >>>>>>>> Is this modification related to this patch? >>>>>>> >>>>>>> Yes Ferruh. >>>>>>> >>>>>>> This was related to the following section in git log comment. >>>>>>> ---- >>>>>>> - bgx_link_status mbox definition was changed in Linux >>>>>>> commit 1cc702591bae ("net: thunderx: Add ethtool support") >>>>>>> --- >>>>>> >>>>>> I see now, thanks. Since this is to sync with Linux PF, shouldn't it be >>>>>> used in driver, perhaps something like in Linux driver: >>>>>> "nic->mac_type = mbx.link_status.mac_type" >>>>>> >>>>>> What is the point of just adding definition without using it? >>>>> >>>>> That is to keep "link_up"(next element in the struct bgx_link_status) points >>>>> to correct location after the kernel change. >>>> >>>> I see, thanks. It is pity that new field added into middle (or beginning >>>> if you exclude msg) of the struct. >>>> >>>>> I thought about, the backward >>>>> compatibility with older kernel, Is it OK to use linux/version.h in PMD drivers >>>>> to detect the kernel version? >>>> >>>> Technically possible, but kernel version check has its problems: >>>> 1) Adds maintenance cost, which gets worse by time. >>>> 2) If added a compile time check, it creates binary distribution >>>> problems, distro guys may not like PMD has dependency to kernel. >>>> 3) It will create dependency to Linux, assuming Thunderx PMD supported >>>> in FreeBSD >>>> >>>>> drivers/net/mlx5/mlx5_ethdev.c has similar >>>>> kernel detection mechanism to make it backward compatible. >>>> >>>> Yes, at least mlx added a dynamic check which prevents above issue 2. >>>> >>>>> If there are no issue with such approach, I will roll out a new revision. >>>> >>>> Is there any way to detect this dynamically, first things I can think of: >>>> - Any mbx versioning to rely on? >>>> - Any common NIC user header file provided by kernel, to use in DPDK, to >>>> prevent any this kind of issues in the future? >>>> - Any msg size value to detect if mac_type exists or not? >>>> - Heuristic approach to other values to detect mac_type existence? >>> >>> Thanks for sharing the options. >>> First three options wont work. Heuristic option is pretty limited, >>> mac_type value can also be 0 or 1(same as link_up) >>> So I think, The best option now to use the patch as is(without kernel >>> check). >>> What do you think? >> >> If this won't cause a problem for you with older kernels, I would prefer >> no kernel check option. > > OK. I will ask our kernel team to backport the offending the kernel change > to old versions based on the requirement. Please merge this patch if you don't > see any other issue. > I can post a separate patch add to the kernel version check if it > is really required. OK > > >> >>> >>>> >>>>> >>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>>> uint8_t link_up; >>>>>>>>> uint8_t duplex; >>>>>>>>> uint32_t speed; >>>>>>>>> >>>>>>>> >>>>>> >>>> >>