On 2/2/2017 10:37 AM, Adrien Mazarguil wrote:

On Thu, Feb 02, 2017 at 10:15:08AM +0000, Ferruh Yigit wrote:

quoted

On 2/2/2017 8:45 AM, Adrien Mazarguil wrote:

quoted

On Wed, Feb 01, 2017 at 06:11:17PM +0000, Ferruh Yigit wrote:

quoted

On 2/1/2017 12:57 PM, Adrien Mazarguil wrote:

quoted

On Wed, Feb 01, 2017 at 11:13:59AM +0000, Ferruh Yigit wrote:

quoted

On 2/1/2017 9:07 AM, Adrien Mazarguil wrote:

quoted

On Wed, Feb 01, 2017 at 06:53:55AM +0000, Shahaf Shuler wrote:

quoted

: Tuesday, January 31, 2017 6:17 PM, Ferruh Yigit:

quoted

On 1/31/2017 11:45 AM, Shahaf Shuler wrote:

quoted

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 <redacted>
---

<...>

quoted

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

quoted

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.

Right, what I'm getting at is this code is supposed to work regardless of
the running kernel version (uname -r), userland headers version
(/usr/include/linux) and source tree version (/usr/src/linux/), all of them
may differ. Basically this PMD can be built on a system and run on another
since it's not a kernel module. In short the following scenarios should be
handled:

- Kernel (uname -r) is 3.2, /usr/include/linux comes from 4.5, and
  /usr/src/linux comes from 4.10.

- Kernel (uname -r) is 4.10, /usr/include/linux comes from 3.2 and 
  /usr/src/linux does not even exist.

- Kernel (uname -r) is 4.8, /usr/include/linux comes from 4.9.

- Kernel (uname -r) is 4.9, /usr/include/linux comes from 4.9 (when one gets
  lucky enough).

quoted

quoted

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

You're right, ETHTOOL_GLINKSETTINGS should be consistent with
LINUX_VERSION_CODE since both includes come from the same
directory. We normally don't need to manage the case where a system has
broken includes, however as you're pointing out below distributions
sometimes make backports and the kernel version means nothing. Relying on
ETHTOOL_GLINKSETTINGS's presence makes more sense.

quoted

quoted

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.

My thought as well.

quoted

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

Yeah, especially since it's not a kernel module, a PMD must perform a
runtime version check.

quoted

quoted

Ferruh, please revert.

But original patch looks good, I will convert it to initial patch, and
keep it until above two items clarified.

I've talked to Shahaf/Nelio, they already intend to send an updated version
due to the above concerns. Do you prefer a fix on top of it?

No, if there will be a new version, I will drop the existing one in the
repo.

quoted

quoted

stable@dpdk.org, please ignore this commit for the the time being, and sorry
for the noise.

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

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

<...>