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

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

Thanks Will for the response!!

On 1/3/2025 9:04 PM, Will Deacon wrote:
> (Please try not to top-post as it makes the thread hard to follow)
> 
> On Thu, Jan 02, 2025 at 04:09:46PM +0530, Charan Teja Kalla wrote:
>> 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(-)
>>
>> 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...
> 
> Can you reproduce the issue on 6.13-rc5? We bodged some of the probe
> code recently:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=229e6ee43d2a160a1592b83aad620d6027084aad
> 
> and a better set of fixes are queued in -next.

Yes, we did try a better patch from Robin[1] and the issue is still
reproducible.

IIUC, these patches are trying to get valid smmu device, during the
probe of an smmu client device, by traversing proper 'klist_devices'
happening from iommu_init_device(). Please CMIW.

But this bug makes iommu_init_device() to completely gets skipped.

static int __iommu_probe_device(struct device *dev, ...)
{
	/* Device is probed already if in a group */
        if (dev->iommu_group)
                return 0;  --> driver call returns from here.

        ret = iommu_init_device(dev, ops);
        if (ret)
                return ret;


}

Regarding the testing, we work on Android that is on LTS kernels, so, it
may not be possible for us to try the thing on 6.13 and this seems a bug
on LTS 6.6 kernel.

[1]
https://lore.kernel.org/all/0952ca36-c5d9-462a-ab7b-b97154c56919@arm.com/

Thanks