[PATCH] iommu/ipmmu-vmsa: Register in a sensible order

[PATCH] iommu/ipmmu-vmsa: Register in a sensible order

[Robin Murphy]

IPMMU registers almost-initialised instances, but misses assigning the
drvdata to make them fully functional, so initial calls back into
ipmmu_probe_device() are likely to fail unnecessarily. Reorder this to
work as it should, also pruning the long-out-of-date comment and adding
the missing sysfs cleanup on error for good measure.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/ipmmu-vmsa.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 074daf1aac4e..e424b279a8cd 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1081,31 +1081,24 @@ static int ipmmu_probe(struct platform_device *pdev)
 		}
 	}
 
+	platform_set_drvdata(pdev, mmu);
 	/*
 	 * Register the IPMMU to the IOMMU subsystem in the following cases:
 	 * - R-Car Gen2 IPMMU (all devices registered)
 	 * - R-Car Gen3 IPMMU (leaf devices only - skip root IPMMU-MM device)
 	 */
-	if (!mmu->features->has_cache_leaf_nodes || !ipmmu_is_root(mmu)) {
-		ret = iommu_device_sysfs_add(&mmu->iommu, &pdev->dev, NULL,
-					     dev_name(&pdev->dev));
-		if (ret)
-			return ret;
+	if (mmu->features->has_cache_leaf_nodes && ipmmu_is_root(mmu))
+		return 0;
 
-		ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
-		if (ret)
-			return ret;
-	}
+	ret = iommu_device_sysfs_add(&mmu->iommu, &pdev->dev, NULL, dev_name(&pdev->dev));
+	if (ret)
+		return ret;
 
-	/*
-	 * We can't create the ARM mapping here as it requires the bus to have
-	 * an IOMMU, which only happens when bus_set_iommu() is called in
-	 * ipmmu_init() after the probe function returns.
-	 */
+	ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
+	if (ret)
+		iommu_device_sysfs_remove(&mmu->iommu);
 
-	platform_set_drvdata(pdev, mmu);
-
-	return 0;
+	return ret;
 }
 
 static void ipmmu_remove(struct platform_device *pdev)
-- 
2.39.2.101.g768bb238c484.dirty

Re: [PATCH] iommu/ipmmu-vmsa: Register in a sensible order

[Geert Uytterhoeven]

Hi Robin,

On Thu, 20 Mar 2025 at 15:41, Robin Murphy <robin.murphy@arm.com> wrote:
> IPMMU registers almost-initialised instances, but misses assigning the
> drvdata to make them fully functional, so initial calls back into
> ipmmu_probe_device() are likely to fail unnecessarily. Reorder this to
> work as it should, also pruning the long-out-of-date comment and adding
> the missing sysfs cleanup on error for good measure.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Thanks for your patch!

This fixes the

    sata_rcar ee300000.sata: late IOMMU probe at driver bind,
something fishy here!
    WARNING: CPU: 1 PID: 13 at drivers/iommu/iommu.c:571
__iommu_probe_device+0x208/0x38c

I saw on Salvator-XS with R-Car M3-N.

It does not fix the second issue reported, so it is indeed too early for a
"Closes: https://lore.kernel.org/CAMuHMdWPFnHTFeeWL2-BU8tKOL-E5K2ROOz=LLBLTJJLCK9NgA@mail.gmail.com"
tag.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -1081,31 +1081,24 @@ static int ipmmu_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       platform_set_drvdata(pdev, mmu);

Nit: perhaps insert a blank line here, before the comment below?

>         /*
>          * Register the IPMMU to the IOMMU subsystem in the following cases:
>          * - R-Car Gen2 IPMMU (all devices registered)
>          * - R-Car Gen3 IPMMU (leaf devices only - skip root IPMMU-MM device)
>          */
> -       if (!mmu->features->has_cache_leaf_nodes || !ipmmu_is_root(mmu)) {
> -               ret = iommu_device_sysfs_add(&mmu->iommu, &pdev->dev, NULL,
> -                                            dev_name(&pdev->dev));
> -               if (ret)
> -                       return ret;
> +       if (mmu->features->has_cache_leaf_nodes && ipmmu_is_root(mmu))
> +               return 0;
>
> -               ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
> -               if (ret)
> -                       return ret;
> -       }
> +       ret = iommu_device_sysfs_add(&mmu->iommu, &pdev->dev, NULL, dev_name(&pdev->dev));
> +       if (ret)
> +               return ret;
>
> -       /*
> -        * We can't create the ARM mapping here as it requires the bus to have
> -        * an IOMMU, which only happens when bus_set_iommu() is called in
> -        * ipmmu_init() after the probe function returns.
> -        */
> +       ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
> +       if (ret)
> +               iommu_device_sysfs_remove(&mmu->iommu);
>
> -       platform_set_drvdata(pdev, mmu);
> -
> -       return 0;
> +       return ret;
>  }
>
>  static void ipmmu_remove(struct platform_device *pdev)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] iommu/ipmmu-vmsa: Register in a sensible order

