Re: [PATCH RFC] iommu: fix wrong DMA ops for iommu device

Charan Teja Kalla
2025-01-02T10:40:01Z

Just a quick ask if you have any comments for the below. BTW, in
internal tests, when we revert the commit f188056352bc ("iommu: Avoid
locking/unlocking for iommu_probe_device()", it is helping.

I can share more details If the below is not sufficient.

Thanks in advance...

On 12/13/2024 8:34 PM, Charan Teja Kalla wrote:
> Race between smmu device probe and iommu device probe is resulting into
> wrong DMA ops used for the device.
>
> bus_iommu_probe(P1) of_iommu_configure(P2)
> (from iommu_device_register) (from insmod of a driver)
> -------------------------- --------------------------
> 1) probe_iommu_group()
> (Fills just dev->iommu_group
> under iommu_probe_device_lock)
> 2) iommu_probe_device():
> (acquires the iommu_probe_device_lock,
> but since dev->iommu_group is already
> set, it just returns without
> allocating the domain.)
>
> 3) of_dma_configure_id()->
> arch_setup_dma_ops() gets called for
> this device, but returns with the
> error message:
> "Failed to set up IOMMU for device;
> retaining platform DMA ops"
>
> 4) device probe is succeeded where it
> can now setup iommu mappings using
> wrong ->dma_ops
>
> 5) domain will be allocated later
> and fills iommu_group->domain.
>
> Step 3) and 4) denies iommu device from using 'iommu_dma_ops'.
>
> This issue does exists on 6.6 because of commit f188056352bc ("iommu:
> Avoid locking/unlocking for iommu_probe_device()") without which 2)
> returns only after filling the domain under group->mutex, thus no issue.
>
> With commit b5c58b2fdc42("dma-mapping: direct calls for dma-iommu"),
> ->iommu_dma_ops is removed altogether and iommu api are directly being
> called under "use_dma_iommu(): return dev->dma_iommu". Again,
> ->dma_iommu flag is set only after domain allocation. So, If
> there is a sufficient delay between steps 4) and 5), issue is still
> exists but that seems very narrow to me.
>
> I am thinking, setup of the domain before returning from step2) is the
> way to fix this issue. Can you please help if you have any suggestions?
>
> Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
> ---
> drivers/iommu/iommu.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 3f1029c0825e..61fc8145f378 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -561,19 +561,43 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> int iommu_probe_device(struct device *dev)
> {
> const struct iommu_ops *ops;
> + struct iommu_group *group;
> int ret;
>
> mutex_lock(&iommu_probe_device_lock);
> ret = __iommu_probe_device(dev, NULL);
> mutex_unlock(&iommu_probe_device_lock);
> if (ret)
> - return ret;
> + goto err_out;
> +
> + group = iommu_group_get(dev);
> + if (!group) {
> + ret = -ENODEV;
> + goto err_release;
> + }
> +
> + mutex_lock(&group->mutex);
> + if (!group->default_domain) {
> + ret = iommu_setup_default_domain(group, 0);
> + if (ret)
> + goto err_unlock;
> + }
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
>
> ops = dev_iommu_ops(dev);
> if (ops->probe_finalize)
> ops->probe_finalize(dev);
>
> return 0;
> +
> +err_unlock:
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> +err_release:
> + iommu_release_device(dev);
> +err_out:
> + return ret;
> }
>
> static void __iommu_group_free_device(struct iommu_group *group,