firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

Allow the qcom_scm driver to be loadable as a permenent module.

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Andy Gross <[email protected]>

Cc Bjorn Andersson <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Linus Walleij <[email protected]>

Cc Vinod Koul <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Maulik Shah <[email protected]>

Cc Saravana Kannan <[email protected]>

Cc Todd Kjos <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Acked-by Kalle Valo <[email protected]>

Acked-by Greg Kroah-Hartman <[email protected]>

Acked-by Will Deacon <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

Signed-off-by John Stultz <[email protected]>

---
v3:
* Fix __arm_smccc_smc build issue reported by kernel test robot <[email protected]>
v4:
* Add "depends on QCOM_SCM || !QCOM_SCM" bit to ath10k config that requires it.
v5:
* Fix QCOM_QCM typo in Kconfig, it should be QCOM_SCM

---
 drivers/firmware/Kconfig                | 2 +-
 drivers/firmware/Makefile               | 3 ++-
 drivers/firmware/qcom_scm.c             | 4 ++++
 drivers/iommu/Kconfig                   | 2 ++
 drivers/net/wireless/ath/ath10k/Kconfig | 1 +
 5 files changed, 10 insertions(+), 2 deletions(-)

drivers/firmware/Kconfig +1 -1

index db0ea2d2d75a..af53778edc7e 100644
@@ -235,7 +235,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
 	depends on HAVE_ARM_SMCCC
 	select RESET_CONTROLLER

drivers/firmware/Makefile +2 -1

index 5e013b6a3692..523173cbff33 100644
@@ -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

drivers/firmware/qcom_scm.c +4 -0

index ee9cb545e73b..bb9ce3f92931 100644
@@ -1296,6 +1296,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 = {

@@ -1312,3 +1313,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");

drivers/iommu/Kconfig +2 -0

index 07b7c25cbed8..f61516c17589 100644
@@ -253,6 +253,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
 	tristate "ARM Ltd. System MMU (SMMU) Support"
 	depends on ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)
+	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

@@ -382,6 +383,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

drivers/net/wireless/ath/ath10k/Kconfig +1 -0

index 40f91bc8514d..741289e385d5 100644
@@ -44,6 +44,7 @@ config ATH10K_SNOC
 	tristate "Qualcomm ath10k SNOC support"
 	depends on ATH10K
 	depends on ARCH_QCOM || COMPILE_TEST
+	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
 	select QCOM_QMI_HELPERS
 	help
 	  This module adds support for integrated WCN3990 chip connected

Signature · 1 line

2.25.1

Quoted 9 lines · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 3 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

Are you (in this last paragraph) saying that all those who have been burnt by fw_devlink during the last months and therefor run with it disabled will have a less fun experience once this is merged?

(I'm picking this up, but I don't fancy the idea that some people are turning the boot process into a lottery)

Quoted 10 lines · Bjorn Andersson

Quoted 9 lines · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 8 lines

Quoted 3 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

Are you (in this last paragraph) saying that all those who have been burnt by fw_devlink during the last months and therefor run with it disabled will have a less fun experience once this is merged?

I guess potentially. So way back when this was originally submitted, some folks had trouble booting if it was set as a module due to it loading due to the deferred_probe_timeout expiring.
My attempts to change the default timeout value to be larger ran into trouble, but Saravana's fw_devlink does manage to resolve things properly for this case.

But if folks are having issues w/ fw_devlink, and have it disabled, and set QCOM_SCM=m they could still trip over the issue with the timeout firing before it is loaded (especially if they are loading modules from late mounted storage rather than ramdisk).

Quoted 2 lines

(I'm picking this up, but I don't fancy the idea that some people are turning the boot process into a lottery)

Me neither, and I definitely think the deferred_probe_timeout logic is way too fragile, which is why I'm eager for fw_devlink as it's a much less racy approach to handling module loading dependencies.

So if you
want to hold on this, while any remaining fw_devlink issues get sorted, that's fine. But I'd also not cast too much ire at fw_devlink, as the global probe timeout approach for handling optional links isn't great, and we need a better solution.

Quoted 12 lines · John Stultz

Quoted 10 lines · Bjorn Andersson

Quoted 9 lines · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

Are you (in this last paragraph) saying that all those who have been burnt by fw_devlink during the last months and therefor run with it disabled will have a less fun experience once this is merged?

Quoted 8 lines

I guess potentially. So way back when this was originally submitted, some folks had trouble booting if it was set as a module due to it loading due to the deferred_probe_timeout expiring.
My attempts to change the default timeout value to be larger ran into trouble, but Saravana's fw_devlink does manage to resolve things properly for this case.

Unfortunately I see really weird things coming out of that, e.g. display on my db845c is waiting for the USB hub on PCIe to load its firmware, which typically times out after 60 seconds.

