[dpdk-dev] [PATCH 0/2] mlx5: remove dependency on kernel version

[dpdk-dev] [PATCH 0/2] mlx5: remove dependency on kernel version

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

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(-)

-- 
2.15.1

[dpdk-dev] [PATCH 1/2] mlx5: don't pass unused argument to sub-functions

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

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/mlx5/mlx5_ethdev.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index a3cef6891d03..388507f109f7 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -755,11 +755,9 @@ mlx5_dev_supported_ptypes_get(struct rte_eth_dev *dev)
  *
  * @param dev
  *   Pointer to Ethernet device structure.
- * @param wait_to_complete
- *   Wait for request completion (ignored).
  */
 static int
-mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev, int wait_to_complete)
+mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
 {
 	struct priv *priv = mlx5_get_priv(dev);
 	struct ethtool_cmd edata = {
@@ -771,7 +769,6 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev, int wait_to_complete)
 
 	/* priv_lock() is not taken to allow concurrent calls. */
 
-	(void)wait_to_complete;
 	if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
 		WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
 		return -1;
@@ -821,11 +818,9 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev, int wait_to_complete)
  *
  * @param dev
  *   Pointer to Ethernet device structure.
- * @param wait_to_complete
- *   Wait for request completion (ignored).
  */
 static int
-mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete)
+mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
 {
 	struct priv *priv = mlx5_get_priv(dev);
 	struct ethtool_link_settings gcmd = { .cmd = ETHTOOL_GLINKSETTINGS };
@@ -833,7 +828,6 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete)
 	struct rte_eth_link dev_link;
 	uint64_t sc;
 
-	(void)wait_to_complete;
 	if (priv_ifreq(priv, SIOCGIFFLAGS, &ifr)) {
 		WARN("ioctl(SIOCGIFFLAGS) failed: %s", strerror(errno));
 		return -1;
@@ -921,7 +915,7 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev, int wait_to_complete)
  *   Wait for request completion (ignored).
  */
 int
-mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete)
+mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused)
 {
 	struct utsname utsname;
 	int ver[3];
@@ -930,8 +924,8 @@ mlx5_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 	    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);
+		return mlx5_link_update_unlocked_gset(dev);
+	return mlx5_link_update_unlocked_gs(dev);
 }
 
 /**
-- 
2.15.1

[dpdk-dev] [PATCH 2/2] mlx5: don't depend on kernel version

[Stephen Hemminger]

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 <stephen@networkplumber.org>
---
 drivers/net/mlx5/mlx5_ethdev.c | 110 ++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 63 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 388507f109f7..2dc32cdf58b9 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -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>
@@ -757,36 +756,25 @@ mlx5_dev_supported_ptypes_get(struct rte_eth_dev *dev)
  *   Pointer to Ethernet device structure.
  */
 static int
-mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
+mlx5_link_update_unlocked_gset(struct priv *priv, struct ifreq *ifr,
+			       struct rte_eth_link *dev_link)
 {
-	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;
 }
 
 /**
@@ -820,23 +800,14 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
  *   Pointer to Ethernet device structure.
  */
 static int
-mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
+mlx5_link_update_unlocked_gs(struct priv *priv, struct ifreq *ifr,
+			     struct rte_eth_link *dev_link)
 {
-	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);
+
+	if (ret)
+		return ret;
+
+	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;
 }
 
 /**
-- 
2.15.1

Re: [dpdk-dev] [PATCH 2/2] mlx5: don't depend on kernel version

[Nelio Laranjeiro]

Hi Stephen,

Please see few comments bellow,

On Tue, Jan 02, 2018 at 12:53:10PM -0800, Stephen Hemminger wrote:
> 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 <stephen@networkplumber.org>
> ---
>  drivers/net/mlx5/mlx5_ethdev.c | 110 ++++++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 388507f109f7..2dc32cdf58b9 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -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>
> @@ -757,36 +756,25 @@ mlx5_dev_supported_ptypes_get(struct rte_eth_dev *dev)
>   *   Pointer to Ethernet device structure.
>   */
>  static int
> -mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
> +mlx5_link_update_unlocked_gset(struct priv *priv, struct ifreq *ifr,
> +			       struct rte_eth_link *dev_link)

Function documentation should also describe the new parameter.

>  {
> -	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;
>  }
>  
>  /**
> @@ -820,23 +800,14 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev)
>   *   Pointer to Ethernet device structure.
>   */
>  static int
> -mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev)
> +mlx5_link_update_unlocked_gs(struct priv *priv, struct ifreq *ifr,
> +			     struct rte_eth_link *dev_link)

Same here.

>  {
> -	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);
> +
> +	if (ret)
> +		return ret;
> +
> +	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;

This function does not respect the PMD coding style, please remove the
blank lines between the code.

Thanks,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH 1/2] mlx5: don't pass unused argument to sub-functions

[Nelio Laranjeiro]

Hi Stephen,

On Tue, Jan 02, 2018 at 12:53:09PM -0800, Stephen Hemminger wrote:
> 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,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH 0/2] mlx5: remove dependency on kernel version

[Nelio Laranjeiro]

On Tue, Jan 02, 2018 at 12:53:08PM -0800, Stephen Hemminger wrote:
> 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(-)
> 
> -- 
> 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.

This series breaks those two requirements,

Thanks,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH 0/2] mlx5: remove dependency on kernel version

[Stephen Hemminger]

On Wed, 3 Jan 2018 11:25:53 +0100
Nelio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:

> On Tue, Jan 02, 2018 at 12:53:08PM -0800, Stephen Hemminger wrote:
> > 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(-)
> > 
> > -- 
> > 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.

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

Re: [dpdk-dev] [PATCH 1/2] mlx5: don't pass unused argument to sub-functions

[Stephen Hemminger]

On Wed, 3 Jan 2018 08:35:23 +0100
Nelio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:

> Hi Stephen,
> 
> On Tue, Jan 02, 2018 at 12:53:09PM -0800, Stephen Hemminger wrote:
> > 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,
> 

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

Re: [dpdk-dev] [PATCH 0/2] mlx5: remove dependency on kernel version

[Nelio Laranjeiro]

On Wed, Jan 03, 2018 at 07:21:11AM -0800, Stephen Hemminger wrote:
> On Wed, 3 Jan 2018 11:25:53 +0100
> Nelio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> 
> > On Tue, Jan 02, 2018 at 12:53:08PM -0800, Stephen Hemminger wrote:
> > > 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(-)
> > > 
> > > -- 
> > > 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.
> 
> The problem is that enterprise distributions backport without
> changing kernel version.

We don't have any guarantee that all patches have been backported,
that's the main reason why we use the kernel version.

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH 1/2] mlx5: don't pass unused argument to sub-functions

[Nelio Laranjeiro]

On Wed, Jan 03, 2018 at 07:21:58AM -0800, Stephen Hemminger wrote:
> On Wed, 3 Jan 2018 08:35:23 +0100
> Nelio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> 
> > Hi Stephen,
> > 
> > On Tue, Jan 02, 2018 at 12:53:09PM -0800, Stephen Hemminger wrote:
> > > 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,
> > 
> 
> There is a standard in DPDK using RTE macros. The whole driver should
> follow that rather than trying to be different.

The standard was introduced after the driver have been submitted, I
agree that a new way is present, but when it was introduced a
modification on the whole code though DPDK should have been done to
avoid mixed ways.

I prefer to have a single commit replacing the (void)foo by
__rte_unused, it will also help the backports to the stable branches.

Thanks,

-- 
Nélio Laranjeiro
6WIND