[Robin Murphy]

On 2025-03-25 3:26 pm, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Thu, 20 Mar 2025 at 15:41, Robin Murphy <robin.murphy@arm.com> wrote:
>> IPMMU registers almost-initialised instances, but misses assigning the
>> drvdata to make them fully functional, so initial calls back into
>> ipmmu_probe_device() are likely to fail unnecessarily. Reorder this to
>> work as it should, also pruning the long-out-of-date comment and adding
>> the missing sysfs cleanup on error for good measure.
>>
>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path")
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> Thanks for your patch!
> 
> This fixes the
> 
>      sata_rcar ee300000.sata: late IOMMU probe at driver bind,
> something fishy here!
>      WARNING: CPU: 1 PID: 13 at drivers/iommu/iommu.c:571
> __iommu_probe_device+0x208/0x38c
> 
> I saw on Salvator-XS with R-Car M3-N.
> 
> It does not fix the second issue reported, so it is indeed too early for a
> "Closes: https://lore.kernel.org/CAMuHMdWPFnHTFeeWL2-BU8tKOL-E5K2ROOz=LLBLTJJLCK9NgA@mail.gmail.com"
> tag.

You mean .of_xlate being called multiple times? That's not an issue, 
it's normal and expected. Every time an IOMMU instance registers, it 
triggers a probe of all relevant devices which do not yet have an IOMMU 
- this has never been selective, so if a device is associated with a 
different already-registered IOMMU instance, but does not have a group 
because that instance's .probe_device rejected it, that probe also gets 
tried (and rejected) again.

The core code behaviour has been this way for a very long time, the only 
new thing is that the .of_xlate calls are now in sync with their 
corresponding .probe_device calls (and the latter are also now working 
properly again for fwspec-based ops).

Was it just that, or is there still something functionally amiss?

> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

Robin.

>> --- a/drivers/iommu/ipmmu-vmsa.c
>> +++ b/drivers/iommu/ipmmu-vmsa.c
>> @@ -1081,31 +1081,24 @@ static int ipmmu_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> +       platform_set_drvdata(pdev, mmu);
> 
> Nit: perhaps insert a blank line here, before the comment below?
> 
>>          /*
>>           * Register the IPMMU to the IOMMU subsystem in the following cases:
>>           * - R-Car Gen2 IPMMU (all devices registered)
>>           * - R-Car Gen3 IPMMU (leaf devices only - skip root IPMMU-MM device)
>>           */
>> -       if (!mmu->features->has_cache_leaf_nodes || !ipmmu_is_root(mmu)) {
>> -               ret = iommu_device_sysfs_add(&mmu->iommu, &pdev->dev, NULL,
>> -                                            dev_name(&pdev->dev));
>> -               if (ret)
>> -                       return ret;
>> +       if (mmu->features->has_cache_leaf_nodes && ipmmu_is_root(mmu))
>> +               return 0;
>>
>> -               ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
>> -               if (ret)
>> -                       return ret;
>> -       }
>> +       ret = iommu_device_sysfs_add(&mmu->iommu, &pdev->dev, NULL, dev_name(&pdev->dev));
>> +       if (ret)
>> +               return ret;
>>
>> -       /*
>> -        * We can't create the ARM mapping here as it requires the bus to have
>> -        * an IOMMU, which only happens when bus_set_iommu() is called in
>> -        * ipmmu_init() after the probe function returns.
>> -        */
>> +       ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
>> +       if (ret)
>> +               iommu_device_sysfs_remove(&mmu->iommu);
>>
>> -       platform_set_drvdata(pdev, mmu);
>> -
>> -       return 0;
>> +       return ret;
>>   }
>>
>>   static void ipmmu_remove(struct platform_device *pdev)
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Re: [PATCH] iommu/ipmmu-vmsa: Register in a sensible order

[Geert Uytterhoeven]

Hi Robin,

