* [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies @ 2017-03-13 16:36 Arnd Bergmann 2017-03-14 9:05 ` Jean Delvare 0 siblings, 1 reply; 4+ messages in thread From: Arnd Bergmann @ 2017-03-13 16:36 UTC (permalink / raw) To: Ohad Ben-Cohen, Bjorn Andersson Cc: Arnd Bergmann, Peter Griffin, Vinod Koul, Loic Pallardy, Pavel Machek, Jean Delvare, linux-remoteproc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies 2017-03-13 16:36 [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies Arnd Bergmann @ 2017-03-14 9:05 ` Jean Delvare 2017-03-14 11:01 ` Arnd Bergmann 0 siblings, 1 reply; 4+ messages in thread From: Jean Delvare @ 2017-03-14 9:05 UTC (permalink / raw) To: Arnd Bergmann Cc: Ohad Ben-Cohen, Bjorn Andersson, Peter Griffin, Vinod Koul, Loic Pallardy, Pavel Machek, linux-remoteproc, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies 2017-03-14 9:05 ` Jean Delvare @ 2017-03-14 11:01 ` Arnd Bergmann 2017-03-20 22:22 ` Bjorn Andersson 0 siblings, 1 reply; 4+ messages in thread From: Arnd Bergmann @ 2017-03-14 11:01 UTC (permalink / raw) To: Jean Delvare Cc: Ohad Ben-Cohen, Bjorn Andersson, Peter Griffin, Vinod Koul, Loic Pallardy, Pavel Machek, linux-remoteproc, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies 2017-03-14 11:01 ` Arnd Bergmann @ 2017-03-20 22:22 ` Bjorn Andersson 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Andersson @ 2017-03-20 22:22 UTC (permalink / raw) To: Arnd Bergmann Cc: Jean Delvare, Ohad Ben-Cohen, Peter Griffin, Vinod Koul, Loic Pallardy, Pavel Machek, linux-remoteproc, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-20 22:22 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-13 16:36 [RESEND PATCH] remoteproc: qcom: fix QCOM_SMD dependencies Arnd Bergmann 2017-03-14 9:05 ` Jean Delvare 2017-03-14 11:01 ` Arnd Bergmann 2017-03-20 22:22 ` Bjorn Andersson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).