Hi Jason Wang Thanks for your review. Yes, you're correct. I will update the patch accordingly to address the issues you pointed out. Here is the link to the V2 version: https://lore.kernel.org/all/20240325110333.249-1-gavin.liu@xxxxxxxxxxxxxxx/ Here is the content of the V2 version: ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- In the vp_vdpa_set_status function, when setting the device status to VIRTIO_CONFIG_S_DRIVER_OK, the vp_vdpa_request_irq function may fail. In such cases, the device status should not be set to DRIVER_OK. Add exception printing to remind the user. Signed-off-by: Yuxue Liu <yuxue.liu@xxxxxxxxxxxxxxx> --- V1 -> V2: Remove redundant printouts V1: https://lore.kernel.org/all/20240315102857.1803-1-gavin.liu@xxxxxxxxxxxxxxx/ --- drivers/vdpa/virtio_pci/vp_vdpa.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c index df5f4a3bccb5..4caca0517cad 100644 --- a/drivers/vdpa/virtio_pci/vp_vdpa.c +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c @@ -216,7 +216,10 @@ static void vp_vdpa_set_status(struct vdpa_device *vdpa, u8 status) if (status & VIRTIO_CONFIG_S_DRIVER_OK && !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { - vp_vdpa_request_irq(vp_vdpa); + if (vp_vdpa_request_irq(vp_vdpa)) { + WARN_ON(1); + return; + } } vp_modern_set_status(mdev, status); -- ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Best regards, Gavin -----Original Message----- From: Jason Wang jasowang@xxxxxxxxxx Sent: March 18, 2024 12:25 To: Gavin Liu gavin.liu@xxxxxxxxxxxxxxx Cc: mst@xxxxxxxxxx; xuanzhuo@xxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxxx; Angus Chen angus.chen@xxxxxxxxxxxxxxx; Gavin Liu gavin.liu@xxxxxxxxxxxxxxx Subject: Re: [PATCH] vp_vdpa: Fix return value check vp_vdpa_request_irq External Mail: This email originated from OUTSIDE of the organization! Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe. On Fri, Mar 15, 2024 at 6:29 PM gavin.liu <gavin.liu@xxxxxxxxxxxxxxx> wrote: > > From: Yuxue Liu <yuxue.liu@xxxxxxxxxxxxxxx> > > In the vp_vdpa_set_status function, when setting the device status to > VIRTIO_CONFIG_S_DRIVER_OK, the vp_vdpa_request_irq function may fail. > In such cases, the device status should not be set to DRIVER_OK. Add > exception printing to remind the user. > > Signed-off-by: Yuxue Liu <yuxue.liu@xxxxxxxxxxxxxxx> > --- > drivers/vdpa/virtio_pci/vp_vdpa.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c > index 281287fae89f..c1270b263867 100644 > --- a/drivers/vdpa/virtio_pci/vp_vdpa.c > +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c > @@ -213,10 +213,16 @@ 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); > + struct pci_dev *pdev = mdev->pci_dev; > > if (status & VIRTIO_CONFIG_S_DRIVER_OK && > !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { > - vp_vdpa_request_irq(vp_vdpa); > + if (vp_vdpa_request_irq(vp_vdpa)) { > + WARN_ON(1); > + dev_err(&pdev->dev, > + "vp_vdpa : fail set status is DRIVER_OK\n"); I think having one of the WARN_ON or dev_err should be sufficient. Thanks > + return; > + } > } > > vp_modern_set_status(mdev, status); > -- > 2.43.0 >
