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

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

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

Signed-off-by: Benoît Ganne <bganne@cisco.com>
---

Following the discussion on dpdk-users [1], I am submitting a tentative
patch.
[1] http://mails.dpdk.org/archives/users/2020-March/thread.html#4744

 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

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

[Matan Azrad]

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

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

[Benoit Ganne (bganne)]

> From: Matan Azrad <matan@mellanox.com>

>> 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?

We are using the no wait option.

>> 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 do not think it is documented and I suppose at the end of the day it depends from the HW and configurations...
My commit message is misleading, my apologizes. What I meant was to let the app decide whether it should retry or not, based on the data it gets.
Right now, the PMD *prevents* the app to get link state if the link speed is undefined even if the app does not care about link speed. It makes an assumption on behalf of the apps. Moreover, it seems to be one of the few PMDs to make this assumption: at least the mlx4 PMD and the i40e PMD does not seem to make this assumption.
This is a problem for our app (fd.io VPP) but I was also told about other apps having the same issue too.

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

I hope to get feedback 😊 it is precisely the goal of this.

ben

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

[Matan Azrad]

Hi

From: Benoit Ganne (bganne) <bganne@cisco.com>
> > From: Matan Azrad <matan@mellanox.com>
> 
> >> 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?
> 
> We are using the no wait option.

I suggest to call again if failed for N retries time.

> >> 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 do not think it is documented and I suppose at the end of the day it
> depends from the HW and configurations...
> My commit message is misleading, my apologizes. What I meant was to let
> the app decide whether it should retry or not, based on the data it gets.
> Right now, the PMD *prevents* the app to get link state if the link speed is
> undefined even if the app does not care about link speed.

In mlx5 this is not the case, we have no one updated and second not - there are going together:

You can see that we have 2 different system calls: 1 to get up\down and second to get link speed.
If link speed doesn't appropriate to the link state it may say that something was changed between the calls and the link status we got from the first call is not correct anymore.

In this case, we should call both calls again, that’s what we are doing in "nowait" option.
If the user doesn't want "nowait" option, (means PMD is not allowed to take more time for response) he should call again when the callback failed in the time and retries manner the user prefers.

> It makes an
> assumption on behalf of the apps. Moreover, it seems to be one of the few
> PMDs to make this assumption: at least the mlx4 PMD and the i40e PMD
> does not seem to make this assumption.
> This is a problem for our app (fd.io VPP) but I was also told about other apps
> having the same issue too.
> 
> > I think, at least you should update ethdev relevant API descriptions
> > and get agreement from more PMD maintainers...
> 
> I hope to get feedback 😊 it is precisely the goal of this.
> 
> ben

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

[Benoit Ganne (bganne)]

Hi Matan,

>>>> 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?
>> We are using the no wait option.
> I suggest to call again if failed for N retries time.

Unfortunately it will not solve our problem: if link speed is undefined but the link is up then the test '!dev_link.link_speed && dev_link.link_status' at http://git.dpdk.org/dpdk/tree/drivers/net/mlx5/mlx5_ethdev.c#n899 will always be true and the function will always return EAGAIN.
This actually happens in Azure with CX4-Lx VFs.

>> What I meant was to let the app decide whether it should retry or not,
>> based on the data it gets.
>> Right now, the PMD *prevents* the app to get link state if the link
>> speed is undefined even if the app does not care about link speed.

> In mlx5 this is not the case, we have no one updated and second not -
> there are going together:
> You can see that we have 2 different system calls: 1 to get up\down and
> second to get link speed.
> If link speed doesn't appropriate to the link state it may say that
> something was changed between the calls and the link status we got from
> the first call is not correct anymore.
> In this case, we should call both calls again, that’s what we are doing in
> "nowait" option.
> If the user doesn't want "nowait" option, (means PMD is not allowed to
> take more time for response) he should call again when the callback failed
> in the time and retries manner the user prefers.

Ok, now I understand the logic behind the current behavior: the 2 syscalls being not atomics, you try to detect inconsistencies that way.
But if the link speed is undefined, then the state will never be correctly updated.
I still believe it is unnecessarily heavy-handed: in most networking application I have seen (and I have 2 examples of current shipping networking products), a missing link speed is not critical whereas link being reported as down means no traffic flowing.

