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 <[email protected]> To: Jean-Philippe Brucker <[email protected]>, Joerg Roedel <[email protected]>, Will Deacon <[email protected]> Cc: [email protected], [email protected], [email protected], Robin Murphy <[email protected]> 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 <[email protected]> --- 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
