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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

Hi, Robin,

Thanks a lot for the educative answers!
And sorry for the late reply, I got sidetracked.

On 3/23/26 10:49 PM, Robin Murphy wrote:
> 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 confirm I have 0c8e9c148e29 ("iommu: Avoid introducing more races") in
my tree.

If the concurrent execution is functionally safe and the dev->driver
check is a known source of false positives during async probing, do you
think it would worth switching the dev_WARN to dev_info? I'm thinking
dev_WARN is a bit harsh, as it can disrupt CI pipelines that halt on
warnings.

>>
>> 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...)
> 

Thank you for the detailed explanation. Between the self-deadlock on
the IOMMU device itself, and the fact that a non-blocking
device_trylock() would be unsafe (as it could fail due to a simple sysfs
read, permanently orphaning the device from the bus walk), it's clear
there is no clean locking fix for this TOCTOU window right now.

Thanks,
ta