Best
ben

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

[Matan Azrad]

Hi
 From: Benoit Ganne 
> Sent: Monday, March 30, 2020 3:03 PM
> To: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/mlx5: fix link state update
> 
> Hi Matan,
> 
> >>>> 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?
> >> We are using the no wait option.
> > I suggest to call again if failed for N retries time.
> 
> Unfortunately it will not solve our problem: if link speed is undefined but the
> link is up then the test '!dev_link.link_speed && dev_link.link_status' at
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.dp
> dk.org%2Fdpdk%2Ftree%2Fdrivers%2Fnet%2Fmlx5%2Fmlx5_ethdev.c%23n8
> 99&amp;data=02%7C01%7Cmatan%40mellanox.com%7C8a0fb9f9ba94422a4a
> 4208d7d4a2539c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637
> 211665962283891&amp;sdata=%2FExOJyhm0Bi4kkp434P0U4kRpQs7U0knuTa
> kvsWFO5Q%3D&amp;reserved=0 will always be true and the function will
> always return EAGAIN.
> This actually happens in Azure with CX4-Lx VFs.
> 
> >> What I meant was to let the app decide whether it should retry or
> >> not, based on the data it gets.
> >> Right now, the PMD *prevents* the app to get link state if the link
> >> speed is undefined even if the app does not care about link speed.
> 
> > In mlx5 this is not the case, we have no one updated and second not -
> > there are going together:
> > You can see that we have 2 different system calls: 1 to get up\down
> > and second to get link speed.
> > If link speed doesn't appropriate to the link state it may say that
> > something was changed between the calls and the link status we got
> > from the first call is not correct anymore.
> > In this case, we should call both calls again, that’s what we are
> > doing in "nowait" option.
> > If the user doesn't want "nowait" option, (means PMD is not allowed to
> > take more time for response) he should call again when the callback
> > failed in the time and retries manner the user prefers.
> 
> Ok, now I understand the logic behind the current behavior: the 2 syscalls
> being not atomics, you try to detect inconsistencies that way.
> But if the link speed is undefined, then the state will never be correctly
> updated.

Why link speed is undefined?
Old kernel?
Kernel mlx5 driver issue?

Do you know?

> I still believe it is unnecessarily heavy-handed: in most networking application
> I have seen (and I have 2 examples of current shipping networking products),
> a missing link speed is not critical whereas link being reported as down means
> no traffic flowing.
> 
> Best
> ben

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

[Benoit Ganne (bganne)]

> Why link speed is undefined?
> Old kernel?
> Kernel mlx5 driver issue?
> Do you know?

Seems to be a bug in kernel mlx5 driver: http://mails.dpdk.org/archives/users/2020-March/004758.html
I still think this test is too strict, and from what I can see mlx4 PMD do not implement it (even if it uses 2 syscalls too).

ben

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

[Matan Azrad]

Hi

From: Benoit Ganne (bganne)
> > Why link speed is undefined?
> > Old kernel?
> > Kernel mlx5 driver issue?
> > Do you know?
> 
> Seems to be a bug in kernel mlx5 driver:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.
> dpdk.org%2Farchives%2Fusers%2F2020-
> March%2F004758.html&data=02%7C01%7Cmatan%40mellanox.com%7C
> 69ce3bfd96484267869908d7d4b1ca89%7Ca652971c7d2e4d9ba6a4d149256f46
> 1b%7C0%7C0%7C637211732388079395&sdata=%2BnfjST6fM4zoIYwJz7L
> Z2zL3rGmmOFXbpW0fK7Weps4%3D&reserved=0
> I still think this test is too strict, and from what I can see mlx4 PMD do not
> implement it (even if it uses 2 syscalls too).
Yes, Doesn't mean this is not a bug in mlx4😊


Thanks Ben.

So we have 2 orthogonal efforts here:

1. To check with our driver team what is the issue to report a VF link speed : For me 😊
2. To change documentation of ethdev API to say that only link status is mandatory report. (all others "nice to have" or "best effort"):
	This is for you - I suggest to send prior patch to ethdev for this and let the community to decide.

