iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: John Stultz <john.stultz@linaro.org>
To: Will Deacon <will@kernel.org>
Cc: Maulik Shah <mkshah@codeaurora.org>,
	Jason Cooper <jason@lakedaemon.net>,
	Saravana Kannan <saravanak@google.com>,
	Marc Zyngier <maz@kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	Lina Iyer <ilina@codeaurora.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	iommu@lists.linux-foundation.org, Andy Gross <agross@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Todd Kjos <tkjos@google.com>
Subject: Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
Date: Tue, 27 Oct 2020 22:53:47 -0700	[thread overview]
Message-ID: <CALAqxLUDVEq4ds2Wbic6uaK3=dELKKO4eGQxjHFFz19GeUFd_w@mail.gmail.com> (raw)
In-Reply-To: <20200713204133.GA3731@willie-the-truck>

On Mon, Jul 13, 2020 at 1:41 PM Will Deacon <will@kernel.org> wrote:
> On Fri, Jul 10, 2020 at 03:21:53PM -0700, John Stultz wrote:
> > On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> > > On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > > > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > > index b510f67dfa49..714893535dd2 100644
> > > > > > --- a/drivers/iommu/Kconfig
> > > > > > +++ b/drivers/iommu/Kconfig
> > > > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > > > >  config ARM_SMMU
> > > > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > > > >       select IOMMU_API
> > > > > >       select IOMMU_IO_PGTABLE_LPAE
> > > > > >       select ARM_DMA_USE_IOMMU if ARM
> > > > >
> > > > > This looks like a giant hack. Is there another way to handle this?
> > > >
> > > > Sorry for the slow response here.
> > > >
> > > > So, I agree the syntax looks strange (requiring a comment obviously
> > > > isn't a good sign), but it's a fairly common way to ensure drivers
> > > > don't get built in if they optionally depend on another driver that
> > > > can be built as a module.
> > > >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > > > !USB_GADGET" in various Kconfig files.
> > > >
> > > > I'm open to using a different method, and in a different thread you
> > > > suggested using something like symbol_get(). I need to look into it
> > > > more, but that approach looks even more messy and prone to runtime
> > > > failures. Blocking the unwanted case at build time seems a bit cleaner
> > > > to me, even if the syntax is odd.
> > >
> > > Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> > > as that driver _really_ doesn't care about SoC details like this. In other
> > > words, add a new entry along the lines of:
> > >
> > >         config ARM_SMMU_QCOM_IMPL
> > >         default y
> > >         #if QCOM_SCM=m this can't be =y
> > >         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
> > >
> > > and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> > > which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> > > so that we don't bother to compile arm-smmu-qcom.o in that case.
> > >
> > > Would that work?
> >
> > I think this proposal still has problems with the directionality of the call.
> >
> > The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
> > So if qcom_scm.o is part of a module, the calling code in
> > arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
> > needs to be a module.
> >
> > I know you said the arm-smmu driver doesn't care about SoC details,
> > but the trouble is that currently the arm-smmu driver does directly
> > call the qcom-scm code. So it is a real dependency. However, if
> > QCOM_SCM is not configured, it calls stubs and that's ok.  In that
> > way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
> > It looks terrible because we're used to boolean logic, but it's
> > ternary.
>
> Yes, it looks ugly, but the part I really have issues with is that building
> QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC
> with the qcom implementation. I don't see why we need to enforce things
> here beyond making sure that all selectable permutations _build_ and
> fail gracefully at runtime on the qcom SoC if SCM isn't available.

Hey Will,
  Sorry to dredge up this old thread. I've been off busy with other
things and didn't get around to trying to rework this until now.

Unfortunately I'm still having some trouble coming up with a better
solution. Initially I figured I'd rework the qcom_scm driver to, so
that we have the various qcom_scm_* as inline functions, which call
out to function pointers that the qcom_scm driver would register when
the module loaded (Oof, and unfortunately there are a *ton* qcom_scm_*
functions so its a bunch of churn).

The trouble I realized with that approach is that if the ARM_SMMU code
is built in, then it may try to use the qcom_scm code before the
module loads and sets those function pointers. So while it would build
ok, the issue would be when the arm_smmu_device_reset() is done by
done on arm_smmu_device_probe(), it wouldn't actually call the right
code.  There isn't a really good way to deal with the module loading
at some random time after arm_smmu_device_probe() completes.

This is the benefit of the module symbol dependency tracking: If the
arm_smmu.ko calls symbols in qcom_scm.ko then qcom_scm.ko has to load
first.
But if arm_smmu is built in, I haven't found a clear mechanism to
force qcom_scm to load before we probe, if it's configured as a
module.

I also looked into the idea of reworking the arm-smmu-impl code to be
modular instead, and while it does provide a similar method of using
function pointers to minimize the amount of symbols the arm-smmu code
needs to know about, the initialization call path is
arm_smmu_device_probe -> arm_smmu_impl_init -> qcom_smmu_impl_init. So
it doesn't really allow for dynamic registration of implementation
modules at runtime.

So I'm sort of stewing on maybe trying to rework the directionality,
so the arm-smmu-qcom.o code probes and calls arm_smmu_impl_init and
that is what initializes the arm_smmu_device_probe logic?

Alternatively, I'm considering trying to switch the module dependency
annotation so that the CONFIG_QCOM_SCM modularity depends on ARM_SMMU
being a module. But that is sort of putting the restriction on the
callee instead of the caller (sort of flipping the meaning of the
depends), which feels prone to later trouble (and with multiple users
of CONFIG_QCOM_SCM needing similar treatment, it would make it
difficult to discover the right combination of configs needed to allow
it to be a module).

Anyway, I wanted to reach out to see if you had any further ideas
here. Sorry for letting such a large time gap pass!

thanks
-john
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  parent reply	other threads:[~2020-10-28  5:54 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-25  0:10 [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules John Stultz
2020-06-25  0:10 ` [PATCH v2 1/5] irq: irqdomain: Export irq_domain_update_bus_token John Stultz
2020-06-25  0:10 ` [PATCH v2 2/5] irq: irqchip: Export irq_chip_retrigger_hierarchy and irq_chip_set_vcpu_affinity_parent John Stultz
2020-06-25  0:10 ` [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module John Stultz
2020-06-26  7:42   ` Stephen Boyd
2020-06-27  1:34     ` John Stultz
2020-06-27  9:37       ` Marc Zyngier
2020-07-10  6:02         ` Stephen Boyd
2020-07-10 22:44           ` John Stultz
2020-07-10 23:27             ` Stephen Boyd
2020-07-12  9:27               ` Marc Zyngier
2020-06-25  0:10 ` [PATCH v2 4/5] pinctrl: qcom: Allow pinctrl-msm code to be loadable as a module John Stultz
2020-06-25  0:10 ` [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module John Stultz
2020-07-02 12:47   ` Greg Kroah-Hartman
2020-07-02 14:18   ` Will Deacon
2020-07-10  3:28     ` John Stultz
2020-07-10  7:54       ` Will Deacon
2020-07-10 22:21         ` John Stultz
2020-07-13 20:41           ` Will Deacon
2020-07-13 20:48             ` John Stultz
2020-07-14  7:56               ` Will Deacon
2020-10-28  5:53             ` John Stultz [this message]
2020-10-28 13:51               ` Will Deacon
2020-10-28 14:51                 ` Robin Murphy
2020-10-30  1:02                   ` John Stultz
2020-10-30 14:12                     ` Robin Murphy
2020-10-31  0:12                       ` John Stultz
2020-10-30  6:09                 ` John Stultz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CALAqxLUDVEq4ds2Wbic6uaK3=dELKKO4eGQxjHFFz19GeUFd_w@mail.gmail.com' \
    --to=john.stultz@linaro.org \
    --cc=agross@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=ilina@codeaurora.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jason@lakedaemon.net \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=mkshah@codeaurora.org \
    --cc=saravanak@google.com \
    --cc=tglx@linutronix.de \
    --cc=tkjos@google.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).