[RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies

[RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies

[Arnd Bergmann]

qcom_smd_register_edge() is provided by either QCOM_SMD or RPMSG_QCOM_SMD,
and if both of them are disabled, it does nothing.

The check for the PIL drivers however only checks for QCOM_SMD, so it breaks
with QCOM_SMD=n && RPMSG_QCOM_SMD=m:

drivers/remoteproc/built-in.o: In function `smd_subdev_remove':
qcom_wcnss_iris.c:(.text+0x231c): undefined reference to `qcom_smd_unregister_edge'
drivers/remoteproc/built-in.o: In function `smd_subdev_probe':
qcom_wcnss_iris.c:(.text+0x2344): undefined reference to `qcom_smd_register_edge'
drivers/remoteproc/built-in.o: In function `smd_subdev_probe':
qcom_q6v5_pil.c:(.text+0x3538): undefined reference to `qcom_smd_register_edge'
qcom_q6v5_pil.c:(.text+0x3538): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `qcom_smd_register_edge'

This clarifies the Kconfig dependency.

Fixes: 4b48921a8f74 ("remoteproc: qcom: Use common SMD edge handler")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Submitted on Feb 28, still needed in mainline as of v4.11-rc2
---
 drivers/remoteproc/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 65f86bc24c07..1dc43fc5f65f 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -76,7 +76,7 @@ config QCOM_ADSP_PIL
 	depends on OF && ARCH_QCOM
 	depends on REMOTEPROC
 	depends on QCOM_SMEM
-	depends on QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n)
+	depends on RPMSG_QCOM_SMD || QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n && RPMSG_QCOM_SMD=n)
 	select MFD_SYSCON
 	select QCOM_MDT_LOADER
 	select QCOM_RPROC_COMMON
@@ -93,7 +93,7 @@ config QCOM_Q6V5_PIL
 	depends on OF && ARCH_QCOM
 	depends on QCOM_SMEM
 	depends on REMOTEPROC
-	depends on QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n)
+	depends on RPMSG_QCOM_SMD || QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n && RPMSG_QCOM_SMD=n)
 	select MFD_SYSCON
 	select QCOM_RPROC_COMMON
 	select QCOM_SCM
@@ -104,7 +104,7 @@ config QCOM_Q6V5_PIL
 config QCOM_WCNSS_PIL
 	tristate "Qualcomm WCNSS Peripheral Image Loader"
 	depends on OF && ARCH_QCOM
-	depends on QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n)
+	depends on RPMSG_QCOM_SMD || QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n && RPMSG_QCOM_SMD=n)
 	depends on QCOM_SMEM
 	depends on REMOTEPROC
 	select QCOM_MDT_LOADER
-- 
2.9.0

Re: [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies

[Jean Delvare]

Hi Arnd,

On Mon, 13 Mar 2017 17:36:25 +0100, Arnd Bergmann wrote:
> qcom_smd_register_edge() is provided by either QCOM_SMD or RPMSG_QCOM_SMD,
> and if both of them are disabled, it does nothing.

Actually the code itself looks wrong to me. There are two sets of stubs
for qcom_smd_register_edge() and qcom_smd_unregister_edge() when the
feature is disabled, one from include/linux/rpmsg/qcom_smd.h and one
from include/linux/soc/qcom/smd.h. They have different definitions, and
different conditions. The former is declared if neither backend is
selected, while the latter is declared if QCOM_SMD isn't selected
(regardless of the value of RPMSG_QCOM_SMD.)

So as it stands, QCOM_SMD=n && RPMSG_QCOM_SMD=m leads to 2
implementations of these functions, inline stubs from
include/linux/soc/qcom/smd.h and actual implementations from
drivers/rpmsg/qcom_smd.c. I know nothing about these drivers but this
looks needlessly complex and error-prone to me. The stubs should be
declared only once and only when no actual implementations are
available. That is, assuming they are really supposed to be the same
and it's not an unfortunate name collision.

> The check for the PIL drivers however only checks for QCOM_SMD, so it breaks
> with QCOM_SMD=n && RPMSG_QCOM_SMD=m:
> 
> drivers/remoteproc/built-in.o: In function `smd_subdev_remove':
> qcom_wcnss_iris.c:(.text+0x231c): undefined reference to `qcom_smd_unregister_edge'
> drivers/remoteproc/built-in.o: In function `smd_subdev_probe':
> qcom_wcnss_iris.c:(.text+0x2344): undefined reference to `qcom_smd_register_edge'
> drivers/remoteproc/built-in.o: In function `smd_subdev_probe':
> qcom_q6v5_pil.c:(.text+0x3538): undefined reference to `qcom_smd_register_edge'
> qcom_q6v5_pil.c:(.text+0x3538): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `qcom_smd_register_edge'
> 
> This clarifies the Kconfig dependency.
> 
> Fixes: 4b48921a8f74 ("remoteproc: qcom: Use common SMD edge handler")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Submitted on Feb 28, still needed in mainline as of v4.11-rc2
> ---
>  drivers/remoteproc/Kconfig | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 65f86bc24c07..1dc43fc5f65f 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -76,7 +76,7 @@ config QCOM_ADSP_PIL
>  	depends on OF && ARCH_QCOM
>  	depends on REMOTEPROC
>  	depends on QCOM_SMEM
> -	depends on QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n)
> +	depends on RPMSG_QCOM_SMD || QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n && RPMSG_QCOM_SMD=n)
>  	select MFD_SYSCON
>  	select QCOM_MDT_LOADER
>  	select QCOM_RPROC_COMMON
> @@ -93,7 +93,7 @@ config QCOM_Q6V5_PIL
>  	depends on OF && ARCH_QCOM
>  	depends on QCOM_SMEM
>  	depends on REMOTEPROC
> -	depends on QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n)
> +	depends on RPMSG_QCOM_SMD || QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n && RPMSG_QCOM_SMD=n)
>  	select MFD_SYSCON
>  	select QCOM_RPROC_COMMON
>  	select QCOM_SCM
> @@ -104,7 +104,7 @@ config QCOM_Q6V5_PIL
>  config QCOM_WCNSS_PIL
>  	tristate "Qualcomm WCNSS Peripheral Image Loader"
>  	depends on OF && ARCH_QCOM
> -	depends on QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n)
> +	depends on RPMSG_QCOM_SMD || QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n && RPMSG_QCOM_SMD=n)
>  	depends on QCOM_SMEM
>  	depends on REMOTEPROC
>  	select QCOM_MDT_LOADER

