From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 473F01150 for ; Thu, 2 Feb 2017 09:45:43 +0100 (CET) Received: by mail-wm0-f43.google.com with SMTP id v77so77135621wmv.0 for ; Thu, 02 Feb 2017 00:45:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=oTz6vs9mixSLHiTCPbl8mpsXebAnE922mNyHs070gJw=; b=W1b4WYvvdps/tLSt6UxEze1yCvfo2awFh+P8odlCoYP6C9edK7KjSqG3T9z87zflz/ hH49S8I6ZykxPEpkNY/h3O9gH26AT8p/yp2ea5yuWK0zAjl97FVV+nE5VXKE07BRBANs TS6DeyjBCkwsOgYv8jukdT+0yttDFdB4G9hWbkjBGn1CnI7cCRsCo1IGrR8rLSyTYFQD q4btbZwtpTqsCsp9XIosnlm8Blht0Au0WBxdZTrKG2G8YRDXZ24m8DR5V4XTOmw/M+4f rydcUYzQmRToFmWULx6UPcXHVhdG3VbC9rja7IFePuNlEjSqjWNXMULNiq/osmGNSkDA MOPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=oTz6vs9mixSLHiTCPbl8mpsXebAnE922mNyHs070gJw=; b=RMLcbcGMMA0/TkOZcgW6Yt+7mB3orNL0fvOuR1HuNUT4RFIs3zuTNpJ+rRzB/04sPK K0TDp3oTmU6CO05Y7ORbl3HR8Pc79+QRnmw0xYNBdb/lKrMyN0oGef/Po8ap/+rFsgPH aEkhka2D+Cj2A5P25tWgQEm6+PecmmiCE4rxNZ54dLycM4aMdixLo7X35J1N2QGIxxpP ehZnVbIwX90GxXyqTP/YBSyBfznFgvqGxNiGShwk2t3mqNzo+OT4u+Rm7+edc/ZBFZIz qlTQFXRR5RH8FjOufT++nw1piMEWg1ZKba7OMfjHaZUrsOJ7lNbpFdqfUbnoduVLttPV kbcg== X-Gm-Message-State: AIkVDXKKk3NxZlhRFfrETMLNZE5krM3enli1ov+6F3aCmH4/OlmPOAOpyAwPJ+ddTTyYhl4S X-Received: by 10.223.164.150 with SMTP id g22mr6381975wrb.148.1486025142893; Thu, 02 Feb 2017 00:45:42 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 191sm1850123wmo.21.2017.02.02.00.45.41 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Feb 2017 00:45:42 -0800 (PST) Date: Thu, 2 Feb 2017 09:45:35 +0100 From: Adrien Mazarguil To: Ferruh Yigit , Shahaf Shuler Cc: =?utf-8?B?TsOpbGlv?= Laranjeiro , "dev@dpdk.org" , "stable@dpdk.org" Message-ID: <20170202084534.GT10133@6wind.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <963be495-c009-2937-5a14-204ae3a3b245@intel.com> 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 08:45:43 -0000 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. 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? - By removing the check on ETHTOOL_GLINKSETTINGS, this code breaks compilation for Linux kernel version < 4.5, which I think is a problem. 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. Ferruh, please revert. 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, > >>>>> <...> > >>> > >> > > > -- Adrien Mazarguil 6WIND