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

[Tudor Ambarus]

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?

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