Re: [dpdk-dev] [PATCH] net/virtio: wait device ready in device reset

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

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

>