Re: [PATCH] vp_vdpa: harden the logic of set status

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

在 2022/11/11 23:14, Stefano Garzarella 写道:
> On Fri, Nov 11, 2022 at 10:55:05PM +0800, Longpeng(Mike) wrote:
> > From: Longpeng <longpeng2@xxxxxxxxxx>
> >
> > 1. We should not set status to 0 when invoking vp_vdpa_set_status().
> >
> > 2. The driver MUST wait for a read of device_status to return 0 before
> >   reinitializing the device.
> >
> > Signed-off-by: Longpeng <longpeng2@xxxxxxxxxx>
> > ---
> > drivers/vdpa/virtio_pci/vp_vdpa.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c 
> > b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > index d448db0c4de3..d35fac5cde11 100644
> > --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> > +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> > @@ -212,8 +212,12 @@ static void vp_vdpa_set_status(struct vdpa_device 
> > *vdpa, u8 status)
> > {
> >     struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
> >     struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
> > -    u8 s = vp_vdpa_get_status(vdpa);
>
> Is this change really needed?
No need to get the status if we try to set status to 0 (trigger BUG).

> > +    u8 s;
> > +
> > +    /* We should never be setting status to 0. */
> > +    BUG_ON(status == 0);
>
> IMHO panicking the kernel seems excessive in this case, please use 
> WARN_ON and maybe return earlier.
Um...I referenced the vp_reset/vp_set_status,

> > +    s = vp_vdpa_get_status(vdpa);
> >     if (status & VIRTIO_CONFIG_S_DRIVER_OK &&
> >         !(s & VIRTIO_CONFIG_S_DRIVER_OK)) {
> >         vp_vdpa_request_irq(vp_vdpa);
> > @@ -229,6 +233,11 @@ static int vp_vdpa_reset(struct vdpa_device *vdpa)
> >     u8 s = vp_vdpa_get_status(vdpa);
> >
> >     vp_modern_set_status(mdev, 0);
> > +    /* After writing 0 to device_status, the driver MUST wait for a 
> > read of
> > +     * device_status to return 0 before reinitializing the device.
> > +     */
> > +    while (vp_modern_get_status(mdev))
> > +        msleep(1);
>
> Should we set a limit after which we give up? A malfunctioning device 
> could keep us here forever.
Yes, but the malfunctioning device maybe can not work anymore, how to 
handle it?

> Thanks,
> Stefano
>
> >     if (s & VIRTIO_CONFIG_S_DRIVER_OK)
> >         vp_vdpa_free_irq(vp_vdpa);
> > --
> > 2.23.0
>
> .