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

[Nelio Laranjeiro <nelio.laranjeiro@6wind.com>]

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