From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 76F03282; Thu, 2 Feb 2017 11:15:11 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 02 Feb 2017 02:15:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,324,1477983600"; d="scan'208";a="1090021531" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38]) ([10.237.220.38]) by orsmga001.jf.intel.com with ESMTP; 02 Feb 2017 02:15:09 -0800 To: Adrien Mazarguil , Shahaf Shuler References: <1485348178-43771-1-git-send-email-shahafs@mellanox.com> <1485863129-6326-1-git-send-email-shahafs@mellanox.com> <20170201090711.GP10133@6wind.com> <64089318-6d12-28b4-61e8-6e52785ce692@intel.com> <20170201125758.GS10133@6wind.com> <963be495-c009-2937-5a14-204ae3a3b245@intel.com> <20170202084534.GT10133@6wind.com> Cc: =?UTF-8?Q?N=c3=a9lio_Laranjeiro?= , "dev@dpdk.org" , "stable@dpdk.org" From: Ferruh Yigit Message-ID: <1c2be312-c11e-62ab-4a36-d91f25520ab2@intel.com> Date: Thu, 2 Feb 2017 10:15:08 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170202084534.GT10133@6wind.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/mlx5: fix link status query 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, 02 Feb 2017 10:15:13 -0000 On 2/2/2017 8:45 AM, Adrien Mazarguil wrote: > On Wed, Feb 01, 2017 at 06:11:17PM +0000, Ferruh Yigit wrote: >> On 2/1/2017 12:57 PM, Adrien Mazarguil wrote: >>> On Wed, Feb 01, 2017 at 11:13:59AM +0000, Ferruh Yigit wrote: >>>> On 2/1/2017 9:07 AM, Adrien Mazarguil wrote: >>>>> On Wed, Feb 01, 2017 at 06:53:55AM +0000, Shahaf Shuler wrote: >>>>>> : Tuesday, January 31, 2017 6:17 PM, Ferruh Yigit: >>>>>>> On 1/31/2017 11:45 AM, Shahaf Shuler wrote: >>>>>>>> Trying to query the link status through new kernel ioctl API >>>>>>>> ETHTOOL_GLINKSETTINGS was always failing due to kernel bug. >>>>>>>> The bug was fixed on version 4.9 >>>>>>>> this patch uses the legacy ioctl API for lower kernels. >>>>>>>> >>>>>>>> Fixes: 188408719888 ("net/mlx5: fix support for newer link speeds") >>>>>>>> CC: stable@dpdk.org >>>>>>>> >>>>>>>> Signed-off-by: Shahaf Shuler >>>>>>>> --- >>>>>>> >>>>>>> <...> >>>>>>> >>>>>>>> @@ -707,7 +708,7 @@ struct priv * >>>>>>>> static int >>>>>>>> mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int >>>>>>>> wait_to_complete) { -#ifdef ETHTOOL_GLINKSETTINGS >>>>>>>> +#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE >>>>>>> >>>>>>> Mostly it is not good idea to do kernel version check in the .c file. >>>>>>> >>>>>>> It is possible to move this comparison to the .h file, and set a feature >>>>>>> macro based on comparison result, like HAVE_ETHTOOL_GLINKSETTINGS, >>>>>>> and >>>>>>> use this macro in the .c file. >>>>>>> >>>>>>> This makes .c code easier to understand. And the abstraction in the >>>>>>> header file lets you update the comparison in the future without >>>>>>> changing the code itself. >>>>>>> >>>>>>> But it is your call, do you prefer to continue with this one? >>>>>> >>>>>> This is a good suggestion. >>>>>> Adrien, Nélio what do you think? >>>>> >>>>> Let's include this patch as-is. Doing so in a header file such as mlx5.h >>>>> would require including linux/version.h from that file and cause the entire >>>>> PMD to be even more OS-dependent. >>>>> >>>>> We'll move this check elsewhere in the future if we need several such >>>>> workarounds, thanks. >>>> >>>> OK. >>>> >>>> One more thing, comment log says: >>>> "The bug was fixed on version 4.9" >>>> >>>> And code does: >>>> "+#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE" >>>> >>>> If the bug is fixed in 4.9, should check be "<" instead of "<=" >>> >>> I'll concede the argument order used in this condition is somewhat unusual >>> but it actually ends up being the same as: >>> >>> #if LINUX_VERSION_CODE > KERNEL_VERSION(4, 9, 0) >> >> I don't think they are same, unless I am missing something obvious. >> >> "+#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE" >> vs >> "#if LINUX_VERSION_CODE > KERNEL_VERSION(4, 9, 0)" >> >> Even if you change the argument order, one covers 4.9 release other not. > > All right, turns out we're both wrong because it should have been >= anyway > regardless of the order as you pointed out. The block must appear when > kernel version is higher or equal to 4.9.0. I see now, my logic was wrong, and I can see what are you trying to explain by changing argument order, thanks ... > > Now I think there are a couple of additional issues with this patch that > require a revert and rework: > > - Kernel headers do not necessarily match the running version. What if this > code is compiled against a 4.10.0 source tree and the running kernel is > 4.5? How about the reverse? It is possible to use proper kernel headers via RTE_KERNELDIR environment variable. But without this patch, /usr/include/linux/ethtool.h used directly in Makefile, which has same problem you mentioned. > > - By removing the check on ETHTOOL_GLINKSETTINGS, this code breaks > compilation for Linux kernel version < 4.5, which I think is a problem. Please help me understand. ETHTOOL_GLINKSETTINGS defined for kernels > 4.5, and this patch replaces "#ifdef ETHTOOL_GLINKSETTINGS" with "#if KERNEL_VERSION(4, 9, 0) <= LINUX_VERSION_CODE" So, kernel version <= 4.5 part should be same, why it is giving a compile error? > > So I believe this patch should be re-worked to maintain a check on > ETHTOOL_GLINKSETTINGS (or define it then not present) and perform an > additional version check at runtime to use the most appropriate ioctl API. Overall, it would be better if you can find a way without dealing kernel versions, which causes lots of maintenance work. Also another issue we are experiencing while maintaining KNI is backported kernels, they tend to break version based checks. So there are distro based checks combined with version checks, I think this wouldn't be something you want J > > Ferruh, please revert. But original patch looks good, I will convert it to initial patch, and keep it until above two items clarified. > > stable@dpdk.org, please ignore this commit for the the time being, and sorry > for the noise. > >>> Which is the correct intent. I guess you can update this line for clarity if >>> you think it's necessary. >> >> If the intention is as following, I can fix it while applying: >> #if KERNEL_VERSION(4, 9, 0) < LINUX_VERSION_CODE >> >>> >>>>> >>>>>>>> struct priv *priv = mlx5_get_priv(dev); >>>>>>>> struct ethtool_link_settings edata = { >>>>>>>> .cmd = ETHTOOL_GLINKSETTINGS, >>>>>>> <...> >>>>> >>>> >>> >> >