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

[Saravana Kannan]

On Mon, Nov 4, 2019 at 5:29 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> 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.

Sorry for the late reply. Got caught up on other work.

I didn't think the SMMU itself was PCI based in the example Jean gave.
I thought it just happened to be the case where the SMMU probes after
the pcieport but before the other children. If the SMMU itself is a
child of the pcieport, how can it be required for the parent to
function? How will suspend/resume even work?! I feel like I'm missing
some context wrt to PCI that everyone else seems to know (which isn't
surprising).

> 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...

Worry about child devices in what sense? From a non-iommu perspective?

-Saravana