From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH] net/thunderx: sync mailbox definitions with Linux PF driver Date: Thu, 23 Mar 2017 14:02:14 +0000 Message-ID: <5655489e-06f9-e5ce-2372-cf45cb80f585@intel.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, Christian Ehrhardt To: Jerin Jacob 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) In-Reply-To: <20170323140026.inooqnbb6dq3jkjy@localhost.localdomain> 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 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; >>>>>>>>> >>>>>>>> >>>>>> >>>> >>