I've stared at it quite a bit and I don't understand how they are related.

Quoted 5 lines

But if folks are having issues w/ fw_devlink, and have it disabled, and set QCOM_SCM=m they could still trip over the issue with the timeout firing before it is loaded (especially if they are loading modules from late mounted storage rather than ramdisk).

I guess we'll have to force QCOM_SCM=y in the defconfig and hope people don't make it =m.

Quoted 6 lines

Quoted 2 lines

(I'm picking this up, but I don't fancy the idea that some people are turning the boot process into a lottery)

Me neither, and I definitely think the deferred_probe_timeout logic is way too fragile, which is why I'm eager for fw_devlink as it's a much less racy approach to handling module loading dependencies.

Right, deferred_probe_timeout is the main issue here. Without it we might get some weird probe deferral runs, but either some driver is missing or it settles eventually.

With deferred_probe_timeout it's rather common for me to see things end up probe out of order (even more now with fw_devlink finding cyclic dependencies) and deferred_probe_timeout just breaking things.

Quoted 7 lines

So if you
want to hold on this, while any remaining fw_devlink issues get sorted, that's fine. But I'd also not cast too much ire at fw_devlink, as the global probe timeout approach for handling optional links isn't great, and we need a better solution.

There's no end to the possible and valid ways you can setup your defconfig and run into the probe deferral issues, so I see no point in holding this one back any longer. I just hope that one day it will be possible to boot the upstream kernel in a reliable fashion.

Quoted 15 lines · Bjorn Andersson

Quoted 12 lines · John Stultz

Quoted 10 lines · Bjorn Andersson

Quoted 9 lines · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

Are you (in this last paragraph) saying that all those who have been burnt by fw_devlink during the last months and therefor run with it disabled will have a less fun experience once this is merged?

Bjorn,

I jump in and help with any reports of issues with fw_devlink if I'm cc'ed. Please feel free to add me and I'll help fix any issues you have with fw_devlink=on.

Quoted 9 lines

Quoted 8 lines

I guess potentially. So way back when this was originally submitted, some folks had trouble booting if it was set as a module due to it loading due to the deferred_probe_timeout expiring.
My attempts to change the default timeout value to be larger ran into trouble, but Saravana's fw_devlink does manage to resolve things properly for this case.

Quoted 6 lines

Unfortunately I see really weird things coming out of that, e.g. display on my db845c is waiting for the USB hub on PCIe to load its firmware, which typically times out after 60 seconds.

I've stared at it quite a bit and I don't understand how they are related.

Can you please add me to any email thread with the details? I'd be happy to help.

First step is to make sure all the devices probe as with fw_devlink=permissive. After that if you are still seeing issues, it's generally timing issues in the driver. But if the actual timing issue is identified (by you or whoever knows the driver seeing the issue), then I can help with fixes or suggestions for fixes.

Quoted 16 lines

Quoted 5 lines

But if folks are having issues w/ fw_devlink, and have it disabled, and set QCOM_SCM=m they could still trip over the issue with the timeout firing before it is loaded (especially if they are loading modules from late mounted storage rather than ramdisk).

I guess we'll have to force QCOM_SCM=y in the defconfig and hope people don't make it =m.

Quoted 6 lines

Quoted 2 lines

(I'm picking this up, but I don't fancy the idea that some people are turning the boot process into a lottery)

Me neither, and I definitely think the deferred_probe_timeout logic is way too fragile, which is why I'm eager for fw_devlink as it's a much less racy approach to handling module loading dependencies.

Quoted 7 lines

Right, deferred_probe_timeout is the main issue here. Without it we might get some weird probe deferral runs, but either some driver is missing or it settles eventually.

With deferred_probe_timeout it's rather common for me to see things end up probe out of order (even more now with fw_devlink finding cyclic dependencies) and deferred_probe_timeout just breaking things.

Again, please CC me on these threads and I'd be happy to help.

Quoted 8 lines

Quoted 7 lines

So if you
want to hold on this, while any remaining fw_devlink issues get sorted, that's fine. But I'd also not cast too much ire at fw_devlink, as the global probe timeout approach for handling optional links isn't great, and we need a better solution.

Quoted 4 lines

There's no end to the possible and valid ways you can setup your defconfig and run into the probe deferral issues, so I see no point in holding this one back any longer. I just hope that one day it will be possible to boot the upstream kernel in a reliable fashion.

Might not be believable, but I'm hoping fw_devlink helps you meet this goal :)

Allow the qcom_scm driver to be loadable as a permenent module.

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

Quoted 1 line · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This feels like a regression, it should be allowed to be a module.

Quoted 7 lines

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 2 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

fw_devlink was supposed to resolve these issues and _allow_ code to be built as modules and not forced to be built into the kernel.

