mlx5: remove dependency on kernel version

Since wait_to_complete is unused, don't pass it to helper functions. Use the standard RTE macro to indicate this is an unused parameter.

Hi Stephen,

Quoted 2 lines · Stephen Hemminger

Since wait_to_complete is unused, don't pass it to helper functions. Use the standard RTE macro to indicate this is an unused parameter.

I would suggest to use the (void) as it is done in the whole driver, a specific patch should be done to use the rte_unused macro in all the sources at once.

Thanks,

Quoted 10 lines · On Wed, 3 Jan 2018 08:35:23 +0100 Nelio Laranjeiro…

Hi Stephen,

Quoted 2 lines · Stephen Hemminger

Since wait_to_complete is unused, don't pass it to helper functions. Use the standard RTE macro to indicate this is an unused parameter.

I would suggest to use the (void) as it is done in the whole driver, a specific patch should be done to use the rte_unused macro in all the sources at once.

Quoted 2 lines

Thanks,

There is a standard in DPDK using RTE macros. The whole driver should follow that rather than trying to be different.

This driver uses ethtool to get link status. The ethtool API has new and old deprecated API. Rather than checking kernel version, use the same algorithm that the ethtool command does; check the new API first and if that fails, try the old one.

Also, use common code for getting link state up/down and comparing for changes.

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/mlx5/mlx5_ethdev.c | 110 ++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 63 deletions(-)

drivers/net/mlx5/mlx5_ethdev.c +0 -1

index 388507f109f7..2dc32cdf58b9 100644
@@ -49,7 +49,6 @@
 #include <netinet/in.h>
 #include <linux/ethtool.h>
 #include <linux/sockios.h>
-#include <linux/version.h>
 #include <fcntl.h>
 #include <stdalign.h>
 #include <sys/un.h>

drivers/net/mlx5/mlx5_ethdev.c +6 -26

 {
-	struct priv *priv = mlx5_get_priv(dev);
 	struct ethtool_cmd edata = {
 		.cmd = ETHTOOL_GSET /* Deprecated since Linux v4.5. */
 	};
-	struct ifreq ifr;
-	struct rte_eth_link dev_link;
 	int link_speed = 0;
 
-	/* priv_lock() is not taken to allow concurrent calls. */
-
-	if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
-		WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
-		return -1;
-	}
-	memset(&dev_link, 0, sizeof(dev_link));
-	dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
-				(ifr.ifr_flags & IFF_RUNNING));
-	ifr.ifr_data = (void *)&edata;
-	if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) {
+	ifr->ifr_data = (void *)&edata;
+	if (priv_ifreq(priv, SIOCETHTOOL, ifr)) {
 		WARN("ioctl(SIOCETHTOOL, ETHTOOL_GSET) failed: %s",
 		     strerror(errno));
 		return -1;
 	}
 	link_speed = ethtool_cmd_speed(&edata);
 	if (link_speed == -1)
-		dev_link.link_speed = 0;
+		dev_link->link_speed = 0;
 	else
-		dev_link.link_speed = link_speed;
+		dev_link->link_speed = link_speed;
 	priv->link_speed_capa = 0;
 	if (edata.supported & SUPPORTED_Autoneg)
 		priv->link_speed_capa |= ETH_LINK_SPEED_AUTONEG;

@@ -800,17 +788,9 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
 			       SUPPORTED_40000baseSR4_Full |
 			       SUPPORTED_40000baseLR4_Full))
 		priv->link_speed_capa |= ETH_LINK_SPEED_40G;
