DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
@ 2021-08-23  6:39 Xueming Li
  2021-08-23  9:56 ` Andrew Rybchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Xueming Li @ 2021-08-23  6:39 UTC (permalink / raw)
  Cc: dev, xuemingl, Maxime Coquelin, Chenbo Xia

According to virtio spec, the device MUST reset when 0 is written to
device_status, and present a 0 in device_status once that is done.

This patch adds the missing part of waiting status 0 in reset function.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 drivers/net/virtio/virtio.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
index 7e1e77797f..f003f612d6 100644
--- a/drivers/net/virtio/virtio.c
+++ b/drivers/net/virtio/virtio.c
@@ -3,6 +3,8 @@
  * Copyright(c) 2020 Red Hat, Inc.
  */
 
+#include <unistd.h>
+
 #include "virtio.h"
 
 uint64_t
@@ -39,8 +41,9 @@ void
 virtio_reset(struct virtio_hw *hw)
 {
 	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
-	/* flush status write */
-	VIRTIO_OPS(hw)->get_status(hw);
+	/* Flush status write and wait device ready. */
+	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET)
+		usleep(1000L);
 }
 
 void
-- 
2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
  2021-08-23  6:39 [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset Xueming Li
@ 2021-08-23  9:56 ` Andrew Rybchenko
  2021-08-23 13:54   ` Xueming(Steven) Li
  2021-09-15  9:21 ` [dpdk-dev] [PATCH v1] net/virtio: wait device ready during reset Xueming Li
  2021-09-15 10:12 ` [dpdk-dev] [PATCH v2] " Xueming Li
  2 siblings, 1 reply; 13+ messages in thread
From: Andrew Rybchenko @ 2021-08-23  9:56 UTC (permalink / raw)
  To: Xueming Li; +Cc: dev, Maxime Coquelin, Chenbo Xia

On 8/23/21 9:39 AM, Xueming Li wrote:
> According to virtio spec, the device MUST reset when 0 is written to
> device_status, and present a 0 in device_status once that is done.
> 
> This patch adds the missing part of waiting status 0 in reset function.
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
>  drivers/net/virtio/virtio.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
> index 7e1e77797f..f003f612d6 100644
> --- a/drivers/net/virtio/virtio.c
> +++ b/drivers/net/virtio/virtio.c
> @@ -3,6 +3,8 @@
>   * Copyright(c) 2020 Red Hat, Inc.
>   */
>  
> +#include <unistd.h>
> +
>  #include "virtio.h"
>  
>  uint64_t
> @@ -39,8 +41,9 @@ void
>  virtio_reset(struct virtio_hw *hw)
>  {
>  	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> -	/* flush status write */
> -	VIRTIO_OPS(hw)->get_status(hw);
> +	/* Flush status write and wait device ready. */
> +	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET)
> +		usleep(1000L);

Don't we need a protection against forever loop here?

>  }
>  
>  void
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
  2021-08-23  9:56 ` Andrew Rybchenko
@ 2021-08-23 13:54   ` Xueming(Steven) Li
  2021-08-24 15:40     ` Andrew Rybchenko
  0 siblings, 1 reply; 13+ messages in thread
From: Xueming(Steven) Li @ 2021-08-23 13:54 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: dev, Maxime Coquelin, Chenbo Xia



> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, August 23, 2021 5:57 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Chenbo Xia <chenbo.xia@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
> 
> On 8/23/21 9:39 AM, Xueming Li wrote:
> > According to virtio spec, the device MUST reset when 0 is written to
> > device_status, and present a 0 in device_status once that is done.
> >
> > This patch adds the missing part of waiting status 0 in reset function.
> >
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > ---
> >  drivers/net/virtio/virtio.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
> > index 7e1e77797f..f003f612d6 100644
> > --- a/drivers/net/virtio/virtio.c
> > +++ b/drivers/net/virtio/virtio.c
> > @@ -3,6 +3,8 @@
> >   * Copyright(c) 2020 Red Hat, Inc.
> >   */
> >
> > +#include <unistd.h>
> > +
> >  #include "virtio.h"
> >
> >  uint64_t
> > @@ -39,8 +41,9 @@ void
> >  virtio_reset(struct virtio_hw *hw)
> >  {
> >  	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> > -	/* flush status write */
> > -	VIRTIO_OPS(hw)->get_status(hw);
> > +	/* Flush status write and wait device ready. */
> > +	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET)
> > +		usleep(1000L);
> 
> Don't we need a protection against forever loop here?