Quoted 5 lines · Greg Kroah-Hartman

Quoted 1 line · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This feels like a regression, it should be allowed to be a module.

I'm sorry, I'm not sure I'm following you, Greg. This patch is trying to enable the driver to be able to be loaded as a module.

Quoted 8 lines

Quoted 8 lines

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 5 lines

Quoted 2 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

fw_devlink was supposed to resolve these issues and _allow_ code to be built as modules and not forced to be built into the kernel.

Right. I'm re-submitting this patch to enable a driver to work as a module, because earlier attempts to submit it ran into boot trouble because fw_devlink wasn't yet enabled.

I worry something in my description made it seem otherwise, so let me know how you read it and I'll try to avoid such confusion in the future.

thanks
-john

Quoted 9 lines · John Stultz

Quoted 5 lines · Greg Kroah-Hartman

Quoted 1 line · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This feels like a regression, it should be allowed to be a module.

Quoted 2 lines

I'm sorry, I'm not sure I'm following you, Greg. This patch is trying to enable the driver to be able to be loaded as a module.

I think the mix up might be that Greg mentally read "permanent module" as "builtin"?

"permanent module" is just something that can't be unloaded once it's loaded. It's not "builtin".

-Saravana

Quoted 24 lines

Quoted 8 lines

Quoted 8 lines

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 5 lines

Quoted 2 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

fw_devlink was supposed to resolve these issues and _allow_ code to be built as modules and not forced to be built into the kernel.

Right. I'm re-submitting this patch to enable a driver to work as a module, because earlier attempts to submit it ran into boot trouble because fw_devlink wasn't yet enabled.

I worry something in my description made it seem otherwise, so let me know how you read it and I'll try to avoid such confusion in the future.

thanks
-john

Quoted 16 lines · Saravana Kannan

Quoted 11 lines · John Stultz

Quoted 5 lines · Greg Kroah-Hartman

Quoted 1 line · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This feels like a regression, it should be allowed to be a module.

I'm sorry, I'm not sure I'm following you, Greg. This patch is trying to enable the driver to be able to be loaded as a module.

I think the mix up might be that Greg mentally read "permanent module" as "builtin"?

Quoted 3 lines

"permanent module" is just something that can't be unloaded once it's loaded. It's not "builtin".

Afaict there's nothing in this patch that makes it more or less permanent.

The module will be quite permanent (in practice) because several other core modules reference symbols in the qcom_scm module.

But thanks to a previous patch, the qcom_scm device comes with suppress_bind_attrs, to prevent that the device goes away from a simple unbind operation - which the API and client drivers aren't designed to handle.

So, it would have been better in this case to omit the word "permanent" from the commit message, but the change is good and I don't want to rebase my tree to drop that word.

Thanks,
Bjorn

Quoted 26 lines

-Saravana

Quoted 24 lines

Quoted 8 lines

Quoted 8 lines

This still uses the "depends on QCOM_SCM || !QCOM_SCM" bit to ensure that drivers that call into the qcom_scm driver are also built as modules. While not ideal in some cases its the only safe way I can find to avoid build errors without having those drivers select QCOM_SCM and have to force it on (as QCOM_SCM=n can be valid for those drivers).

Quoted 5 lines

Quoted 2 lines

Reviving this now that Saravana's fw_devlink defaults to on, which should avoid loading troubles seen before.

fw_devlink was supposed to resolve these issues and _allow_ code to be built as modules and not forced to be built into the kernel.

Right. I'm re-submitting this patch to enable a driver to work as a module, because earlier attempts to submit it ran into boot trouble because fw_devlink wasn't yet enabled.

I worry something in my description made it seem otherwise, so let me know how you read it and I'll try to avoid such confusion in the future.

thanks
-john

Quoted 23 lines · Bjorn Andersson

Quoted 19 lines · Saravana Kannan

Quoted 11 lines · John Stultz

Quoted 5 lines · Greg Kroah-Hartman

Quoted 1 line · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This feels like a regression, it should be allowed to be a module.

I'm sorry, I'm not sure I'm following you, Greg. This patch is trying to enable the driver to be able to be loaded as a module.

I think the mix up might be that Greg mentally read "permanent module" as "builtin"?

"permanent module" is just something that can't be unloaded once it's loaded. It's not "builtin".

Quoted 2 lines

Afaict there's nothing in this patch that makes it more or less permanent.

The lack of a module_exit() makes it a permanent module. If you do lsmod, it'll mark this as "[permanent]".

Quoted 5 lines · Greg Kroah-Hartman

Quoted 1 line · John Stultz

Allow the qcom_scm driver to be loadable as a permenent module.

This feels like a regression, it should be allowed to be a module.

I'm sorry, I'm not sure I'm following you, Greg. This patch is trying to enable the driver to be able to be loaded as a module.