I don't think the COMPILE_TEST adds any value here. The whole set of
drivers is architecture specific anyway so you won't gain much build
test coverage. It may even prevent a legitimate combination of options
on the intended target, if the feature provided by QCOM_SMD and
RPMSG_QCOM_SMD is optional (if not, I would suggest to drop all the
stubs and simply depend on RPMSG_QCOM_SMD || QCOM_SMD, for the sake of
simplicity.)

Don't get me wrong, I am a big fan of COMPILE_TEST, but only when we
can use it to get build testing coverage for free. If you have to change
the code itself in order to be able to get the extra build testing
coverage, I don't think it is a good idea. The kernel and the Kconfig
dependencies can be complex enough as is.

-- 
Jean Delvare
SUSE L3 Support

Re: [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies

[Arnd Bergmann]

On Tue, Mar 14, 2017 at 10:05 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Arnd,
>
> On Mon, 13 Mar 2017 17:36:25 +0100, Arnd Bergmann wrote:
>> qcom_smd_register_edge() is provided by either QCOM_SMD or RPMSG_QCOM_SMD,
>> and if both of them are disabled, it does nothing.
>
> Actually the code itself looks wrong to me. There are two sets of stubs
> for qcom_smd_register_edge() and qcom_smd_unregister_edge() when the
> feature is disabled, one from include/linux/rpmsg/qcom_smd.h and one
> from include/linux/soc/qcom/smd.h. They have different definitions, and
> different conditions. The former is declared if neither backend is
> selected, while the latter is declared if QCOM_SMD isn't selected
> (regardless of the value of RPMSG_QCOM_SMD.)

This driver always includes the former header (linux/rpmsg/qcom_smd.h),
which has both checks. The second header should be removed as soon
as we have moved over the users to the new one.

> So as it stands, QCOM_SMD=n && RPMSG_QCOM_SMD=m leads to 2
> implementations of these functions, inline stubs from
> include/linux/soc/qcom/smd.h and actual implementations from
> drivers/rpmsg/qcom_smd.c.

QCOM_SMD=n && RPMSG_QCOM_SMD=m should result in no code
including include/linux/soc/qcom/smd.h, and I don't get any other
failures here.

 I know nothing about these drivers but this
> looks needlessly complex and error-prone to me. The stubs should be
> declared only once and only when no actual implementations are
> available. That is, assuming they are really supposed to be the same
> and it's not an unfortunate name collision.

Agreed, but I think fixing that can be a separate effort. I don't
know what Bjorn's time frame is for removing the soc/qcom/smd
driver, but I'd guess we can already merge the headers into
one as a first step.

>> @@ -104,7 +104,7 @@ config QCOM_Q6V5_PIL
>>  config QCOM_WCNSS_PIL
>>       tristate "Qualcomm WCNSS Peripheral Image Loader"
>>       depends on OF && ARCH_QCOM
>> -     depends on QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n)
>> +     depends on RPMSG_QCOM_SMD || QCOM_SMD || (COMPILE_TEST && QCOM_SMD=n && RPMSG_QCOM_SMD=n)
>>       depends on QCOM_SMEM
>>       depends on REMOTEPROC
>>       select QCOM_MDT_LOADER
>
> I don't think the COMPILE_TEST adds any value here. The whole set of
> drivers is architecture specific anyway so you won't gain much build
> test coverage. It may even prevent a legitimate combination of options
> on the intended target, if the feature provided by QCOM_SMD and
> RPMSG_QCOM_SMD is optional (if not, I would suggest to drop all the
> stubs and simply depend on RPMSG_QCOM_SMD || QCOM_SMD, for the sake of
> simplicity.)
>
> Don't get me wrong, I am a big fan of COMPILE_TEST, but only when we
> can use it to get build testing coverage for free. If you have to change
> the code itself in order to be able to get the extra build testing
> coverage, I don't think it is a good idea. The kernel and the Kconfig
> dependencies can be complex enough as is.