Good question, ideally we need, kernel driver function vp_reset() seems to have same issue.
How about leaving an error message before return? 

> 
> >  }
> >
> >  void
> >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
  2021-08-23 13:54   ` Xueming(Steven) Li
@ 2021-08-24 15:40     ` Andrew Rybchenko
  2021-08-26  7:15       ` Xia, Chenbo
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Rybchenko @ 2021-08-24 15:40 UTC (permalink / raw)
  To: Xueming(Steven) Li; +Cc: dev, Maxime Coquelin, Chenbo Xia

On 8/23/21 4:54 PM, Xueming(Steven) Li wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, August 23, 2021 5:57 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>
>> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Chenbo Xia <chenbo.xia@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
>>
>> On 8/23/21 9:39 AM, Xueming Li wrote:
>>> According to virtio spec, the device MUST reset when 0 is written to
>>> device_status, and present a 0 in device_status once that is done.
>>>
>>> This patch adds the missing part of waiting status 0 in reset function.
>>>
>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>> ---
>>>  drivers/net/virtio/virtio.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
>>> index 7e1e77797f..f003f612d6 100644
>>> --- a/drivers/net/virtio/virtio.c
>>> +++ b/drivers/net/virtio/virtio.c
>>> @@ -3,6 +3,8 @@
>>>   * Copyright(c) 2020 Red Hat, Inc.
>>>   */
>>>
>>> +#include <unistd.h>
>>> +
>>>  #include "virtio.h"
>>>
>>>  uint64_t
>>> @@ -39,8 +41,9 @@ void
>>>  virtio_reset(struct virtio_hw *hw)
>>>  {
>>>  	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
>>> -	/* flush status write */
>>> -	VIRTIO_OPS(hw)->get_status(hw);
>>> +	/* Flush status write and wait device ready. */
>>> +	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET)
>>> +		usleep(1000L);
>>
>> Don't we need a protection against forever loop here?
> 
> Good question, ideally we need, kernel driver function vp_reset() seems to have same issue.

Yes, I've seen it.

> How about leaving an error message before return? 

@Maxime, @Chenbo, what do you think?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
  2021-08-24 15:40     ` Andrew Rybchenko
@ 2021-08-26  7:15       ` Xia, Chenbo
  2021-09-15  9:23         ` Xueming(Steven) Li
  0 siblings, 1 reply; 13+ messages in thread
From: Xia, Chenbo @ 2021-08-26  7:15 UTC (permalink / raw)
  To: Andrew Rybchenko, Xueming(Steven) Li; +Cc: dev, Maxime Coquelin

Hi Adrew & Xueming,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, August 24, 2021 11:41 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
> 
> On 8/23/21 4:54 PM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Monday, August 23, 2021 5:57 PM
> >> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> >> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Chenbo Xia
> <chenbo.xia@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device
> reset
> >>
> >> On 8/23/21 9:39 AM, Xueming Li wrote:
> >>> According to virtio spec, the device MUST reset when 0 is written to
> >>> device_status, and present a 0 in device_status once that is done.
> >>>
> >>> This patch adds the missing part of waiting status 0 in reset function.
> >>>
> >>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> >>> ---
> >>>  drivers/net/virtio/virtio.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
> >>> index 7e1e77797f..f003f612d6 100644
> >>> --- a/drivers/net/virtio/virtio.c
> >>> +++ b/drivers/net/virtio/virtio.c
> >>> @@ -3,6 +3,8 @@
> >>>   * Copyright(c) 2020 Red Hat, Inc.
> >>>   */
> >>>
> >>> +#include <unistd.h>
> >>> +
> >>>  #include "virtio.h"
> >>>
> >>>  uint64_t
> >>> @@ -39,8 +41,9 @@ void
> >>>  virtio_reset(struct virtio_hw *hw)
> >>>  {
> >>>  	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> >>> -	/* flush status write */
> >>> -	VIRTIO_OPS(hw)->get_status(hw);
> >>> +	/* Flush status write and wait device ready. */
> >>> +	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET)
> >>> +		usleep(1000L);
> >>
> >> Don't we need a protection against forever loop here?
> >
> > Good question, ideally we need, kernel driver function vp_reset() seems to
> have same issue.
> 
> Yes, I've seen it.
> 
> > How about leaving an error message before return?
> 
> @Maxime, @Chenbo, what do you think?