Are you agree?


> ben

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

[Benoit Ganne (bganne)]

Hi Matan,

>> Seems to be a bug in kernel mlx5 driver:
> 1. To check with our driver team what is the issue to report a VF link

Note that even with the kernel patch, there still seem to be a valid case where it can return SPEED_UNKNOWN.
If this is true, then I don't think we can rely on valid link speed to detect whether the info are correct.

ben

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

[Matan Azrad]

Hi

From: Benoit Ganne
> Hi Matan,
> 
> >> Seems to be a bug in kernel mlx5 driver:
> > 1. To check with our driver team what is the issue to report a VF link

Yes, driver team said the 3 kernel patches should solve the issue.

> Note that even with the kernel patch, there still seem to be a valid case
> where it can return SPEED_UNKNOWN.
> If this is true, then I don't think we can rely on valid link speed to detect
> whether the info are correct.

As I said I'm not against this approach, but need to define the API well.
 
> ben

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

[Thomas Monjalon]

30/03/2020 18:13, Matan Azrad:
> 2. To change documentation of ethdev API to say that only link status
> is mandatory report. (all others "nice to have" or "best effort"):
> 	This is for you - I suggest to send prior patch to ethdev
> 	for this and let the community to decide.

Matan, Benoit, I guess nobody tried to update ethdev API doc yet?
I can do it if you prefer.

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

[Benoit Ganne (bganne)]

>> 2. To change documentation of ethdev API to say that only link status
>> is mandatory report. (all others "nice to have" or "best effort"):
>> 	This is for you - I suggest to send prior patch to ethdev
>> 	for this and let the community to decide.
> Matan, Benoit, I guess nobody tried to update ethdev API doc yet?
> I can do it if you prefer.

My understanding was that this [1] was basically saying 'returning UNKNOWN is ok'. But if this is not what you had in mind, I'll be glad you can take care of it 😊

[1] http://mails.dpdk.org/archives/dev/2020-April/thread.html#161538

Best
ben

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

[Matan Azrad]

+@Asaf Penso

From: Benoit Ganne 
> >> 2. To change documentation of ethdev API to say that only link status
> >> is mandatory report. (all others "nice to have" or "best effort"):
> >> 	This is for you - I suggest to send prior patch to ethdev
> >> 	for this and let the community to decide.
> > Matan, Benoit, I guess nobody tried to update ethdev API doc yet?
> > I can do it if you prefer.
> 
> My understanding was that this [1] was basically saying 'returning UNKNOWN
> is ok'. But if this is not what you had in mind, I'll be glad you can take care of it
> 😊
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.dpdk
> .org%2Farchives%2Fdev%2F2020-
> April%2Fthread.html%23161538&data=02%7C01%7Cmatan%40mellanox.c
> om%7Cee2c8eebc00d4ea3106b08d7daf97473%7Ca652971c7d2e4d9ba6a4d149
> 256f461b%7C0%7C0%7C637218637252717747&sdata=JSFaHkmSzE1tWB7
> DqEaqVI5B1pIZuIdDrfw0SFyUE8s%3D&reserved=0
> 
> Best
> ben


Hi
Now, when the API is updated that the speed value is best effort using ETH_SPEED_NUM_UNKNOWN new value,

You can go for your approach:
In case the link is up and speed is invalid by kernel we can return link up and ETH_SPEED_NUM_UNKNOWN.

In case applications wants a valid speed they should call the API again.
Need to update it in mlx5 release notes section.

The original patch should be rebased and updated.

Matan

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

[Raslan Darawsheh]

From: Benoît Ganne <bganne@cisco.com>

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

Signed-off-by: Benoît Ganne <bganne@cisco.com>
Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
---
 doc/guides/rel_notes/release_20_11.rst  |  1 +
 drivers/net/mlx5/linux/mlx5_ethdev_os.c | 10 ----------
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 1c262d39a5..75b4ebc84b 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -207,6 +207,7 @@ New Features
     by rte_flow API.
   * Added support of Age action query.
   * Added support of multi-ports hairpin.
