Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

[Thierry Reding]

On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote:
> 01.10.2020 14:04, Nicolin Chen пишет:
> > On Thu, Oct 01, 2020 at 12:23:16PM +0200, Thierry Reding wrote:
> >  > > >>>>>> It looks to me like the only reason why you need this new global API is
> >>>>>>>>>> because PCI devices may not have a device tree node with a phandle to
> >>>>>>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >>>>>>>>>> root complex has an iommus property, right? In that case, can't we
> >>>>>>>>>> simply do something like this:
> >>>>>>>>>>
> >>>>>>>>>> 	if (dev_is_pci(dev))
> >>>>>>>>>> 		np = find_host_bridge(dev)->of_node;
> >>>>>>>>>> 	else
> >>>>>>>>>> 		np = dev->of_node;
> > 
> >>> I personally am not a fan of adding a path for PCI device either,
> >>> since PCI/IOMMU cores could have taken care of it while the same
> >>> path can't be used for other buses.
> >>
> >> There's already plenty of other drivers that do something similar to
> >> this. Take a look at the arm-smmu driver, for example, which seems to be
> >> doing exactly the same thing to finding the right device tree node to
> >> look at (see dev_get_dev_node() in drivers/iommu/arm-smmu/arm-smmu.c).
> > 
> > Hmm..okay..that is quite convincing then...
> 
> Not very convincing to me. I don't see a "plenty of other drivers",
> there is only one arm-smmu driver.

There's ARM SMMU, ARM SMMU v3 and at least FSL PAMU. Even some of the
x86 platforms use dev_is_pci() to special-case PCI devices. That's just
because PCI is fundamentally different from fixed devices such as those
on a platform bus.

> The dev_get_dev_node() is under CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS (!).
> Guys, doesn't it look strange to you? :)

See above, there are other cases where PCI devices are treated special.
For example, pretty much every driver that supports PCI differentiates
between PCI and other devices in their ->device_group() callback.

> The arm-smmu driver does a similar thing for the modern bindings to what
> Nicolin's v3 is doing.

I don't really have any objections to doing something similar to what
ARM SMMU does. My main objection is to the use of a global pointer that
is used to look up the SMMU. As you see, the ARM SMMU driver also does
this lookup via driver_find_device_by_fwnode() rather than storing a
global pointer.

Also you can't quite equate ARM SMMU with Tegra SMMU. ARM SMMU can
properly deal with devices behind a PCI host bridge, whereas on Tegra
all those devices are thrown in the same bucket with the same IOMMU
domain. So it's to be expected that some things will have to be
different between the two drivers.

> >>> If we can't come to an agreement on globalizing mc pointer, would
> >>> it be possible to pass tegra_mc_driver through tegra_smmu_probe()
> >>> so we can continue to use driver_find_device_by_fwnode() as v1?
> >>>
> >>> v1: https://lkml.org/lkml/2020/9/26/68
> >>
> >> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean
> >> tegra_smmu_probe_device()? I don't think we can do that because it isn't
> > 
> > I was saying to have a global parent_driver pointer: similar to
> > my v1, yet rather than "extern" the tegra_mc_driver, we pass it
> > through egra_smmu_probe() and store it in a static global value
> > so as to call tegra_smmu_get_by_fwnode() in ->probe_device().
> > 
> > Though I agree that creating a global device pointer (mc) might
> > be controversial, yet having a global parent_driver pointer may
> > not be against the rule, considering that it is common in iommu
> > drivers to call driver_find_device_by_fwnode in probe_device().
> 
> You don't need the global pointer if you have SMMU OF node.
> 
> You could also get driver pointer from mc->dev->driver.
> 
> But I don't think you need to do this at all. The probe_device() could
> be invoked only for the tegra_smmu_ops and then seems you could use
> dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver
> does.

Have you also seen that sun50i-iommu does look up the SMMU from a
phandle using of_find_device_by_node()? So I think you've shown yourself
that even "modern" drivers avoid global pointers and look up via
phandle.

Thierry

Attachment: signature.asc
Description: PGP signature