On 2025-03-25 3:26 pm, Geert Uytterhoeven wrote:
Hi Robin, On Thu, 20 Mar 2025 at 15:41, Robin Murphy <robin.murphy@xxxxxxx> 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@xxxxxxxxxxxxxx> Fixes: bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path") Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>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@xxxxxxxxxxxxxx"; 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@xxxxxxxxx> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
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
