Re: [PATCH v3 0/2] iommu/virtio: Probe fixes for proper driver state

[Weimin Xiong <xiongwm2026@163.com>]

Subject: Re: [PATCH v3 0/2] iommu/virtio: Probe fixes for proper driver state

Hi Will,

Thank you for your patience and I'm really sorry for the issues you raised.

Please note: I sent a combined patch earlier (around 10:42 on July 24) that supersedes this v3 series. Your reply below may have been based on the earlier version. The combined patch addressed all the issues mentioned below.

1. About sending patches as attachments:
I apologize. I will read Documentation/process/submitting-patches.rst and make sure to send patches as inline text in the email body from now on.

2. About the incorrect Reviewed-by and Suggested-by tags:
I sincerely apologize for including incorrect Reviewed-by and Suggested-by tags in my previous patches. These tags were added without your explicit consent and should not have been included. I take full responsibility for this mistake and will be more careful in the future.

---

Regarding the combined patch:

I've combined the two probe fixes into a single patch. Here's what it does:

1. Store vdev->priv before creating virtqueues so the event callback always sees initialized driver state

2. Check the return value of iommu_device_register() and clean up sysfs entries on failure

3. Changed viommu_init_vqs() to take vdev instead of viommu, since we already have a link between them

I've also dropped the "Reject short event buffers" patch since Xu Rao independently submitted an identical fix.

---

The clean patch is included inline below:

From: Xiong Weimin <xiongweimin@kylinos.cn>
To: Jean-Philippe Brucker <jpb@kernel.org>,
	Joerg Roedel <joro@8bytes.org>,
	Will Deacon <will@kernel.org>
Cc: virtualization@lists.linux.dev,
	iommu@lists.linux.dev,
	linux-kernel@vger.kernel.org,
	Robin Murphy <robin.murphy@arm.com>
Subject: [PATCH] iommu/virtio: Fix probe error handling and driver state initialization

The event virtqueue callback retrieves the driver state through
vq->vdev->priv. viommu_probe() currently initializes that pointer only
after virtio_device_ready() and after the event queue is populated.

Store the driver data before creating the virtqueues so callbacks always
see initialized driver state once the device is made ready. Clear the
pointer again on probe failure.

Also check the return value of iommu_device_register() and properly
clean up sysfs entries on failure. This ensures that the device sysfs
directory is removed if registration fails.

Change viommu_init_vqs() to take a struct virtio_device * instead of
struct viommu_dev *, since we already have a link between them.

Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
 drivers/iommu/virtio-iommu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 587fc1319..288eea386 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -1110,9 +1110,9 @@ static const struct iommu_ops viommu_ops = {
 	}
 };
 
-static int viommu_init_vqs(struct viommu_dev *viommu)
+static int viommu_init_vqs(struct virtio_device *vdev)
 {
-	struct virtio_device *vdev = dev_to_virtio(viommu->dev);
+	struct viommu_dev *viommu = vdev->priv;
 	struct virtqueue_info vqs_info[] = {
 		{ "request" },
 		{ "event", viommu_event_handler },
@@ -1169,11 +1169,12 @@ static int viommu_probe(struct virtio_device *vdev)
 	ida_init(&viommu->domain_ids);
 	viommu->dev = dev;
 	viommu->vdev = vdev;
+	vdev->priv = viommu;
 	INIT_LIST_HEAD(&viommu->requests);
 
-	ret = viommu_init_vqs(viommu);
+	ret = viommu_init_vqs(vdev);
 	if (ret)
-		return ret;
+		goto err_clear_priv;
 
 	virtio_cread_le(vdev, struct virtio_iommu_config, page_size_mask,
 			&viommu->pgsize_bitmap);
@@ -1236,7 +1237,9 @@ static int viommu_probe(struct virtio_device *vdev)
 
 	vdev->priv = viommu;
 
-	iommu_device_register(&viommu->iommu, &viommu_ops, parent_dev);
+	ret = iommu_device_register(&viommu->iommu, &viommu_ops, parent_dev);
+	if (ret)
+		goto err_free_sysfs;
 
 	dev_info(dev, "input address: %u bits\n",
 		 order_base_2(viommu->geometry.aperture_end));
@@ -1244,8 +1247,12 @@ static int viommu_probe(struct virtio_device *vdev)
 
 	return 0;
 
+err_free_sysfs:
+	iommu_device_sysfs_remove(&viommu->iommu);
 err_free_vqs:
 	vdev->config->del_vqs(vdev);
+err_clear_priv:
+	vdev->priv = NULL;
 
 	return ret;
 }

Sorry again for the confusion and thank you for your patience.

Best regards,
Weimin Xiong