I think the problem is the ARCH_QCOM dependency here, which
clearly gets in the way of COMPILE_TEST having any real effect.

I think generally speaking we either want to run the code and need both
ARCH_QCOM and (RPMSG_QCOM_SMD || QCOM_SMD), or we do
build-testing and need (COMPILE_TEST && RPMSG_QCOM_SMD=n
&& QCOM_SMD=n). We could add another Kconfig symbol that
captures the dependency and then just add a simple dependency
here.

      Arnd

Re: [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies

[Bjorn Andersson]

On Tue 14 Mar 04:01 PDT 2017, Arnd Bergmann wrote:

> On Tue, Mar 14, 2017 at 10:05 AM, Jean Delvare <jdelvare@suse.de> wrote:
[..]
> > I don't think the COMPILE_TEST adds any value here. The whole set of
> > drivers is architecture specific anyway so you won't gain much build
> > test coverage. It may even prevent a legitimate combination of options
> > on the intended target, if the feature provided by QCOM_SMD and
> > RPMSG_QCOM_SMD is optional (if not, I would suggest to drop all the
> > stubs and simply depend on RPMSG_QCOM_SMD || QCOM_SMD, for the sake of
> > simplicity.)
> >
> > Don't get me wrong, I am a big fan of COMPILE_TEST, but only when we
> > can use it to get build testing coverage for free. If you have to change
> > the code itself in order to be able to get the extra build testing
> > coverage, I don't think it is a good idea. The kernel and the Kconfig
> > dependencies can be complex enough as is.
> 
> I think the problem is the ARCH_QCOM dependency here, which
> clearly gets in the way of COMPILE_TEST having any real effect.
> 
> I think generally speaking we either want to run the code and need both
> ARCH_QCOM and (RPMSG_QCOM_SMD || QCOM_SMD), or we do
> build-testing and need (COMPILE_TEST && RPMSG_QCOM_SMD=n
> && QCOM_SMD=n). We could add another Kconfig symbol that
> captures the dependency and then just add a simple dependency
> here.

The drivers doesn't depend on SMD for loading and booting the
remoteprocs, the dependency is just to get the appropriate
functions/stubs - although currently all firmware I've seen requires SMD
to be in place.

I'll push forward with purging QCOM_SMD and I'll have a proper review of
the dependencies as part of/following that process.

I believe it makes more sense to check for ARCH_QCOM || COMPILE_TEST and
just have the others to check if we have each dependency build-in or
module.


I forwarded your patch to Linus for v4.11, thanks.

Regards,
Bjorn