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

[Charan Teja Kalla <quic_charante@quicinc.com>]

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,