I would vote for waiting for some time before return rather than forever loop
and error message is needed.

My understanding is for kernel, it's fine to sleep forever as kernel could schedule
it but for DPDK, it will lead to main lcore unable to do other things but sleep
forever. Meanwhile, users will see the app stuck but don't know what's wrong here.

Thanks,
Chenbo

 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v1] net/virtio: wait device ready during reset
  2021-08-23  6:39 [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset Xueming Li
  2021-08-23  9:56 ` Andrew Rybchenko
@ 2021-09-15  9:21 ` Xueming Li
  2021-09-15  9:27   ` Andrew Rybchenko
  2021-09-15 10:12 ` [dpdk-dev] [PATCH v2] " Xueming Li
  2 siblings, 1 reply; 13+ messages in thread
From: Xueming Li @ 2021-09-15  9:21 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Andrew Rybchenko, Maxime Coquelin, Chenbo Xia

According to virtio spec, the device MUST reset when 0 is written to
device_status, and present 0 in device_status once reset is done.

This patch waits status value to be 0 during reset operation, if
timeout in 3 seconds, log and continue.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/virtio/virtio.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
index 7e1e77797f..f865b27b65 100644
--- a/drivers/net/virtio/virtio.c
+++ b/drivers/net/virtio/virtio.c
@@ -3,7 +3,10 @@
  * Copyright(c) 2020 Red Hat, Inc.
  */
 
+#include <unistd.h>
+
 #include "virtio.h"
+#include "virtio_logs.h"
 
 uint64_t
 virtio_negotiate_features(struct virtio_hw *hw, uint64_t host_features)
@@ -38,9 +41,17 @@ virtio_write_dev_config(struct virtio_hw *hw, size_t offset,
 void
 virtio_reset(struct virtio_hw *hw)
 {
+	uint32_t retry = 0;
+
 	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
-	/* flush status write */
-	VIRTIO_OPS(hw)->get_status(hw);
+	/* Flush status write and wait device ready max 3 seconds. */
+	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET) {
+		if (retry++ > 3000) {
+			PMD_INIT_LOG(WARNING, "device reset timeout");
+			break;
+		}
+		usleep(1000L);
+	}
 }
 
 void
-- 
2.33.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
  2021-08-26  7:15       ` Xia, Chenbo
@ 2021-09-15  9:23         ` Xueming(Steven) Li
  0 siblings, 0 replies; 13+ messages in thread
From: Xueming(Steven) Li @ 2021-09-15  9:23 UTC (permalink / raw)
  To: chenbo.xia, andrew.rybchenko; +Cc: maxime.coquelin, dev

On Thu, 2021-08-26 at 07:15 +0000, Xia, Chenbo wrote:
> Hi Adrew & Xueming,
> 
> > -----Original Message-----
> > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Sent: Tuesday, August 24, 2021 11:41 PM
> > To: Xueming(Steven) Li <xuemingl@nvidia.com>
> > Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> > <chenbo.xia@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset
> > 
> > On 8/23/21 4:54 PM, Xueming(Steven) Li wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > > > Sent: Monday, August 23, 2021 5:57 PM
> > > > To: Xueming(Steven) Li <xuemingl@nvidia.com>
> > > > Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Chenbo Xia
> > <chenbo.xia@intel.com>
> > > > Subject: Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device
> > reset
> > > > 
> > > > On 8/23/21 9:39 AM, Xueming Li wrote:
> > > > > According to virtio spec, the device MUST reset when 0 is written to
> > > > > device_status, and present a 0 in device_status once that is done.
> > > > > 
> > > > > This patch adds the missing part of waiting status 0 in reset function.
> > > > > 
> > > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > > ---
> > > > >  drivers/net/virtio/virtio.c | 7 +++++--
> > > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
> > > > > index 7e1e77797f..f003f612d6 100644
> > > > > --- a/drivers/net/virtio/virtio.c
> > > > > +++ b/drivers/net/virtio/virtio.c
> > > > > @@ -3,6 +3,8 @@
> > > > >   * Copyright(c) 2020 Red Hat, Inc.
> > > > >   */
> > > > > 
> > > > > +#include <unistd.h>
> > > > > +
> > > > >  #include "virtio.h"
> > > > > 
> > > > >  uint64_t
> > > > > @@ -39,8 +41,9 @@ void
> > > > >  virtio_reset(struct virtio_hw *hw)
> > > > >  {
> > > > >  	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> > > > > -	/* flush status write */
> > > > > -	VIRTIO_OPS(hw)->get_status(hw);
> > > > > +	/* Flush status write and wait device ready. */
> > > > > +	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET)
> > > > > +		usleep(1000L);
> > > > 
> > > > Don't we need a protection against forever loop here?
> > > 
> > > Good question, ideally we need, kernel driver function vp_reset() seems to
> > have same issue.
> > 
> > Yes, I've seen it.
> > 
> > > How about leaving an error message before return?
> > 
> > @Maxime, @Chenbo, what do you think?
> 
> I would vote for waiting for some time before return rather than forever loop
> and error message is needed.
> 
> My understanding is for kernel, it's fine to sleep forever as kernel could schedule
> it but for DPDK, it will lead to main lcore unable to do other things but sleep
> forever. Meanwhile, users will see the app stuck but don't know what's wrong here.
> 
> Thanks,
> Chenbo
> 

Hi all, thanks for you sugestion, new version posted:
https://mails.dpdk.org/archives/dev/2021-September/219866.html

>  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v1] net/virtio: wait device ready during reset
  2021-09-15  9:21 ` [dpdk-dev] [PATCH v1] net/virtio: wait device ready during reset Xueming Li
@ 2021-09-15  9:27   ` Andrew Rybchenko
  2021-09-15 10:14     ` Xueming(Steven) Li
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Rybchenko @ 2021-09-15  9:27 UTC (permalink / raw)
  To: Xueming Li, dev; +Cc: Maxime Coquelin, Chenbo Xia

On 9/15/21 12:21 PM, Xueming Li wrote:
> According to virtio spec, the device MUST reset when 0 is written to
> device_status, and present 0 in device_status once reset is done.
> 
> This patch waits status value to be 0 during reset operation, if
> timeout in 3 seconds, log and continue.

I have no strong opinion on timeout.

> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>  drivers/net/virtio/virtio.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
> index 7e1e77797f..f865b27b65 100644
> --- a/drivers/net/virtio/virtio.c
> +++ b/drivers/net/virtio/virtio.c
> @@ -3,7 +3,10 @@
>   * Copyright(c) 2020 Red Hat, Inc.
>   */
>  
> +#include <unistd.h>
> +
>  #include "virtio.h"
> +#include "virtio_logs.h"
>  
>  uint64_t
>  virtio_negotiate_features(struct virtio_hw *hw, uint64_t host_features)
> @@ -38,9 +41,17 @@ virtio_write_dev_config(struct virtio_hw *hw, size_t offset,
>  void
>  virtio_reset(struct virtio_hw *hw)
>  {
> +	uint32_t retry = 0;
> +
>  	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> -	/* flush status write */
> -	VIRTIO_OPS(hw)->get_status(hw);
> +	/* Flush status write and wait device ready max 3 seconds. */
> +	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET) {
> +		if (retry++ > 3000) {
> +			PMD_INIT_LOG(WARNING, "device reset timeout");

I think it would be very useful to log ethdev port ID here.

> +			break;
> +		}
> +		usleep(1000L);
> +	}
>  }
>  
>  void
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v2] net/virtio: wait device ready during reset
  2021-08-23  6:39 [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset Xueming Li
  2021-08-23  9:56 ` Andrew Rybchenko
  2021-09-15  9:21 ` [dpdk-dev] [PATCH v1] net/virtio: wait device ready during reset Xueming Li
@ 2021-09-15 10:12 ` Xueming Li
  2021-09-15 10:30   ` Andrew Rybchenko
                     ` (2 more replies)
  2 siblings, 3 replies; 13+ messages in thread
