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

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

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

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

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
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: Marc Zyngier <maz@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Maulik Shah <mkshah@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
Acked-by: Kalle Valo <kvalo@codeaurora.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Will Deacon <will@kernel.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v3:
* Fix __arm_smccc_smc build issue reported by
  kernel test robot <lkp@intel.com>
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(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index db0ea2d2d75a..af53778edc7e 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -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
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 5e013b6a3692..523173cbff33 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 ee9cb545e73b..bb9ce3f92931 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -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");
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 07b7c25cbed8..f61516c17589 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -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
diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig
index 40f91bc8514d..741289e385d5 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -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
-- 
2.25.1

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Bjorn Andersson]

On Tue 06 Jul 23:53 CDT 2021, John Stultz wrote:

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

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)

Regards,
Bjorn

> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> 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: Marc Zyngier <maz@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: Maulik Shah <mkshah@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
> Acked-by: Kalle Valo <kvalo@codeaurora.org>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Will Deacon <will@kernel.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v3:
> * Fix __arm_smccc_smc build issue reported by
>   kernel test robot <lkp@intel.com>
> 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(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index db0ea2d2d75a..af53778edc7e 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -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
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 5e013b6a3692..523173cbff33 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 ee9cb545e73b..bb9ce3f92931 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -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");
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 07b7c25cbed8..f61516c17589 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -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
> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig
> index 40f91bc8514d..741289e385d5 100644
> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -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
> -- 
> 2.25.1
> 

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[John Stultz]

On Fri, Jul 16, 2021 at 10:01 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Tue 06 Jul 23:53 CDT 2021, John Stultz wrote:
> > 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.
> >
>
> 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).

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

thanks
-john

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Bjorn Andersson]

On Mon 19 Jul 14:00 CDT 2021, John Stultz wrote:

> On Fri, Jul 16, 2021 at 10:01 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Tue 06 Jul 23:53 CDT 2021, John Stultz wrote:
> > > 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.
> > >
> >
> > 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.
> 

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.

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

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

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

Thanks,
Bjorn

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Saravana Kannan]

On Mon, Jul 19, 2021 at 12:36 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 19 Jul 14:00 CDT 2021, John Stultz wrote:
>
> > On Fri, Jul 16, 2021 at 10:01 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > > On Tue 06 Jul 23:53 CDT 2021, John Stultz wrote:
> > > > 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.
> > > >
> > >
> > > 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.

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

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.

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

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

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

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

-Saravana

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Bjorn Andersson]

On Mon 19 Jul 16:53 CDT 2021, Saravana Kannan wrote:

> On Mon, Jul 19, 2021 at 12:36 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Mon 19 Jul 14:00 CDT 2021, John Stultz wrote:
> >
> > > On Fri, Jul 16, 2021 at 10:01 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > > On Tue 06 Jul 23:53 CDT 2021, John Stultz wrote:
> > > > > 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.
> > > > >
> > > >
> > > > 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.
> 

Thanks Saravana, unfortunately I've only heard these reports second hand
so far, not been able to reproduce them on my own. I appreciate your
support and will certainly reach out if I need some assistance.

> > >
> > > 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.
> 
> 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.
> 
> > > 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.
> >
> > > > (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.
> 
> Again, please CC me on these threads and I'd be happy to help.
> 
> >
> > > 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.
> 
> Might not be believable, but I'm hoping fw_devlink helps you meet this goal :)
> 

Sounds good, I hope so too :)

Regards,
Bjorn

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Greg Kroah-Hartman]

On Wed, Jul 07, 2021 at 04:53:20AM +0000, John Stultz wrote:
> 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.

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

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.

Let's not go backwards please.

thanks,

greg k-h

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[John Stultz]

On Wed, Jul 21, 2021 at 4:54 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 07, 2021 at 04:53:20AM +0000, John Stultz wrote:
> > 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.

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

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Saravana Kannan]

On Wed, Jul 21, 2021 at 10:24 AM John Stultz <john.stultz@linaro.org> wrote:
>
> On Wed, Jul 21, 2021 at 4:54 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jul 07, 2021 at 04:53:20AM +0000, John Stultz wrote:
> > > 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".

-Saravana

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

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Bjorn Andersson]

On Wed 21 Jul 13:00 CDT 2021, Saravana Kannan wrote:

> On Wed, Jul 21, 2021 at 10:24 AM John Stultz <john.stultz@linaro.org> wrote:
> >
> > On Wed, Jul 21, 2021 at 4:54 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jul 07, 2021 at 04:53:20AM +0000, John Stultz wrote:
> > > > 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".
> 

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

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

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Saravana Kannan]

On Wed, Jul 21, 2021 at 1:23 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 21 Jul 13:00 CDT 2021, Saravana Kannan wrote:
>
> > On Wed, Jul 21, 2021 at 10:24 AM John Stultz <john.stultz@linaro.org> wrote:
> > >
> > > On Wed, Jul 21, 2021 at 4:54 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Jul 07, 2021 at 04:53:20AM +0000, John Stultz wrote:
> > > > > 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".
> >
>
> 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]".

-Saravana

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

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Bjorn Andersson]

On Wed 21 Jul 16:07 CDT 2021, Saravana Kannan wrote:

> On Wed, Jul 21, 2021 at 1:23 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 21 Jul 13:00 CDT 2021, Saravana Kannan wrote:
> >
> > > On Wed, Jul 21, 2021 at 10:24 AM John Stultz <john.stultz@linaro.org> wrote:
> > > >
> > > > On Wed, Jul 21, 2021 at 4:54 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Jul 07, 2021 at 04:53:20AM +0000, John Stultz wrote:
> > > > > > 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".
> > >
> >
> > 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]".
> 

Cool, I didn't know that.

Thanks,
Bjorn

Re: [PATCH] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

[Greg Kroah-Hartman]

On Wed, Jul 21, 2021 at 10:24:01AM -0700, John Stultz wrote:
> On Wed, Jul 21, 2021 at 4:54 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jul 07, 2021 at 04:53:20AM +0000, John Stultz wrote:
> > > 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.

Ah, sorry, you are right, my mistake.

nevermind...