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

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

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.