From: Xueming Li @ 2021-09-15 10:12 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Andrew Rybchenko, Maxime Coquelin, Chenbo Xia

According to virtio spec, the device MUST reset when 0 is written to
device_status, and present 0 in device_status once reset is done.

This patch waits status value to be 0 during reset operation, if
timeout in 3 seconds, log and continue.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/virtio/virtio.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
index 7e1e77797f..d9e642f412 100644
--- a/drivers/net/virtio/virtio.c
+++ b/drivers/net/virtio/virtio.c
@@ -3,7 +3,10 @@
  * Copyright(c) 2020 Red Hat, Inc.
  */
 
+#include <unistd.h>
+
 #include "virtio.h"
+#include "virtio_logs.h"
 
 uint64_t
 virtio_negotiate_features(struct virtio_hw *hw, uint64_t host_features)
@@ -38,9 +41,17 @@ virtio_write_dev_config(struct virtio_hw *hw, size_t offset,
 void
 virtio_reset(struct virtio_hw *hw)
 {
+	uint32_t retry = 0;
+
 	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
-	/* flush status write */
-	VIRTIO_OPS(hw)->get_status(hw);
+	/* Flush status write and wait device ready max 3 seconds. */
+	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET) {
+		if (retry++ > 3000) {
+			PMD_INIT_LOG(WARNING, "port %u device reset timeout", hw->port_id);
+			break;
+		}
+		usleep(1000L);
+	}
 }
 
 void
