diff mbox series

[v2,5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

Message ID 20200625001039.56174-6-john.stultz@linaro.org
State Superseded
Headers show
Series [v2,1/5] irq: irqdomain: Export irq_domain_update_bus_token | expand

Commit Message

John Stultz June 25, 2020, 12:10 a.m. UTC
Allow the qcom_scm driver to be loadable as a
permenent module.

Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Maulik Shah <mkshah@codeaurora.org>
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: iommu@lists.linux-foundation.org
Cc: linux-gpio@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/firmware/Kconfig    | 2 +-
 drivers/firmware/Makefile   | 3 ++-
 drivers/firmware/qcom_scm.c | 4 ++++
 drivers/iommu/Kconfig       | 2 ++
 4 files changed, 9 insertions(+), 2 deletions(-)

Comments

Will Deacon July 10, 2020, 7:54 a.m. UTC | #1
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?

Will
John Stultz July 13, 2020, 8:48 p.m. UTC | #2
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.


Ok. Thanks, that context/rationale makes sense to me now!  I'll dig in
and see if I can figure out the runtime get_symbol handling you
suggested for the scm callout.

Thanks again for the feedback!
-john
Will Deacon Oct. 28, 2020, 1:51 p.m. UTC | #3
On Tue, Oct 27, 2020 at 10:53:47PM -0700, John Stultz wrote:
> 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!

Well we can always go with your original hack, if it helps?

https://lore.kernel.org/linux-iommu/20200714075603.GE4277@willie-the-truck/

Will
Robin Murphy Oct. 28, 2020, 2:51 p.m. UTC | #4
On 2020-10-28 13:51, Will Deacon wrote:
> On Tue, Oct 27, 2020 at 10:53:47PM -0700, John Stultz wrote:
>> 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!
> 
> Well we can always go with your original hack, if it helps?
> 
> https://lore.kernel.org/linux-iommu/20200714075603.GE4277@willie-the-truck/

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?

Robin.
John Stultz Oct. 30, 2020, 6:09 a.m. UTC | #5
On Wed, Oct 28, 2020 at 6:51 AM Will Deacon <will@kernel.org> wrote:
> On Tue, Oct 27, 2020 at 10:53:47PM -0700, John Stultz wrote:
> > 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!
>
> Well we can always go with your original hack, if it helps?
>
> https://lore.kernel.org/linux-iommu/20200714075603.GE4277@willie-the-truck/

Yea. After trying a few more ideas that didn't pan out, I think I'm
going to fall back to that. :(

thanks
-john
Robin Murphy Oct. 30, 2020, 2:12 p.m. UTC | #6
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.
diff mbox series

Patch

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index fbd785dd0513..9e533a462bf4 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -236,7 +236,7 @@  config INTEL_STRATIX10_RSU
 	  Say Y here if you want Intel RSU support.
 
 config QCOM_SCM
-	bool
+	tristate "Qcom SCM driver"
 	depends on ARM || ARM64
 	select RESET_CONTROLLER
 
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 99510be9f5ed..cf24d674216b 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -17,7 +17,8 @@  obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
-obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
+qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 0e7233a20f34..b5e88bf66975 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1155,6 +1155,7 @@  static const struct of_device_id qcom_scm_dt_match[] = {
 	{ .compatible = "qcom,scm" },
 	{}
 };
+MODULE_DEVICE_TABLE(of, qcom_scm_dt_match);
 
 static struct platform_driver qcom_scm_driver = {
 	.driver = {
@@ -1170,3 +1171,6 @@  static int __init qcom_scm_init(void)
 	return platform_driver_register(&qcom_scm_driver);
 }
 subsys_initcall(qcom_scm_init);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. SCM driver");
+MODULE_LICENSE("GPL v2");
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
@@ -500,6 +501,7 @@  config QCOM_IOMMU
 	# Note: iommu drivers cannot (yet?) be built as modules
 	bool "Qualcomm IOMMU Support"
 	depends on ARCH_QCOM || (COMPILE_TEST && !GENERIC_ATOMIC64)
+	depends on QCOM_SCM=y
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select ARM_DMA_USE_IOMMU