+  * Allow unknown link speed.
 
   Updated Mellanox mlx5 vDPA driver:
 
diff --git a/drivers/net/mlx5/linux/mlx5_ethdev_os.c b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
index 19b281925f..f7ef1492b1 100644
--- a/drivers/net/mlx5/linux/mlx5_ethdev_os.c
+++ b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
@@ -425,11 +425,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;
 }
@@ -571,11 +566,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

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

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Raslan Darawsheh
> Sent: Thursday, November 19, 2020 4:20 PM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Benoît Ganne <bganne@cisco.com>
> Subject: [dpdk-dev] [PATCH v2] net/mlx5: fix link state update
> 
> From: Benoît Ganne <bganne@cisco.com>
> 
> 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.
> 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.
> 
> Signed-off-by: Benoît Ganne <bganne@cisco.com>
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>

Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh

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

[Ferruh Yigit]

On 11/19/2020 2:20 PM, Raslan Darawsheh wrote:
> From: Benoît Ganne <bganne@cisco.com>
> 
> 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.
> 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.
> 

Hi Raslan, Matan,

Can you please provide the Fixes tag? Also should this fix backported?

> Signed-off-by: Benoît Ganne <bganne@cisco.com>
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>

<...>

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

[Thomas Monjalon]

19/11/2020 18:48, Ferruh Yigit:
> On 11/19/2020 2:20 PM, Raslan Darawsheh wrote:
> > From: Benoît Ganne <bganne@cisco.com>
> > 
> > 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.
> > 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.
> > 
> 
> Hi Raslan, Matan,
> 
> Can you please provide the Fixes tag? Also should this fix backported?

I think it should not be backported because it is a behaviour change
relying on API doc change.

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

[Ferruh Yigit]

On 11/19/2020 6:42 PM, Thomas Monjalon wrote:
> 19/11/2020 18:48, Ferruh Yigit:
>> On 11/19/2020 2:20 PM, Raslan Darawsheh wrote:
>>> From: Benoît Ganne <bganne@cisco.com>
>>>
>>> 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.
>>> 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.
>>>
>>
>> Hi Raslan, Matan,
>>
>> Can you please provide the Fixes tag? Also should this fix backported?
> 
> I think it should not be backported because it is a behaviour change
> relying on API doc change.
> 

As far as I understand, API change you mention is making 'ETH_SPEED_NUM_UNKNOWN' 
speed value a valid value return value.

Commit log doesn't give the context that it relies on an ethdev behavior change,
it sounds like it is fixing behavior in the driver, it is not possible to figure 
out the relation without digging the mail list discussions.
It would be nice to update the commit log to give more details, I think problem 
is clear but can good to add why the check was there at first place and why it 
can be removed now, etc..

Btw with LTS, with the faulty kernel driver, PMD still won't able to get link 
speed which will prevent returning link status.
Or is the kernel driver fixed and no need to worry about this anymore?

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

[Thomas Monjalon]

20/11/2020 11:51, Ferruh Yigit:
> On 11/19/2020 6:42 PM, Thomas Monjalon wrote:
> > 19/11/2020 18:48, Ferruh Yigit:
> >> On 11/19/2020 2:20 PM, Raslan Darawsheh wrote:
> >>> From: Benoît Ganne <bganne@cisco.com>
> >>>
> >>> 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.
> >>> 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.
> >>>
> >>
> >> Hi Raslan, Matan,
> >>
> >> Can you please provide the Fixes tag? Also should this fix backported?
> > 
> > I think it should not be backported because it is a behaviour change
> > relying on API doc change.
> > 
> 
> As far as I understand, API change you mention is making 'ETH_SPEED_NUM_UNKNOWN' 
> speed value a valid value return value.
> 
> Commit log doesn't give the context that it relies on an ethdev behavior change,
> it sounds like it is fixing behavior in the driver, it is not possible to figure 
> out the relation without digging the mail list discussions.
> It would be nice to update the commit log to give more details, I think problem 
> is clear but can good to add why the check was there at first place and why it 
> can be removed now, etc..

Yes I can help improving the explanations.

