Re: [PATCH 0/7] iommu: Permit modular builds of ARM SMMU[v3] drivers

[Robin Murphy]

On 04/11/2019 12:16, John Garry wrote:
> On 01/11/2019 21:13, Saravana Kannan wrote:
> > On Fri, Nov 1, 2019 at 3:28 AM John Garry <john.garry@xxxxxxxxxx> wrote:
> > > On 31/10/2019 23:34, Saravana Kannan via iommu wrote:
> > > > I looked into the iommu-map property and it shouldn't be too hard to
> > > > add support for it. Looks like we can simply hold off on probing the
> > > > root bridge device till all the iommus in its iommu-map are probed and
> > > > we should be fine.
> > > >
> > > > > I'm also unsure about distro vendors agreeing to a mandatory kernel
> > > > > parameter (of_devlink). Do you plan to eventually enable it by 
> > > > > default?
> > > > >
> > > > > > static const struct supplier_bindings of_supplier_bindings[] = {
> > > > > > ÂÂÂÂÂÂÂÂÂ { .parse_prop = parse_clocks, },
> > > > > > ÂÂÂÂÂÂÂÂÂ { .parse_prop = parse_interconnects, },
> > > > > > ÂÂÂÂÂÂÂÂÂ { .parse_prop = parse_regulators, },
> > > > > > +ÂÂÂÂÂÂÂ { .parse_prop = parse_iommus, },
> > > > > > ÂÂÂÂÂÂÂÂÂ {},
> > > > > > };
> > > > > >
> > > > > > I plan to upstream this pretty soon, but I have other patches in
> > > > > > flight that touch the same file and I'm waiting for those to get
> > > > > > accepted. I also want to clean up the code a bit to reduce some
> > > > > > repetition before I add support for more bindings.
> > > > > I'm also wondering about ACPI support.
> > > > I'd love to add ACPI support too, but I have zero knowledge of ACPI.
> > > > I'd be happy to help anyone who wants to add ACPI support that allows
> > > > ACPI to add device links.
> > >
> > > If possible to add, that may be useful for remedying this:
> > >
> > > https://lore.kernel.org/linux-iommu/9625faf4-48ef-2dd3-d82f-931d9cf26976@xxxxxxxxxx/ 
> >
> > I'm happy that this change might fix that problem, but isn't the
> > problem reported in that thread more to do with child devices getting
> > added before the parent probes successfully? That doesn't make sense
> > to me. 
>
> So the pcieport device and then the child device are added in the PCI 
> scan, but only some time later do the device drivers probe for these 
> devices; so it's not that the that pcieport driver creates the child 
> device.
>
> The problem then occurs in that the ordering the of device driver probe 
> is such that we have this: pcieport probe + defer (as no IOMMU group 
> registered), SMMU probe (registers the IOMMU group), child device probe, 
> pcieport really probe.
>
> Can't the piceport driver not add its child devices before it
> > probes successfully? Or more specifically, who adds the child devices
> > of the pcieport before the pcieport itself probes?
>
> The devices are actually added in order pcieport, child device, but not 
> really probed in that same order, as above.

Right, in short the fundamental problem is that of_iommu_configure() now 
does the wrong thing. Deferring probe of the entire host bridge/root 
complex based on "iommu-map" would indeed happen to solve the problem by 
brute force, I think, but could lead to a dependency cycle for PCI-based 
IOMMUs as Jean points out. I hope to have time this week to work a bit 
more on pulling of_iommu_configure() apart to fix it properly, after 
which of_devlink *should* only have to worry about the child devices 
themselves...

Robin.

> I'll add you to that thread if you want to discuss further.
>
> Thanks,
> John