iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: John Stultz <john.stultz@linaro.org>
Cc: Maulik Shah <mkshah@codeaurora.org>,
	Jason Cooper <jason@lakedaemon.net>,
	Saravana Kannan <saravanak@google.com>,
	Marc Zyngier <maz@kernel.org>,
	Linus Walleij <linus.walleij@linaro.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>,
	Will Deacon <will@kernel.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: Fri, 30 Oct 2020 14:12:15 +0000	[thread overview]
Message-ID: <b5c5146d-4112-e0c2-d1dd-2ad0882190b3@arm.com> (raw)
In-Reply-To: <CALAqxLW13=cvTX3ghskb9uG_YoVh7kvp8UQGUB8mVDGYXHWpVQ@mail.gmail.com>

On 2020-10-30 01:02, John Stultz wrote:
> On Wed, Oct 28, 2020 at 7:51 AM Robin Murphy <robin.murphy@arm.com> wrote:
>> Hmm, perhaps I'm missing something here, but even if the config options
>> *do* line up, what prevents arm-smmu probing before qcom-scm and
>> dereferencing NULL in qcom_scm_qsmmu500_wait_safe_toggle() before __scm
>> is initialised?
> 
> Oh man, this spun me on a "wait, but how does it all work!" trip. :)
> 
> So in the non-module case, the qcom_scm driver is a subsys_initcall
> and the arm-smmu is a module_platform_driver, so the ordering works
> out.
> 
> In the module case, the arm-smmu code isn't loaded until the qcom_scm
> driver finishes probing due to the symbol dependency handling.

My point is that module load != driver probe. AFAICS you could disable 
drivers_autoprobe, load both modules, bind the SMMU to its driver first, 
and boom!

> To double check this, I added a big msleep at the top of the
> qcom_scm_probe to try to open the race window you described, but the
> arm_smmu_device_probe() doesn't run until after qcom_scm_probe
> completes.

I don't think asynchronous probing is enabled by default, so indeed I 
would expect that to still happen to work ;)

> So at least as a built in / built in, or a module/module case its ok.
> And in the case where arm-smmu is a module and qcom_scm is built in
> that's ok too.

In the built-in case, I'm sure it happens to work out similarly because 
the order of nodes in the DTB tends to be the order in which devices are 
autoprobed. Again, async probe might be enough to break things; manually 
binding drivers definitely should; moving the firmware node to the end 
of the DTB probably would as well.

> Its just the case my patch is trying to prevent is where arm-smmu is
> built in, but qcom_scm is a module that it can't work (due to build
> errors in missing symbols,  or if we tried to use function pointers to
> plug in the qcom_scm - the lack of initialization ordering).
> 
> Hopefully that addresses your concern? Let me know if I'm still
> missing something.

What I was dancing around is that the SCM API (and/or its users) appears 
to need a general way to tell whether SCM is ready or not, because the 
initialisation ordering problem is there anyway. Neither Kconfig nor the 
module loader can solve that.

One possible self-contained workaround would be to see if an SCM DT node 
exists, see if a corresponding device exists, and see if that device has 
a driver bound. It's a little ugly, and strictly it still doesn't tell 
you that the _right_ driver is bound, but at least it's a lot more 
robust than implicitly relying on DT order, default probing behaviours, 
and hope.

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

  reply	other threads:[~2020-10-30 14:12 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
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 [this message]
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=b5c5146d-4112-e0c2-d1dd-2ad0882190b3@arm.com \
    --to=robin.murphy@arm.com \
    --cc=agross@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=ilina@codeaurora.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jason@lakedaemon.net \
    --cc=john.stultz@linaro.org \
    --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 \
    --subject='Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module' \
    /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

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