> Btw with LTS, with the faulty kernel driver, PMD still won't able to get link 
> speed which will prevent returning link status.

With this PMD change, the link status is returned even if link speed
is not available.

> Or is the kernel driver fixed and no need to worry about this anymore?

Yes the kernel driver is fixed since a long time.

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

[Ferruh Yigit]

On 11/20/2020 1:50 PM, Thomas Monjalon wrote:
> 20/11/2020 11:51, Ferruh Yigit:
>> On 11/19/2020 6:42 PM, Thomas Monjalon wrote:
>>> 19/11/2020 18:48, Ferruh Yigit:
>>>> On 11/19/2020 2:20 PM, Raslan Darawsheh wrote:
>>>>> From: Benoît Ganne <bganne@cisco.com>
>>>>>
>>>>> 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.
>>>>> 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.
>>>>>
>>>>
>>>> Hi Raslan, Matan,
>>>>
>>>> Can you please provide the Fixes tag? Also should this fix backported?
>>>
>>> I think it should not be backported because it is a behaviour change
>>> relying on API doc change.
>>>
>>
>> As far as I understand, API change you mention is making 'ETH_SPEED_NUM_UNKNOWN'
>> speed value a valid value return value.
>>
>> Commit log doesn't give the context that it relies on an ethdev behavior change,
>> it sounds like it is fixing behavior in the driver, it is not possible to figure
>> out the relation without digging the mail list discussions.
>> It would be nice to update the commit log to give more details, I think problem
>> is clear but can good to add why the check was there at first place and why it
>> can be removed now, etc..
> 
> Yes I can help improving the explanations.
> 
>> Btw with LTS, with the faulty kernel driver, PMD still won't able to get link
>> speed which will prevent returning link status.
> 
> With this PMD change, the link status is returned even if link speed
> is not available.
> 

That is why I am asking if this patch should be backported :)

>> Or is the kernel driver fixed and no need to worry about this anymore?
> 
> Yes the kernel driver is fixed since a long time.
> 
> 
> 

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

[Raslan Darawsheh]

Hi,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, November 20, 2020 4:21 PM
> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
> Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Benoît
> Ganne <bganne@cisco.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/mlx5: fix link state update
> 
> On 11/20/2020 1:50 PM, Thomas Monjalon wrote:
> > 20/11/2020 11:51, Ferruh Yigit:
> >> On 11/19/2020 6:42 PM, Thomas Monjalon wrote:
> >>> 19/11/2020 18:48, Ferruh Yigit:
> >>>> On 11/19/2020 2:20 PM, Raslan Darawsheh wrote:
> >>>>> From: Benoît Ganne <bganne@cisco.com>
> >>>>>
> >>>>> 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.
> >>>>> 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.
> >>>>>
> >>>>
> >>>> Hi Raslan, Matan,
> >>>>
> >>>> Can you please provide the Fixes tag? Also should this fix backported?
> >>>
> >>> I think it should not be backported because it is a behaviour change
> >>> relying on API doc change.
> >>>
> >>
> >> As far as I understand, API change you mention is making
> 'ETH_SPEED_NUM_UNKNOWN'
> >> speed value a valid value return value.
> >>
> >> Commit log doesn't give the context that it relies on an ethdev behavior
> change,
> >> it sounds like it is fixing behavior in the driver, it is not possible to figure
> >> out the relation without digging the mail list discussions.
> >> It would be nice to update the commit log to give more details, I think
> problem
> >> is clear but can good to add why the check was there at first place and
> why it
> >> can be removed now, etc..
> >
> > Yes I can help improving the explanations.
> >
> >> Btw with LTS, with the faulty kernel driver, PMD still won't able to get link
> >> speed which will prevent returning link status.
> >
> > With this PMD change, the link status is returned even if link speed
> > is not available.
> >
> 
> That is why I am asking if this patch should be backported :)
> 
> >> Or is the kernel driver fixed and no need to worry about this anymore?
> >
> > Yes the kernel driver is fixed since a long time.
> >
> >
> >
Thanks for your comments and reviews, 
I'll send a v3 changing the commit log accordingly.