On Tue, 1 Apr 2025 at 15:53, Robin Murphy <robin.murphy@arm.com> wrote:
> On 2025-03-25 3:26 pm, Geert Uytterhoeven wrote:
> > On Thu, 20 Mar 2025 at 15:41, Robin Murphy <robin.murphy@arm.com> wrote:
> >> IPMMU registers almost-initialised instances, but misses assigning the
> >> drvdata to make them fully functional, so initial calls back into
> >> ipmmu_probe_device() are likely to fail unnecessarily. Reorder this to
> >> work as it should, also pruning the long-out-of-date comment and adding
> >> the missing sysfs cleanup on error for good measure.
> >>
> >> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >> Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path")
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >
> > Thanks for your patch!
> >
> > This fixes the
> >
> >      sata_rcar ee300000.sata: late IOMMU probe at driver bind,
> > something fishy here!
> >      WARNING: CPU: 1 PID: 13 at drivers/iommu/iommu.c:571
> > __iommu_probe_device+0x208/0x38c
> >
> > I saw on Salvator-XS with R-Car M3-N.
> >
> > It does not fix the second issue reported, so it is indeed too early for a
> > "Closes: https://lore.kernel.org/CAMuHMdWPFnHTFeeWL2-BU8tKOL-E5K2ROOz=LLBLTJJLCK9NgA@mail.gmail.com"
> > tag.
>
> You mean .of_xlate being called multiple times? That's not an issue,
> it's normal and expected. Every time an IOMMU instance registers, it
> triggers a probe of all relevant devices which do not yet have an IOMMU
> - this has never been selective, so if a device is associated with a
> different already-registered IOMMU instance, but does not have a group
> because that instance's .probe_device rejected it, that probe also gets
> tried (and rejected) again.
>
> The core code behaviour has been this way for a very long time, the only
> new thing is that the .of_xlate calls are now in sync with their
> corresponding .probe_device calls (and the latter are also now working
> properly again for fwspec-based ops).

Hmm, I started seeing the extra calls only after bcb81ac6ae3c,
i.e. not since a very long time?

> Was it just that, or is there still something functionally amiss?

That's all for now ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] iommu/ipmmu-vmsa: Register in a sensible order

[Robin Murphy]

On 01/04/2025 3:11 pm, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Tue, 1 Apr 2025 at 15:53, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 2025-03-25 3:26 pm, Geert Uytterhoeven wrote:
>>> On Thu, 20 Mar 2025 at 15:41, Robin Murphy <robin.murphy@arm.com> wrote:
>>>> IPMMU registers almost-initialised instances, but misses assigning the
>>>> drvdata to make them fully functional, so initial calls back into
>>>> ipmmu_probe_device() are likely to fail unnecessarily. Reorder this to
>>>> work as it should, also pruning the long-out-of-date comment and adding
>>>> the missing sysfs cleanup on error for good measure.
>>>>
>>>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>>> Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path")
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>
>>> Thanks for your patch!
>>>
>>> This fixes the
>>>
>>>       sata_rcar ee300000.sata: late IOMMU probe at driver bind,
>>> something fishy here!
>>>       WARNING: CPU: 1 PID: 13 at drivers/iommu/iommu.c:571
>>> __iommu_probe_device+0x208/0x38c
>>>
>>> I saw on Salvator-XS with R-Car M3-N.
>>>
>>> It does not fix the second issue reported, so it is indeed too early for a
>>> "Closes: https://lore.kernel.org/CAMuHMdWPFnHTFeeWL2-BU8tKOL-E5K2ROOz=LLBLTJJLCK9NgA@mail.gmail.com"
>>> tag.
>>
>> You mean .of_xlate being called multiple times? That's not an issue,
>> it's normal and expected. Every time an IOMMU instance registers, it
>> triggers a probe of all relevant devices which do not yet have an IOMMU
>> - this has never been selective, so if a device is associated with a
>> different already-registered IOMMU instance, but does not have a group
>> because that instance's .probe_device rejected it, that probe also gets
>> tried (and rejected) again.
>>
>> The core code behaviour has been this way for a very long time, the only
>> new thing is that the .of_xlate calls are now in sync with their
>> corresponding .probe_device calls (and the latter are also now working
>> properly again for fwspec-based ops).
> 
> Hmm, I started seeing the extra calls only after bcb81ac6ae3c,
> i.e. not since a very long time?

OK, maybe not "very" - there are a lot of pieces in this puzzle that 
have all moved underfoot over time, but the point I was getting at is 
that although fwspec-based drivers *are* only now seeing those extra 
.probe_device calls, the design of the core code has been *trying* to 
make them since at least 3-ish years ago when the last of 
bus_set_iommu() went away.

Conversely though, you should also now *not* be seeing extra calls where 
you weren't looking for them. I bet if you were to build the DMA driver 
as a module and load/unload it a bunch of times, you would have seen 
more repeated calls to .of_xlate/.probe_device, whereas now you 
(correctly) won't.

>> Was it just that, or is there still something functionally amiss?
> 
> That's all for now ;-)

Great!

Thanks,
Robin.

Re: [PATCH] iommu/ipmmu-vmsa: Register in a sensible order

[Joerg Roedel]

On Thu, Mar 20, 2025 at 02:41:27PM +0000, Robin Murphy wrote:
>  drivers/iommu/ipmmu-vmsa.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)

Applied, thanks.