Re: [PATCH v10 1/4] iommu: Always define struct iommu_fwspec

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

On Thu, Nov 03, 2022 at 12:23:20PM +0000, Robin Murphy wrote:
> On 2022-11-03 04:38, Prathamesh Shete wrote:
> > In order to fully make use of the !IOMMU_API stub functions, make the
> > struct iommu_fwspec always available so that users of the stubs can keep
> > using the structure's internals without causing compile failures.
> 
> I'm really in two minds about this... fwspecs are an internal detail of the
> IOMMU API that are meant to be private between individual drivers and
> firmware code, so anything poking at them arguably does and should depend on
> CONFIG_IOMMU_API. It looks like the stub for dev_iommu_fwspec_get() was only
> added for the sake of one driver that was misusing it where it really wanted
> device_iommu_mapped(), and has since been fixed, so if anything my
> preference would be to remove that stub :/

Tegra has been using this type of weak dependency on IOMMU_API mainly in
order to allow building without the IOMMU support on some old platforms
where people may actually care about the kernel size (Tegra20 systems
were sometimes severely constrained and don't have anything that we'd
call an IOMMU today).

We have similar stubs in place for most other major subsystems in order
to allow code to simply compile out if the subsystem is disabled, which
is quite convenient for sharing code between platforms that may want a
given feature and other platforms that may not want it, without causing
too much of a hassle with compile-testing.

> I don't technically have much objection to this patch in isolation, but what
> I don't like is the direction of travel it implies. I see the anti-pattern
> is only spread across Tegra drivers, making Tegra-specific assumptions, so
> in my view the best answer would be to abstract that fwpsec dependency into
> a single Tegra-specific helper, which would better represent the nature of
> what's really going on here.

I don't see how this is an anti-pattern. It might not be common for
drivers to need to reach into iommu_fwspec, so that might indeed be
specific to Tegra (for whatever reason our IP seems to want extra
flexibility), but the general pattern of using stubs is wide-spread,
so I don't see why IOMMU_API would need to be special.

Of course we could get rid of the stubs and make these hard
dependencies, but I suspect some people may then get mad that they can
no longer disable the IOMMU dependency.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]