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 A91031150; Wed, 1 Feb 2017 19:11:20 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP; 01 Feb 2017 10:11:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,321,1477983600"; d="scan'208";a="1120780793" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38]) ([10.237.220.38]) by fmsmga002.fm.intel.com with ESMTP; 01 Feb 2017 10:11:18 -0800 To: Adrien Mazarguil 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> Cc: Shahaf Shuler , =?UTF-8?Q?N=c3=a9lio_Laranjeiro?= , "dev@dpdk.org" , "stable@dpdk.org" From: Ferruh Yigit Message-ID: <963be495-c009-2937-5a14-204ae3a3b245@intel.com> Date: Wed, 1 Feb 2017 18:11:17 +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: <20170201125758.GS10133@6wind.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Feb 2017 18:11:21 -0000 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. > > 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, >>>>> <...> >>> >> >