-	dev_link.link_duplex = ((edata.duplex == DUPLEX_HALF) ?
+	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 &
-			ETH_LINK_SPEED_FIXED);
-	if (memcmp(&dev_link, &dev->data->dev_link, sizeof(dev_link))) {
-		/* Link status changed. */
-		dev->data->dev_link = dev_link;
-		return 0;
-	}
-	/* Link status is still the same. */
-	return -1;
+	return 0;
 }
 
 /**

drivers/net/mlx5/mlx5_ethdev.c +27 -34

 {
-	struct priv *priv = mlx5_get_priv(dev);
 	struct ethtool_link_settings gcmd = { .cmd = ETHTOOL_GLINKSETTINGS };
-	struct ifreq ifr;
-	struct rte_eth_link dev_link;
 	uint64_t sc;
 
-	if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
-		WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
-		return -1;
-	}
-	memset(&dev_link, 0, sizeof(dev_link));
-	dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
-				(ifr.ifr_flags & IFF_RUNNING));
-	ifr.ifr_data = (void *)&gcmd;
-	if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) {
+	ifr->ifr_data = (void *)&gcmd;
+	if (priv_ifreq(priv, SIOCETHTOOL, ifr)) {
 		DEBUG("ioctl(SIOCETHTOOL, ETHTOOL_GLINKSETTINGS) failed: %s",
 		      strerror(errno));
 		return -1;

@@ -849,13 +820,13 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
 	struct ethtool_link_settings *ecmd = (void *)data;
 
 	*ecmd = gcmd;
-	ifr.ifr_data = (void *)ecmd;
-	if (priv_ifreq(priv, SIOCETHTOOL, &ifr)) {
+	ifr->ifr_data = (void *)ecmd;
+	if (priv_ifreq(priv, SIOCETHTOOL, ifr)) {
 		DEBUG("ioctl(SIOCETHTOOL, ETHTOOL_GLINKSETTINGS) failed: %s",
 		      strerror(errno));
 		return -1;
 	}
-	dev_link.link_speed = ecmd->speed;
+	dev_link->link_speed = ecmd->speed;
 	sc = ecmd->link_mode_masks[0] |
 		((uint64_t)ecmd->link_mode_masks[1] << 32);
 	priv->link_speed_capa = 0;

@@ -893,17 +864,9 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
 		  MLX5_BITSHIFT(ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT) |
 		  MLX5_BITSHIFT(ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT)))
 		priv->link_speed_capa |= ETH_LINK_SPEED_100G;
-	dev_link.link_duplex = ((ecmd->duplex == DUPLEX_HALF) ?
+	dev_link->link_duplex = ((ecmd->duplex == DUPLEX_HALF) ?
 				ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
-	dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
-				  ETH_LINK_SPEED_FIXED);
-	if (memcmp(&dev_link, &dev->data->dev_link, sizeof(dev_link))) {
-		/* Link status changed. */
-		dev->data->dev_link = dev_link;
-		return 0;
-	}
-	/* Link status is still the same. */
-	return -1;
+	return 0;
 }
 
 /**

@@ -917,15 +880,36 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
 int
 mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused)
 {
-	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);
-	return mlx5_link_update_unlocked_gs(dev);
+	struct priv *priv = mlx5_get_priv(dev);
+	struct rte_eth_link dev_link;
+	struct ifreq ifr;
+	int ret;
+	
+	if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
+		WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
+		return -1;
+	}
+	memset(&dev_link, 0, sizeof(dev_link));
+	dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
+				(ifr.ifr_flags & IFF_RUNNING));
+
+	dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
+			ETH_LINK_SPEED_FIXED);
+
+	ret = mlx5_link_update_unlocked_gs(priv, &ifr, &dev_link);
+	if (ret)
+		ret = mlx5_link_update_unlocked_gset(priv, &ifr, &dev_link);
+

Trying to eliminate all runtime calls to look at kernel version to determine API because they are source of portablity problems in distributions.

Stephen Hemminger (2):
mlx5: don't pass unused argument to sub-functions
mlx5: don't depend on kernel version

 drivers/net/mlx5/mlx5_ethdev.c | 118 +++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 70 deletions(-)

Signature · 1 line

2.15.1

Quoted 4 lines · Stephen Hemminger

Trying to eliminate all runtime calls to look at kernel version to determine API because they are source of portablity problems in distributions.

Quoted 9 lines

Stephen Hemminger (2):
mlx5: don't pass unused argument to sub-functions
mlx5: don't depend on kernel version

 drivers/net/mlx5/mlx5_ethdev.c | 118 +++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 70 deletions(-)

Signature · 1 line

2.15.1  

Hi Stephen,

Thinking about one point, this PMD is able to work on current distribution releases using large version of kernels. This code was handling two points:

1. Compiling against a kernel before v4.5 is possible. As this situation is supported by MLNX_OFED it should remain.

2. Between v4.5 up to v4.9 the new link status API is buggee causing a wrong status. This was the main reason why the kernel version was verified.

Quoted 24 lines · On Wed, 3 Jan 2018 11:25:53 +0100 Nelio Laranjeiro…

Quoted 4 lines · Stephen Hemminger

Trying to eliminate all runtime calls to look at kernel version to determine API because they are source of portablity problems in distributions.

Quoted 9 lines

Stephen Hemminger (2):
mlx5: don't pass unused argument to sub-functions
mlx5: don't depend on kernel version

 drivers/net/mlx5/mlx5_ethdev.c | 118 +++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 70 deletions(-)

Signature · 1 line

2.15.1  

Hi Stephen,

Thinking about one point, this PMD is able to work on current distribution releases using large version of kernels. This code was handling two points:

1. Compiling against a kernel before v4.5 is possible. As this situation is supported by MLNX_OFED it should remain.

Quoted 3 lines

2. Between v4.5 up to v4.9 the new link status API is buggee causing a wrong status. This was the main reason why the kernel version was verified.

The problem is that enterprise distributions backport without changing kernel version.