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.
Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> --- 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@xxxxxxxxxxxxxx
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