-- 
2.33.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v1] net/virtio: wait device ready during reset
  2021-09-15  9:27   ` Andrew Rybchenko
@ 2021-09-15 10:14     ` Xueming(Steven) Li
  0 siblings, 0 replies; 13+ messages in thread
From: Xueming(Steven) Li @ 2021-09-15 10:14 UTC (permalink / raw)
  To: andrew.rybchenko, dev; +Cc: chenbo.xia, maxime.coquelin

On Wed, 2021-09-15 at 12:27 +0300, Andrew Rybchenko wrote:
> On 9/15/21 12:21 PM, Xueming Li wrote:
> > According to virtio spec, the device MUST reset when 0 is written to
> > device_status, and present 0 in device_status once reset is done.
> > 
> > This patch waits status value to be 0 during reset operation, if
> > timeout in 3 seconds, log and continue.
> 
> I have no strong opinion on timeout.
> 
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > ---
> >  drivers/net/virtio/virtio.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
> > index 7e1e77797f..f865b27b65 100644
> > --- a/drivers/net/virtio/virtio.c
> > +++ b/drivers/net/virtio/virtio.c
> > @@ -3,7 +3,10 @@
> >   * Copyright(c) 2020 Red Hat, Inc.
> >   */
> >  
> > +#include <unistd.h>
> > +
> >  #include "virtio.h"
> > +#include "virtio_logs.h"
> >  
> >  uint64_t
> >  virtio_negotiate_features(struct virtio_hw *hw, uint64_t host_features)
> > @@ -38,9 +41,17 @@ virtio_write_dev_config(struct virtio_hw *hw, size_t offset,
> >  void
> >  virtio_reset(struct virtio_hw *hw)
> >  {
> > +	uint32_t retry = 0;
> > +
> >  	VIRTIO_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> > -	/* flush status write */
> > -	VIRTIO_OPS(hw)->get_status(hw);
> > +	/* Flush status write and wait device ready max 3 seconds. */
> > +	while (VIRTIO_OPS(hw)->get_status(hw) != VIRTIO_CONFIG_STATUS_RESET) {
> > +		if (retry++ > 3000) {
> > +			PMD_INIT_LOG(WARNING, "device reset timeout");
> 
> I think it would be very useful to log ethdev port ID here.

Good suggesiton, v2 sent.

> 
> > +			break;
> > +		}
> > +		usleep(1000L);
> > +	}
> >  }
> >  
> >  void
> > 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: wait device ready during reset
  2021-09-15 10:12 ` [dpdk-dev] [PATCH v2] " Xueming Li
@ 2021-09-15 10:30   ` Andrew Rybchenko
  2021-09-15 11:44   ` Xia, Chenbo
  2021-09-28 15:31   ` Maxime Coquelin
  2 siblings, 0 replies; 13+ messages in thread
From: Andrew Rybchenko @ 2021-09-15 10:30 UTC (permalink / raw)
  To: Xueming Li, dev; +Cc: Maxime Coquelin, Chenbo Xia

On 9/15/21 1:12 PM, Xueming Li wrote:
> According to virtio spec, the device MUST reset when 0 is written to
> device_status, and present 0 in device_status once reset is done.
> 
> This patch waits status value to be 0 during reset operation, if
> timeout in 3 seconds, log and continue.
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: wait device ready during reset
  2021-09-15 10:12 ` [dpdk-dev] [PATCH v2] " Xueming Li
  2021-09-15 10:30   ` Andrew Rybchenko
@ 2021-09-15 11:44   ` Xia, Chenbo
  2021-09-28 15:31   ` Maxime Coquelin
  2 siblings, 0 replies; 13+ messages in thread
From: Xia, Chenbo @ 2021-09-15 11:44 UTC (permalink / raw)
  To: Xueming Li, dev; +Cc: Andrew Rybchenko, Maxime Coquelin

> -----Original Message-----
> From: Xueming Li <xuemingl@nvidia.com>
> Sent: Wednesday, September 15, 2021 6:12 PM
> To: dev@dpdk.org
> Cc: xuemingl@nvidia.com; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>;
> Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Subject: [PATCH v2] net/virtio: wait device ready during reset
> 
> According to virtio spec, the device MUST reset when 0 is written to
> device_status, and present 0 in device_status once reset is done.
> 
> This patch waits status value to be 0 during reset operation, if
> timeout in 3 seconds, log and continue.
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> --
> 2.33.0

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: wait device ready during reset
  2021-09-15 10:12 ` [dpdk-dev] [PATCH v2] " Xueming Li
  2021-09-15 10:30   ` Andrew Rybchenko
  2021-09-15 11:44   ` Xia, Chenbo
@ 2021-09-28 15:31   ` Maxime Coquelin
  2 siblings, 0 replies; 13+ messages in thread
From: Maxime Coquelin @ 2021-09-28 15:31 UTC (permalink / raw)
  To: Xueming Li, dev; +Cc: Andrew Rybchenko, Chenbo Xia



On 9/15/21 12:12, Xueming Li wrote:
> According to virtio spec, the device MUST reset when 0 is written to
> device_status, and present 0 in device_status once reset is done.
> 
> This patch waits status value to be 0 during reset operation, if
> timeout in 3 seconds, log and continue.
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>   drivers/net/virtio/virtio.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-09-28 15:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23  6:39 [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset Xueming Li
2021-08-23  9:56 ` Andrew Rybchenko
2021-08-23 13:54   ` Xueming(Steven) Li
2021-08-24 15:40     ` Andrew Rybchenko
2021-08-26  7:15       ` Xia, Chenbo
2021-09-15  9:23         ` Xueming(Steven) Li
2021-09-15  9:21 ` [dpdk-dev] [PATCH v1] net/virtio: wait device ready during reset Xueming Li
2021-09-15  9:27   ` Andrew Rybchenko
2021-09-15 10:14     ` Xueming(Steven) Li
2021-09-15 10:12 ` [dpdk-dev] [PATCH v2] " Xueming Li
2021-09-15 10:30   ` Andrew Rybchenko
2021-09-15 11:44   ` Xia, Chenbo
2021-09-28 15:31   ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).