Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAPDyKFqJdiCDkAfrONfnBVKw1v8=jZ+hEJiKGK70EQ4o7BSxaQ@mail.gmail.com>
 <20221103043852.24718-1-pshete@nvidia.com> <6be39bae-f325-12e0-374b-a27c9ee2ef2b@arm.com>
 <Y2PJq27wkVwPg6rp@orome>
In-Reply-To: <Y2PJq27wkVwPg6rp@orome>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 3 Nov 2022 15:55:09 +0100
Message-ID: <CAPDyKFq8szzryFBNkw20wFoPTbAa8YDy0wJnb57yckZ-HFTAMw@mail.gmail.com>
Subject: Re: [PATCH v10 1/4] iommu: Always define struct iommu_fwspec
To:     Robin Murphy <robin.murphy@arm.com>,
        Thierry Reding <thierry.reding@gmail.com>
Cc:     Prathamesh Shete <pshete@nvidia.com>, joro@8bytes.org,
        adrian.hunter@intel.com, jonathanh@nvidia.com,
        p.zabel@pengutronix.de, linux-mmc@vger.kernel.org,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
        will@kernel.org, iommu@lists.linux.dev, anrao@nvidia.com,
        smangipudi@nvidia.com, kyarlagadda@nvidia.com,
        Thierry Reding <treding@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 3 Nov 2022 at 15:01, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> 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 agree with the above.

Moreover, the stubs make the code more portable/scalable and so it
becomes easier to maintain.

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

Again, I agree.

Moreover, a "git grep CONFIG_IOMMU_API" indicates that the problem
isn't specific to Tegra. The "#ifdef CONFIG_IOMMU_API" seems to be
sprinkled across the kernel. I think it would be nice if we could
improve the situation. So far, using stubs along with what the
$subject patch proposes, seems to me to be the best approach.

[...]

Kind regards
Uffe