* [PATCH 1/2] net/mlx5: fix link status query
@ 2017-01-25 12:42 Shahaf Shuler
2017-01-30 15:25 ` Nélio Laranjeiro
2017-01-31 11:45 ` [PATCH v2] " Shahaf Shuler
0 siblings, 2 replies; 19+ messages in thread
From: Shahaf Shuler @ 2017-01-25 12:42 UTC (permalink / raw)
To: adrien.mazarguil, nelio.laranjeiro; +Cc: dev, stable
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>
---
while the ETHTOOL_GLINKSETTINGS API was introduced on kernel v4.6 [1]
the bug was fixed only on kernel v4.9 [2]
[1] https://github.com/torvalds/linux/commit/3f1ac7a700d039c61d8d8b99f28d605d489a60cf
[2] https://github.com/torvalds/linux/commit/8006f6bf5e39f11c697f48df20382b81d2f2f8b8
---
drivers/net/mlx5/mlx5_ethdev.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 8efdff7..e77238f 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -46,6 +46,7 @@
#include <netinet/in.h>
#include <linux/ethtool.h>
#include <linux/sockios.h>
+#include <linux/version.h>
#include <fcntl.h>
/* DPDK headers don't like -pedantic. */
@@ -697,7 +698,7 @@ struct priv *
/**
* Retrieve physical link information (unlocked version using new ioctl from
- * Linux 4.5).
+ * Linux 4.9).
*
* @param dev
* Pointer to Ethernet device structure.
@@ -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
struct priv *priv = mlx5_get_priv(dev);
struct ethtool_link_settings edata = {
.cmd = ETHTOOL_GLINKSETTINGS,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] net/mlx5: fix link status query
2017-01-25 12:42 [PATCH 1/2] net/mlx5: fix link status query Shahaf Shuler
@ 2017-01-30 15:25 ` Nélio Laranjeiro
2017-01-31 11:45 ` [PATCH v2] " Shahaf Shuler
1 sibling, 0 replies; 19+ messages in thread
From: Nélio Laranjeiro @ 2017-01-30 15:25 UTC (permalink / raw)
To: Shahaf Shuler; +Cc: Adrien Mazarguil, dev, stable
On Wed, Jan 25, 2017 at 02:42:58PM +0200, 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>
> ---
> while the ETHTOOL_GLINKSETTINGS API was introduced on kernel v4.6 [1]
> the bug was fixed only on kernel v4.9 [2]
>
> [1] https://github.com/torvalds/linux/commit/3f1ac7a700d039c61d8d8b99f28d605d489a60cf
> [2] https://github.com/torvalds/linux/commit/8006f6bf5e39f11c697f48df20382b81d2f2f8b8
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 8efdff7..e77238f 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -46,6 +46,7 @@
> #include <netinet/in.h>
> #include <linux/ethtool.h>
> #include <linux/sockios.h>
> +#include <linux/version.h>
> #include <fcntl.h>
>
> /* DPDK headers don't like -pedantic. */
> @@ -697,7 +698,7 @@ struct priv *
>
> /**
> * Retrieve physical link information (unlocked version using new ioctl from
> - * Linux 4.5).
> + * Linux 4.9).
> *
> * @param dev
> * Pointer to Ethernet device structure.
> @@ -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
> struct priv *priv = mlx5_get_priv(dev);
> struct ethtool_link_settings edata = {
> .cmd = ETHTOOL_GLINKSETTINGS,
> --
> 1.8.3.1
Hi Shahaf,
This function embeds some HAVE_ETHTOOL_LINK_MODE_* to handle the
different version of Linux Kernel where those link speeds were added.
With this patch, they becomes useless.
It could be great for configuration and compilation time to remove them
from this file and the Makefile.
Regards,
--
Nélio Laranjeiro
6WIND
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] net/mlx5: fix link status query
2017-01-25 12:42 [PATCH 1/2] net/mlx5: fix link status query Shahaf Shuler
2017-01-30 15:25 ` Nélio Laranjeiro
@ 2017-01-31 11:45 ` Shahaf Shuler
2017-01-31 15:42 ` Nélio Laranjeiro
` (2 more replies)
1 sibling, 3 replies; 19+ messages in thread
From: Shahaf Shuler @ 2017-01-31 11:45 UTC (permalink / raw)
To: adrien.mazarguil, nelio.laranjeiro; +Cc: dev, stable
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>
---
on v2:
* remove HAVE_ETHTOOL_LINK_MODE_*
---
drivers/net/mlx5/Makefile | 15 ---------------
drivers/net/mlx5/mlx5_ethdev.c | 12 +++---------
2 files changed, 3 insertions(+), 24 deletions(-)
diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
index 671089c..0b8f7ba 100644
--- a/drivers/net/mlx5/Makefile
+++ b/drivers/net/mlx5/Makefile
@@ -122,21 +122,6 @@ mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
infiniband/mlx5_hw.h \
enum MLX5_OPCODE_TSO \
$(AUTOCONF_OUTPUT)
- $Q sh -- '$<' '$@' \
- HAVE_ETHTOOL_LINK_MODE_25G \
- /usr/include/linux/ethtool.h \
- enum ETHTOOL_LINK_MODE_25000baseCR_Full_BIT \
- $(AUTOCONF_OUTPUT)
- $Q sh -- '$<' '$@' \
- HAVE_ETHTOOL_LINK_MODE_50G \
- /usr/include/linux/ethtool.h \
- enum ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT \
- $(AUTOCONF_OUTPUT)
- $Q sh -- '$<' '$@' \
- HAVE_ETHTOOL_LINK_MODE_100G \
- /usr/include/linux/ethtool.h \
- enum ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT \
- $(AUTOCONF_OUTPUT)
# Create mlx5_autoconf.h or update it in case it differs from the new one.
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 8efdff7..53599fa 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -46,6 +46,7 @@
#include <netinet/in.h>
#include <linux/ethtool.h>
#include <linux/sockios.h>
+#include <linux/version.h>
#include <fcntl.h>
/* DPDK headers don't like -pedantic. */
@@ -697,7 +698,7 @@ struct priv *
/**
* Retrieve physical link information (unlocked version using new ioctl from
- * Linux 4.5).
+ * Linux 4.9).
*
* @param dev
* Pointer to Ethernet device structure.
@@ -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
struct priv *priv = mlx5_get_priv(dev);
struct ethtool_link_settings edata = {
.cmd = ETHTOOL_GLINKSETTINGS,
@@ -757,25 +758,18 @@ struct priv *
ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT |
ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_56G;
- /* Link speeds available in kernel v4.6. */
-#ifdef HAVE_ETHTOOL_LINK_MODE_25G
if (sc & (ETHTOOL_LINK_MODE_25000baseCR_Full_BIT |
ETHTOOL_LINK_MODE_25000baseKR_Full_BIT |
ETHTOOL_LINK_MODE_25000baseSR_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_25G;
-#endif
-#ifdef HAVE_ETHTOOL_LINK_MODE_50G
if (sc & (ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT |
ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_50G;
-#endif
-#ifdef HAVE_ETHTOOL_LINK_MODE_100G
if (sc & (ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT |
ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT |
ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT |
ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_100G;
-#endif
dev_link.link_duplex = ((edata.duplex == DUPLEX_HALF) ?
ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net/mlx5: fix link status query
2017-01-31 11:45 ` [PATCH v2] " Shahaf Shuler
@ 2017-01-31 15:42 ` Nélio Laranjeiro
2017-02-01 18:31 ` [dpdk-stable] " Ferruh Yigit
2017-01-31 16:17 ` Ferruh Yigit
2017-02-09 12:29 ` [PATCH v3] " Shahaf Shuler
2 siblings, 1 reply; 19+ messages in thread
From: Nélio Laranjeiro @ 2017-01-31 15:42 UTC (permalink / raw)
To: Shahaf Shuler; +Cc: guillaume.gaudonville, adrien.mazarguil, dev, stable
On Tue, Jan 31, 2017 at 01:45:29PM +0200, 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>
> ---
> on v2:
> * remove HAVE_ETHTOOL_LINK_MODE_*
>
> ---
> drivers/net/mlx5/Makefile | 15 ---------------
> drivers/net/mlx5/mlx5_ethdev.c | 12 +++---------
> 2 files changed, 3 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> index 671089c..0b8f7ba 100644
> --- a/drivers/net/mlx5/Makefile
> +++ b/drivers/net/mlx5/Makefile
> @@ -122,21 +122,6 @@ mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-config-h.sh
> infiniband/mlx5_hw.h \
> enum MLX5_OPCODE_TSO \
> $(AUTOCONF_OUTPUT)
> - $Q sh -- '$<' '$@' \
> - HAVE_ETHTOOL_LINK_MODE_25G \
> - /usr/include/linux/ethtool.h \
> - enum ETHTOOL_LINK_MODE_25000baseCR_Full_BIT \
> - $(AUTOCONF_OUTPUT)
> - $Q sh -- '$<' '$@' \
> - HAVE_ETHTOOL_LINK_MODE_50G \
> - /usr/include/linux/ethtool.h \
> - enum ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT \
> - $(AUTOCONF_OUTPUT)
> - $Q sh -- '$<' '$@' \
> - HAVE_ETHTOOL_LINK_MODE_100G \
> - /usr/include/linux/ethtool.h \
> - enum ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT \
> - $(AUTOCONF_OUTPUT)
>
> # Create mlx5_autoconf.h or update it in case it differs from the new one.
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 8efdff7..53599fa 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -46,6 +46,7 @@
> #include <netinet/in.h>
> #include <linux/ethtool.h>
> #include <linux/sockios.h>
> +#include <linux/version.h>
> #include <fcntl.h>
>
> /* DPDK headers don't like -pedantic. */
> @@ -697,7 +698,7 @@ struct priv *
>
> /**
> * Retrieve physical link information (unlocked version using new ioctl from
> - * Linux 4.5).
> + * Linux 4.9).
> *
> * @param dev
> * Pointer to Ethernet device structure.
> @@ -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
> struct priv *priv = mlx5_get_priv(dev);
> struct ethtool_link_settings edata = {
> .cmd = ETHTOOL_GLINKSETTINGS,
> @@ -757,25 +758,18 @@ struct priv *
> ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT |
> ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT))
> priv->link_speed_capa |= ETH_LINK_SPEED_56G;
> - /* Link speeds available in kernel v4.6. */
> -#ifdef HAVE_ETHTOOL_LINK_MODE_25G
> if (sc & (ETHTOOL_LINK_MODE_25000baseCR_Full_BIT |
> ETHTOOL_LINK_MODE_25000baseKR_Full_BIT |
> ETHTOOL_LINK_MODE_25000baseSR_Full_BIT))
> priv->link_speed_capa |= ETH_LINK_SPEED_25G;
> -#endif
> -#ifdef HAVE_ETHTOOL_LINK_MODE_50G
> if (sc & (ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT |
> ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT))
> priv->link_speed_capa |= ETH_LINK_SPEED_50G;
> -#endif
> -#ifdef HAVE_ETHTOOL_LINK_MODE_100G
> if (sc & (ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT |
> ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT |
> ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT |
> ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT))
> priv->link_speed_capa |= ETH_LINK_SPEED_100G;
> -#endif
> dev_link.link_duplex = ((edata.duplex == DUPLEX_HALF) ?
> ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
> dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> --
> 1.8.3.1
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
--
Nélio Laranjeiro
6WIND
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-01-31 11:45 ` [PATCH v2] " Shahaf Shuler
2017-01-31 15:42 ` Nélio Laranjeiro
@ 2017-01-31 16:17 ` Ferruh Yigit
2017-02-01 6:53 ` Shahaf Shuler
2017-02-09 12:29 ` [PATCH v3] " Shahaf Shuler
2 siblings, 1 reply; 19+ messages in thread
From: Ferruh Yigit @ 2017-01-31 16:17 UTC (permalink / raw)
To: Shahaf Shuler, adrien.mazarguil, nelio.laranjeiro; +Cc: dev, stable
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,
<...>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-01-31 16:17 ` Ferruh Yigit
@ 2017-02-01 6:53 ` Shahaf Shuler
2017-02-01 9:07 ` Adrien Mazarguil
0 siblings, 1 reply; 19+ messages in thread
From: Shahaf Shuler @ 2017-02-01 6:53 UTC (permalink / raw)
To: Ferruh Yigit, Adrien Mazarguil, Nélio Laranjeiro; +Cc: dev, stable
: 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 <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?
This is a good suggestion.
Adrien, Nélio what do you think?
>
> > struct priv *priv = mlx5_get_priv(dev);
> > struct ethtool_link_settings edata = {
> > .cmd = ETHTOOL_GLINKSETTINGS,
> <...>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-01 6:53 ` Shahaf Shuler
@ 2017-02-01 9:07 ` Adrien Mazarguil
2017-02-01 11:13 ` Ferruh Yigit
0 siblings, 1 reply; 19+ messages in thread
From: Adrien Mazarguil @ 2017-02-01 9:07 UTC (permalink / raw)
To: Shahaf Shuler; +Cc: Ferruh Yigit, Nélio Laranjeiro, dev, stable
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 <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?
>
> 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.
> > > struct priv *priv = mlx5_get_priv(dev);
> > > struct ethtool_link_settings edata = {
> > > .cmd = ETHTOOL_GLINKSETTINGS,
> > <...>
--
Adrien Mazarguil
6WIND
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-01 9:07 ` Adrien Mazarguil
@ 2017-02-01 11:13 ` Ferruh Yigit
2017-02-01 12:57 ` Adrien Mazarguil
0 siblings, 1 reply; 19+ messages in thread
From: Ferruh Yigit @ 2017-02-01 11:13 UTC (permalink / raw)
To: Adrien Mazarguil, Shahaf Shuler; +Cc: Nélio Laranjeiro, dev, stable
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 <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?
>>
>> 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 "<="
>
>>>> struct priv *priv = mlx5_get_priv(dev);
>>>> struct ethtool_link_settings edata = {
>>>> .cmd = ETHTOOL_GLINKSETTINGS,
>>> <...>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-01 11:13 ` Ferruh Yigit
@ 2017-02-01 12:57 ` Adrien Mazarguil
2017-02-01 18:11 ` Ferruh Yigit
0 siblings, 1 reply; 19+ messages in thread
From: Adrien Mazarguil @ 2017-02-01 12:57 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Shahaf Shuler, Nélio Laranjeiro, dev, stable
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 <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?
> >>
> >> 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)
Which is the correct intent. I guess you can update this line for clarity if
you think it's necessary.
> >
> >>>> struct priv *priv = mlx5_get_priv(dev);
> >>>> struct ethtool_link_settings edata = {
> >>>> .cmd = ETHTOOL_GLINKSETTINGS,
> >>> <...>
> >
>
--
Adrien Mazarguil
6WIND
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-01 12:57 ` Adrien Mazarguil
@ 2017-02-01 18:11 ` Ferruh Yigit
2017-02-02 8:45 ` Adrien Mazarguil
0 siblings, 1 reply; 19+ messages in thread
From: Ferruh Yigit @ 2017-02-01 18:11 UTC (permalink / raw)
To: Adrien Mazarguil; +Cc: Shahaf Shuler, Nélio Laranjeiro, dev, stable
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 <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?
>>>>
>>>> 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,
>>>>> <...>
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-01-31 15:42 ` Nélio Laranjeiro
@ 2017-02-01 18:31 ` Ferruh Yigit
2017-02-02 8:20 ` Nélio Laranjeiro
0 siblings, 1 reply; 19+ messages in thread
From: Ferruh Yigit @ 2017-02-01 18:31 UTC (permalink / raw)
To: Nélio Laranjeiro, Shahaf Shuler
Cc: guillaume.gaudonville, adrien.mazarguil, dev, stable
On 1/31/2017 3:42 PM, Nélio Laranjeiro wrote:
> On Tue, Jan 31, 2017 at 01:45:29PM +0200, 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>
> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Applied to dpdk-next-net/master, thanks.
(Updated kernel version check to:
"+#if KERNEL_VERSION(4, 9, 0) < LINUX_VERSION_CODE")
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-01 18:31 ` [dpdk-stable] " Ferruh Yigit
@ 2017-02-02 8:20 ` Nélio Laranjeiro
2017-02-02 10:18 ` Ferruh Yigit
0 siblings, 1 reply; 19+ messages in thread
From: Nélio Laranjeiro @ 2017-02-02 8:20 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Shahaf Shuler, adrien.mazarguil, dev, stable
On Wed, Feb 01, 2017 at 06:31:17PM +0000, Ferruh Yigit wrote:
> On 1/31/2017 3:42 PM, Nélio Laranjeiro wrote:
> > On Tue, Jan 31, 2017 at 01:45:29PM +0200, 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>
>
> > Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>
> Applied to dpdk-next-net/master, thanks.
>
> (Updated kernel version check to:
> "+#if KERNEL_VERSION(4, 9, 0) < LINUX_VERSION_CODE")
Ferruh,
The issue is fixed in 4.9, so <= 4.9 is correct.
Am I missing something?
Regards,
--
Nélio Laranjeiro
6WIND
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-01 18:11 ` Ferruh Yigit
@ 2017-02-02 8:45 ` Adrien Mazarguil
2017-02-02 10:15 ` Ferruh Yigit
0 siblings, 1 reply; 19+ messages in thread
From: Adrien Mazarguil @ 2017-02-02 8:45 UTC (permalink / raw)
To: Ferruh Yigit, Shahaf Shuler; +Cc: Nélio Laranjeiro, dev, stable
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 <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?
> >>>>
> >>>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-02 8:45 ` Adrien Mazarguil
@ 2017-02-02 10:15 ` Ferruh Yigit
2017-02-02 10:37 ` Adrien Mazarguil
0 siblings, 1 reply; 19+ messages in thread
From: Ferruh Yigit @ 2017-02-02 10:15 UTC (permalink / raw)
To: Adrien Mazarguil, Shahaf Shuler; +Cc: Nélio Laranjeiro, dev, stable
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 <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?
>>>>>>
>>>>>> 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,
>>>>>>> <...>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-02 8:20 ` Nélio Laranjeiro
@ 2017-02-02 10:18 ` Ferruh Yigit
0 siblings, 0 replies; 19+ messages in thread
From: Ferruh Yigit @ 2017-02-02 10:18 UTC (permalink / raw)
To: Nélio Laranjeiro; +Cc: Shahaf Shuler, adrien.mazarguil, dev, stable
On 2/2/2017 8:20 AM, Nélio Laranjeiro wrote:
> On Wed, Feb 01, 2017 at 06:31:17PM +0000, Ferruh Yigit wrote:
>> On 1/31/2017 3:42 PM, Nélio Laranjeiro wrote:
>>> On Tue, Jan 31, 2017 at 01:45:29PM +0200, 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>
>>
>>> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
>>
>> Applied to dpdk-next-net/master, thanks.
>>
>> (Updated kernel version check to:
>> "+#if KERNEL_VERSION(4, 9, 0) < LINUX_VERSION_CODE")
>
> Ferruh,
>
> The issue is fixed in 4.9, so <= 4.9 is correct.
>
> Am I missing something?
My bad Nelio,
I converted back to original, please check.
>
> Regards,
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-02 10:15 ` Ferruh Yigit
@ 2017-02-02 10:37 ` Adrien Mazarguil
2017-02-02 10:43 ` Ferruh Yigit
0 siblings, 1 reply; 19+ messages in thread
From: Adrien Mazarguil @ 2017-02-02 10:37 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: Shahaf Shuler, Nélio Laranjeiro, dev, stable
On Thu, Feb 02, 2017 at 10:15:08AM +0000, Ferruh Yigit wrote:
> 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 <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?
> >>>>>>
> >>>>>> 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.
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).
> > - 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.
> > 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.
> 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.
> > 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?
> > 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v2] net/mlx5: fix link status query
2017-02-02 10:37 ` Adrien Mazarguil
@ 2017-02-02 10:43 ` Ferruh Yigit
0 siblings, 0 replies; 19+ messages in thread
From: Ferruh Yigit @ 2017-02-02 10:43 UTC (permalink / raw)
To: Adrien Mazarguil; +Cc: Shahaf Shuler, Nélio Laranjeiro, dev, stable
On 2/2/2017 10:37 AM, Adrien Mazarguil wrote:
> On Thu, Feb 02, 2017 at 10:15:08AM +0000, Ferruh Yigit wrote:
>> 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 <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?
>>>>>>>>
>>>>>>>> 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.
>
> 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).
>
>>> - 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.
>
>>> 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.
>
>> 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.
>
>>> 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.
>
>>> 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,
>>>>>>>>> <...>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] net/mlx5: fix link status query
2017-01-31 11:45 ` [PATCH v2] " Shahaf Shuler
2017-01-31 15:42 ` Nélio Laranjeiro
2017-01-31 16:17 ` Ferruh Yigit
@ 2017-02-09 12:29 ` Shahaf Shuler
2017-02-09 14:45 ` [dpdk-stable] " Ferruh Yigit
2 siblings, 1 reply; 19+ messages in thread
From: Shahaf Shuler @ 2017-02-09 12:29 UTC (permalink / raw)
To: adrien.mazarguil, nelio.laranjeiro; +Cc: dev, stable
Trying to query the link status through the new ETHTOOL_GLINKSETTINGS
ioctl available since Linux 4.5 was always failing due to a kernel bug
fixed since version 4.9.
This commit also addresses a common issue where the headers version used
at compile time differs from that of the kernel on the target system, by
always defining missing symbols and moving the kernel version check at run
time.
Fixes: 188408719888 ("net/mlx5: fix support for newer link speeds")
CC: stable@dpdk.org
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
on v3:
* Dynamic decision on the link_update function according to the running kernel.
on v2:
* remove HAVE_ETHTOOL_LINK_MODE_*
---
drivers/net/mlx5/mlx5_ethdev.c | 84 +++++++++++++++++++++++++++++++-----------
1 file changed, 63 insertions(+), 21 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 8efdff7..40336ad 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -43,9 +43,11 @@
#include <net/if.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
+#include <sys/utsname.h>
#include <netinet/in.h>
#include <linux/ethtool.h>
#include <linux/sockios.h>
+#include <linux/version.h>
#include <fcntl.h>
/* DPDK headers don't like -pedantic. */
@@ -67,6 +69,57 @@
#include "mlx5_rxtx.h"
#include "mlx5_utils.h"
+/* Add defines in case the running kernel is not the same as user headers. */
+#ifndef ETHTOOL_GLINKSETTINGS
+struct ethtool_link_settings {
+ uint32_t cmd;
+ uint32_t speed;
+ uint8_t duplex;
+ uint8_t port;
+ uint8_t phy_address;
+ uint8_t autoneg;
+ uint8_t mdio_support;
+ uint8_t eth_to_mdix;
+ uint8_t eth_tp_mdix_ctrl;
+ int8_t link_mode_masks_nwords;
+ uint32_t reserved[8];
+ uint32_t link_mode_masks[];
+};
+
+#define ETHTOOL_GLINKSETTINGS 0x0000004c
+#define ETHTOOL_LINK_MODE_1000baseT_Full_BIT 5
+#define ETHTOOL_LINK_MODE_Autoneg_BIT 6
+#define ETHTOOL_LINK_MODE_1000baseKX_Full_BIT 17
+#define ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT 18
+#define ETHTOOL_LINK_MODE_10000baseKR_Full_BIT 19
+#define ETHTOOL_LINK_MODE_10000baseR_FEC_BIT 20
+#define ETHTOOL_LINK_MODE_20000baseMLD2_Full_BIT 21
+#define ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT 22
+#define ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT 23
+#define ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT 24
+#define ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT 25
+#define ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT 26
+#define ETHTOOL_LINK_MODE_56000baseKR4_Full_BIT 27
+#define ETHTOOL_LINK_MODE_56000baseCR4_Full_BIT 28
+#define ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT 29
+#define ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT 30
+#endif
+#ifndef HAVE_ETHTOOL_LINK_MODE_25G
+#define ETHTOOL_LINK_MODE_25000baseCR_Full_BIT 31
+#define ETHTOOL_LINK_MODE_25000baseKR_Full_BIT 32
+#define ETHTOOL_LINK_MODE_25000baseSR_Full_BIT 33
+#endif
+#ifndef HAVE_ETHTOOL_LINK_MODE_50G
+#define ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT 34
+#define ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT 35
+#endif
+#ifndef HAVE_ETHTOOL_LINK_MODE_100G
+#define ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT 36
+#define ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT 37
+#define ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT 38
+#define ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT 39
+#endif
+
/**
* Return private structure associated with an Ethernet device.
*
@@ -696,8 +749,7 @@ struct priv *
}
/**
- * Retrieve physical link information (unlocked version using new ioctl from
- * Linux 4.5).
+ * Retrieve physical link information (unlocked version using new ioctl).
*
* @param dev
* Pointer to Ethernet device structure.
@@ -707,7 +759,6 @@ struct priv *
static int
mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete)
{
-#ifdef ETHTOOL_GLINKSETTINGS
struct priv *priv = mlx5_get_priv(dev);
struct ethtool_link_settings edata = {
.cmd = ETHTOOL_GLINKSETTINGS,
@@ -734,7 +785,6 @@ struct priv *
sc = edata.link_mode_masks[0] |
((uint64_t)edata.link_mode_masks[1] << 32);
priv->link_speed_capa = 0;
- /* Link speeds available in kernel v4.5. */
if (sc & ETHTOOL_LINK_MODE_Autoneg_BIT)
priv->link_speed_capa |= ETH_LINK_SPEED_AUTONEG;
if (sc & (ETHTOOL_LINK_MODE_1000baseT_Full_BIT |
@@ -757,25 +807,18 @@ struct priv *
ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT |
ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_56G;
- /* Link speeds available in kernel v4.6. */
-#ifdef HAVE_ETHTOOL_LINK_MODE_25G
if (sc & (ETHTOOL_LINK_MODE_25000baseCR_Full_BIT |
ETHTOOL_LINK_MODE_25000baseKR_Full_BIT |
ETHTOOL_LINK_MODE_25000baseSR_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_25G;
-#endif
-#ifdef HAVE_ETHTOOL_LINK_MODE_50G
if (sc & (ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT |
ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_50G;
-#endif
-#ifdef HAVE_ETHTOOL_LINK_MODE_100G
if (sc & (ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT |
ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT |
ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT |
ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT))
priv->link_speed_capa |= ETH_LINK_SPEED_100G;
-#endif
dev_link.link_duplex = ((edata.duplex == DUPLEX_HALF) ?
ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
@@ -785,10 +828,6 @@ struct priv *
dev->data->dev_link = dev_link;
return 0;
}
-#else
- (void)dev;
- (void)wait_to_complete;
-#endif
/* Link status is still the same. */
return -1;
}
@@ -804,12 +843,15 @@ struct priv *
int
mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete)
{
- int ret;
-
- ret = mlx5_link_update_unlocked_gs(dev, wait_to_complete);
- if (ret < 0)
- ret = mlx5_link_update_unlocked_gset(dev, wait_to_complete);
- return ret;
+ struct utsname utsname;
+ int ver[3];
+
+ if (uname(&utsname) == -1 ||
+ sscanf(utsname.release, "%d.%d.%d",
+ &ver[0], &ver[1], &ver[2]) != 3 ||
+ KERNEL_VERSION(ver[0], ver[1], ver[2]) < KERNEL_VERSION(4, 9, 0))
+ return mlx5_link_update_unlocked_gset(dev, wait_to_complete);
+ return mlx5_link_update_unlocked_gs(dev, wait_to_complete);
}
/**
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v3] net/mlx5: fix link status query
2017-02-09 12:29 ` [PATCH v3] " Shahaf Shuler
@ 2017-02-09 14:45 ` Ferruh Yigit
0 siblings, 0 replies; 19+ messages in thread
From: Ferruh Yigit @ 2017-02-09 14:45 UTC (permalink / raw)
To: Shahaf Shuler, adrien.mazarguil, nelio.laranjeiro; +Cc: dev, stable
On 2/9/2017 12:29 PM, Shahaf Shuler wrote:
> Trying to query the link status through the new ETHTOOL_GLINKSETTINGS
> ioctl available since Linux 4.5 was always failing due to a kernel bug
> fixed since version 4.9.
>
> This commit also addresses a common issue where the headers version used
> at compile time differs from that of the kernel on the target system, by
> always defining missing symbols and moving the kernel version check at run
> time.
>
> Fixes: 188408719888 ("net/mlx5: fix support for newer link speeds")
> CC: stable@dpdk.org
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Applied to dpdk-next-net/master, thanks.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-02-09 14:45 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-25 12:42 [PATCH 1/2] net/mlx5: fix link status query Shahaf Shuler
2017-01-30 15:25 ` Nélio Laranjeiro
2017-01-31 11:45 ` [PATCH v2] " Shahaf Shuler
2017-01-31 15:42 ` Nélio Laranjeiro
2017-02-01 18:31 ` [dpdk-stable] " Ferruh Yigit
2017-02-02 8:20 ` Nélio Laranjeiro
2017-02-02 10:18 ` Ferruh Yigit
2017-01-31 16:17 ` Ferruh Yigit
2017-02-01 6:53 ` Shahaf Shuler
2017-02-01 9:07 ` Adrien Mazarguil
2017-02-01 11:13 ` Ferruh Yigit
2017-02-01 12:57 ` Adrien Mazarguil
2017-02-01 18:11 ` Ferruh Yigit
2017-02-02 8:45 ` Adrien Mazarguil
2017-02-02 10:15 ` Ferruh Yigit
2017-02-02 10:37 ` Adrien Mazarguil
2017-02-02 10:43 ` Ferruh Yigit
2017-02-09 12:29 ` [PATCH v3] " Shahaf Shuler
2017-02-09 14:45 ` [dpdk-stable] " Ferruh Yigit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.