diff mbox series

net/virtio: wait device ready in device reset

Message ID 20210823063906.1382544-1-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: wait device ready in device reset |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-abi-testing warning Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-aarch64-compile-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS

Commit Message

Xueming Li Aug. 23, 2021, 6:39 a.m. UTC 
  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(-)

Comments

Andrew Rybchenko Aug. 23, 2021, 9:56 a.m. UTC | #1 
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
>
Xueming Li Aug. 23, 2021, 1:54 p.m. UTC | #2 
> -----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
> >
Andrew Rybchenko Aug. 24, 2021, 3:40 p.m. UTC | #3 
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?
Chenbo Xia Aug. 26, 2021, 7:15 a.m. UTC | #4 
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
Xueming Li Sept. 15, 2021, 9:23 a.m. UTC | #5 
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

>
diff mbox series

Patch

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