Kindest regards,
Raslan Darawsheh

[dpdk-dev] [PATCH v3] net/mlx5: allow unknown link speed

[Raslan Darawsheh]

From: Benoît Ganne <bganne@cisco.com>

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.

Commit [1] allowed returning unknown link speed, so now pmd allow
the return of unknown link speed in the above case.

Due to some old kernel driver bug, link speed wasn't detected properly.

[1] http://git.dpdk.org/dpdk/commit/?id=810b17d116f03

Signed-off-by: Benoît Ganne <bganne@cisco.co>
Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
v2: rebase the code and add doc update
v3: reword commit log, and return correct link speed
---
 doc/guides/rel_notes/release_20_11.rst  |  1 +
 drivers/net/mlx5/linux/mlx5_ethdev_os.c | 16 +++-------------
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 1c262d39a5..75b4ebc84b 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -207,6 +207,7 @@ New Features
     by rte_flow API.
   * Added support of Age action query.
   * Added support of multi-ports hairpin.
+  * Allow unknown link speed.
 
   Updated Mellanox mlx5 vDPA driver:
 
diff --git a/drivers/net/mlx5/linux/mlx5_ethdev_os.c b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
index 19b281925f..8f9f14156b 100644
--- a/drivers/net/mlx5/linux/mlx5_ethdev_os.c
+++ b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
@@ -405,7 +405,7 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev,
 	}
 	link_speed = ethtool_cmd_speed(&edata);
 	if (link_speed == -1)
-		dev_link.link_speed = ETH_SPEED_NUM_NONE;
+		dev_link.link_speed = ETH_SPEED_NUM_UNKNOWN;
 	else
 		dev_link.link_speed = link_speed;
 	priv->link_speed_capa = 0;
@@ -425,11 +425,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;
 }
@@ -517,8 +512,8 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev,
 			dev->data->port_id, strerror(rte_errno));
 		return ret;
 	}
-	dev_link.link_speed = (ecmd->speed == UINT32_MAX) ? ETH_SPEED_NUM_NONE :
-							    ecmd->speed;
+	dev_link.link_speed = (ecmd->speed == UINT32_MAX) ?
+				ETH_SPEED_NUM_UNKNOWN : ecmd->speed;
 	sc = ecmd->link_mode_masks[0] |
 		((uint64_t)ecmd->link_mode_masks[1] << 32);
 	priv->link_speed_capa = 0;
@@ -571,11 +566,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

Re: [dpdk-dev] [PATCH v3] net/mlx5: allow unknown link speed

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Raslan Darawsheh
> Sent: Sunday, November 22, 2020 12:04 PM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Benoît Ganne <bganne@cisco.com>; Benoît
> Ganne <bganne@cisco.co>
> Subject: [dpdk-dev] [PATCH v3] net/mlx5: allow unknown link speed
> 
> From: Benoît Ganne <bganne@cisco.com>
> 
> 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.
> 
> Commit [1] allowed returning unknown link speed, so now pmd allow
> the return of unknown link speed in the above case.
> 
> Due to some old kernel driver bug, link speed wasn't detected properly.
> 
> [1]
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.dp
> dk.org%2Fdpdk%2Fcommit%2F%3Fid%3D810b17d116f03&amp;data=04%7C0
> 1%7Crasland%40nvidia.com%7C3965964e417641f146d008d88ece1678%7C430
> 83d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637416363079871052%7CUn
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6
> Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=t0Qj8hEjCBYi1bzy25lJAGTr
> bI0C4l6oImqAe174rNA%3D&amp;reserved=0
> 
> Signed-off-by: Benoît Ganne <bganne@cisco.co>
> Signed-off-by: Raslan Darawsheh <rasland@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
> v2: rebase the code and add doc update
> v3: reword commit log, and return correct link speed
> ---
>  doc/guides/rel_notes/release_20_11.rst  |  1 +
>  drivers/net/mlx5/linux/mlx5_ethdev_os.c | 16 +++-------------
>  2 files changed, 4 insertions(+), 13 deletions(-)

Reverted V2 and applied v3 to next-net-mlx,

Kindest regards,
Raslan Darawsheh