Re: [PATCH v2] virtio_pmem: Check device status before requesting flush

[Ira Weiny <ira.weiny@intel.com>]

Philip Chen wrote:
> Hi,
> 
> On Tue, Aug 20, 2024 at 1:01 PM Dave Jiang <dave.jiang@intel.com> wrote:
> >
> >
> >
> > On 8/20/24 10:22 AM, Philip Chen wrote:
> > > If a pmem device is in a bad status, the driver side could wait for
> > > host ack forever in virtio_pmem_flush(), causing the system to hang.
> > >
> > > So add a status check in the beginning of virtio_pmem_flush() to return
> > > early if the device is not activated.
> > >
> > > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > > ---
> > >
> > > v2:
> > > - Remove change id from the patch description
> > > - Add more details to the patch description
> > >
> > >  drivers/nvdimm/nd_virtio.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> > > index 35c8fbbba10e..97addba06539 100644
> > > --- a/drivers/nvdimm/nd_virtio.c
> > > +++ b/drivers/nvdimm/nd_virtio.c
> > > @@ -44,6 +44,15 @@ static int virtio_pmem_flush(struct nd_region *nd_region)
> > >       unsigned long flags;
> > >       int err, err1;
> > >
> > > +     /*
> > > +      * Don't bother to submit the request to the device if the device is
> > > +      * not acticated.
> >
> > s/acticated/activated/
> 
> Thanks for the review.
> I'll fix this typo in v3.
> 
> In addition to this typo, does anyone have any other concerns?

I'm not super familiar with the virtio-pmem workings and the needs reset
flag is barely used.

Did you actually experience this hang?  How was this found?  What is the
user visible issue and how critical is it?

Thanks,
Ira

> 
> >
> > > +      */
> > > +     if (vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
> > > +             dev_info(&vdev->dev, "virtio pmem device needs a reset\n");
> > > +             return -EIO;
> > > +     }
> > > +
> > >       might_sleep();
> > >       req_data = kmalloc(sizeof(*req_data), GFP_KERNEL);
> > >       if (!req_data)