Re: [dpdk-dev] [PATCH] net/mlx5: fix link state update

[Matan Azrad <matan@mellanox.com>]

Hi

 From: Benoît Ganne
> mlx5 PMD refuses to update link state if link speed is defined but status is
> down or if link speed is undefined but status is up, even if the ioctl()
> succeeded.
> This prevents application to detect link up/down event, especially when the
> link speed is not correctly detected.

Do you use the wait option? Or no wait?

> As link speed is nice to have whereas link status is mandatory for operations,
> always update link state regardless of link speed. The application can then
> check link speed if needs be.

Is it documented well? I didn't find doc\description says link speed is best effort.

PMD cannot guess whether link speed is mandatory for the user or not....

I think, at least you should update ethdev relevant API descriptions and get agreement from more PMD maintainers... 

 
> Signed-off-by: Benoît Ganne <bganne@cisco.com>
> ---
> 
> Following the discussion on dpdk-users [1], I am submitting a tentative patch.
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.
> dpdk.org%2Farchives%2Fusers%2F2020-
> March%2Fthread.html%234744&amp;data=02%7C01%7Cmatan%40mellanox.
> com%7C2cb881fc068c434e2f6b08d7d273c924%7Ca652971c7d2e4d9ba6a4d14
> 9256f461b%7C0%7C0%7C637209267055735109&amp;sdata=WV%2Fsd%2B%2
> BKssI3d8uxMf8cqackb%2FHrpqRIWOos2BWynU4%3D&amp;reserved=0
> 
>  drivers/net/mlx5/mlx5_ethdev.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c index d7d3bc73c..c15f4d62b 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -896,11 +896,6 @@ mlx5_link_update_unlocked_gset(struct
> rte_eth_dev *dev,
>  				ETH_LINK_HALF_DUPLEX :
> ETH_LINK_FULL_DUPLEX);
>  	dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
>  			ETH_LINK_SPEED_FIXED);
> -	if (((dev_link.link_speed && !dev_link.link_status) ||
> -	     (!dev_link.link_speed && dev_link.link_status))) {
> -		rte_errno = EAGAIN;
> -		return -rte_errno;
> -	}
>  	*link = dev_link;
>  	return 0;
>  }
> @@ -1032,11 +1027,6 @@ mlx5_link_update_unlocked_gs(struct
> rte_eth_dev *dev,
>  				ETH_LINK_HALF_DUPLEX :
> ETH_LINK_FULL_DUPLEX);
>  	dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
>  				  ETH_LINK_SPEED_FIXED);
> -	if (((dev_link.link_speed && !dev_link.link_status) ||
> -	     (!dev_link.link_speed && dev_link.link_status))) {
> -		rte_errno = EAGAIN;
> -		return -rte_errno;
> -	}
>  	*link = dev_link;
>  	return 0;
>  }
> --
> 2.17.1