Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query

From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Shahaf Shuler <shahafs@mellanox.com>,
	adrien.mazarguil@6wind.com, nelio.laranjeiro@6wind.com
Cc: dev@dpdk.org, stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
Date: Tue, 31 Jan 2017 16:17:09 +0000	[thread overview]
Message-ID: <c9f8112d-b308-9dff-5d2b-71862dbcb3b7@intel.com> (raw)
In-Reply-To: <1485863129-6326-1-git-send-email-shahafs@mellanox.com>

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 <shahafs@mellanox.com>
> ---

<...>

> @@ -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?

>  	struct priv *priv = mlx5_get_priv(dev);
>  	struct ethtool_link_settings edata = {
>  		.cmd = ETHTOOL_GLINKSETTINGS,
<...>