Re: [PATCH v2 4/4] iommu: Get DT/ACPI parsing into the proper probe path

[Robin Murphy <robin.murphy@arm.com>]

On 23/03/2026 5:18 pm, Tudor Ambarus wrote:
> Hi, Robin,
> 
> On 2/28/25 5:46 PM, Robin Murphy wrote:
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index a3b45b84f42b..1cec7074367a 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
>>   	if (!dev_iommu_get(dev))
>>   		return -ENOMEM;
>>   	/*
>> -	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
>> -	 * instances with non-NULL fwnodes, and client devices should have been
>> -	 * identified with a fwspec by this point. Otherwise, we can currently
>> +	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
>> +	 * is buried in the bus dma_configure path. Properly unpicking that is
>> +	 * still a big job, so for now just invoke the whole thing. The device
>> +	 * already having a driver bound means dma_configure has already run and
>> +	 * either found no IOMMU to wait for, or we're in its replay call right
>> +	 * now, so either way there's no point calling it again.
>> +	 */
>> +	if (!dev->driver && dev->bus->dma_configure) {
>> +		mutex_unlock(&iommu_probe_device_lock);
>> +		dev->bus->dma_configure(dev);
>> +		mutex_lock(&iommu_probe_device_lock);
>> +	}
> 
> I was chasing the "something fishy" dev_WARN on a 6.19+ downstream
> android kernel and while looking at the IOMMU code I couldn't help
> myself and ask whether we shall prevent concurrent execution of
> dma_configure().
> 
> It seems to me that while the IOMMU subsystem is executing
> dma_configure(), the deferred probe workqueue can concurrently pick up
> the same device, enter really_probe(), set dev->driver, and execute
> dma_configure(). Is it worth protecting against this?

Yes, it's certainly still possible to hit a false-positive if thread A 
in iommu_device_register()->bus_iommu_probe() races against thread B 
attempting to bind, simply because thread B can set dev->driver long 
before it gets to any point where ends up serialising on 
iommu_probe_device_lock again, so thread A can observe that even while 
it is doing the IOMMU probe in the "correct" context. Other than the 
warning though, it's still functionally OK even if the "wrong" thread 
does end up finishing the probe, at least after 0c8e9c148e29 ("iommu: 
Avoid introducing more races").

> I can try to prove it if needed, using a downstream iommu driver (sigh).
> 
> Thanks!
> ta
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e61927b4d41f..5f0c1a8064b5 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -461,9 +461,19 @@ static int iommu_init_device(struct device *dev)
>           * already having a driver bound means dma_configure has already run and
>           * found no IOMMU to wait for, so there's no point calling it again.
>           */
> -       if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
> +       if (!dev->iommu->fwspec && !READ_ONCE(dev->driver) &&
> +           dev->bus->dma_configure) {
>                  mutex_unlock(&iommu_probe_device_lock);
> -               dev->bus->dma_configure(dev);
> +
> +               /*
> +                * Serialize with really_probe(). Recheck dev->driver in case a
> +                * driver bound while we were waiting for the lock.
> +                */
> +               device_lock(dev);
> +               if (!dev->driver)
> +                       dev->bus->dma_configure(dev);
> +               device_unlock(dev);

Much as I can't wait to get rid of iommu_probe_device_lock, the main 
reason we still can't rely on device_lock() at the moment is not 
actually the remaining sketchy replay-dependers per the comment in 
__iommu_probe_device(), but more fundamentally that for most IOMMU 
drivers this will deadlock in that same 
iommu_device_register()->bus_iommu_probe() path, when the bus walk 
happens to stumble across the IOMMU device itself, which of course is 
already locked as it's still in the middle of its own driver bind. I 
couldn't see an easy, clean and reliable way to get around that, so that 
can got kicked down the road in order to get the "call of_xlate in the 
right order and make iommu_device_register() actually work" basics 
landed (and start shaking out all these other problems...)

Thanks,
Robin.

> +
>                  mutex_lock(&iommu_probe_device_lock);
>                  /* If another instance finished the job for us, skip it */
>                  if (!dev->iommu || dev->iommu_group)
> (END)