Skip to contentSkip to conversation

interconnect: qcom: fix rpmh link failures

PATCH Applied Arnd Bergmann 3 participants 4 messages Dec 4, 2020 – Mar 1, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 interconnect: qcom: fix rpmh link failures

From: Arnd Bergmann <[email protected]>

When CONFIG_COMPILE_TEST is set, it is possible to build some of the interconnect drivers into the kernel while their dependencies are loadable modules, which is bad:

arm-linux-gnueabi-ld: drivers/interconnect/qcom/bcm-voter.o: in function `qcom_icc_bcm_voter_commit':
(.text+0x1f8): undefined reference to `rpmh_invalidate'
arm-linux-gnueabi-ld: (.text+0x20c): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: (.text+0x2b0): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: (.text+0x2e8): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: drivers/interconnect/qcom/icc-rpmh.o: in function `qcom_icc_bcm_init':
(.text+0x2ac): undefined reference to `cmd_db_read_addr'
arm-linux-gnueabi-ld: (.text+0x2c8): undefined reference to `cmd_db_read_aux_data'

The exact dependencies are a bit complicated, so split them out into a hidden Kconfig symbol that all drivers can in turn depend on to get it right.

Fixes 976daac4a1c5 ("interconnect: qcom: Consolidate interconnect RPMh support")
---
 drivers/interconnect/qcom/Kconfig | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)
drivers/interconnect/qcom/Kconfig +15 -8
index a8f93ba265f8..b3fb5b02bcf1 100644
@@ -42,13 +42,23 @@ config INTERCONNECT_QCOM_QCS404
 	  This is a driver for the Qualcomm Network-on-Chip on qcs404-based
 	  platforms.
 
+config INTERCONNECT_QCOM_RPMH_POSSIBLE
+	tristate
+	default INTERCONNECT_QCOM
+	depends on QCOM_RPMH || (COMPILE_TEST && !QCOM_RPMH)
+	depends on QCOM_COMMAND_DB || (COMPILE_TEST && !QCOM_COMMAND_DB)
+	depends on OF || COMPILE_TEST
+	help
+	  Compile-testing RPMH drivers is possible on other platforms,
+	  but in order to avoid link failures, drivers must not be built-in
+	  when QCOM_RPMH or QCOM_COMMAND_DB are loadable modules
+
 config INTERCONNECT_QCOM_RPMH
 	tristate
 
 config INTERCONNECT_QCOM_SC7180
 	tristate "Qualcomm SC7180 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
@@ -57,8 +67,7 @@ config INTERCONNECT_QCOM_SC7180
 
 config INTERCONNECT_QCOM_SDM845
 	tristate "Qualcomm SDM845 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
@@ -67,8 +76,7 @@ config INTERCONNECT_QCOM_SDM845
 
 config INTERCONNECT_QCOM_SM8150
 	tristate "Qualcomm SM8150 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
@@ -77,8 +85,7 @@ config INTERCONNECT_QCOM_SM8150
 
 config INTERCONNECT_QCOM_SM8250
 	tristate "Qualcomm SM8250 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
Signature · 1 line
2.27.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 15 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

When CONFIG_COMPILE_TEST is set, it is possible to build some of the interconnect drivers into the kernel while their dependencies are loadable modules, which is bad:

arm-linux-gnueabi-ld: drivers/interconnect/qcom/bcm-voter.o: in function `qcom_icc_bcm_voter_commit':
(.text+0x1f8): undefined reference to `rpmh_invalidate'
arm-linux-gnueabi-ld: (.text+0x20c): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: (.text+0x2b0): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: (.text+0x2e8): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: drivers/interconnect/qcom/icc-rpmh.o: in function `qcom_icc_bcm_init':
(.text+0x2ac): undefined reference to `cmd_db_read_addr'
arm-linux-gnueabi-ld: (.text+0x2c8): undefined reference to `cmd_db_read_aux_data'

Quoted 6 lines

The exact dependencies are a bit complicated, so split them out into a hidden Kconfig symbol that all drivers can in turn depend on to get it right.

Fixes 976daac4a1c5 ("interconnect: qcom: Consolidate interconnect RPMh support")

Your patch looks correct to me, so:

But we're going to have to sprinkle a handful of these throughout the tree and we're not a lot of people who "understand" what it does (and at least I keep getting them wrong...)

Perhaps it would be more reasonable to maintain this long term if we drop the possibility of compile testing these drivers independently of rpmh and command db? (I.e. drop the function stubs and rely on RPMH/COMMAND_DB to enable building under COMPILE_TEST)?

And just to make it clear, I think we should merge your patch to fix v5.11; then consider to simplify this past that.

Regards,
Bjorn

Quoted 67 lines
---
 drivers/interconnect/qcom/Kconfig | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)
drivers/interconnect/qcom/Kconfig +15 -8
index a8f93ba265f8..b3fb5b02bcf1 100644
@@ -42,13 +42,23 @@ config INTERCONNECT_QCOM_QCS404
 	  This is a driver for the Qualcomm Network-on-Chip on qcs404-based
 	  platforms.
 
+config INTERCONNECT_QCOM_RPMH_POSSIBLE
+	tristate
+	default INTERCONNECT_QCOM
+	depends on QCOM_RPMH || (COMPILE_TEST && !QCOM_RPMH)
+	depends on QCOM_COMMAND_DB || (COMPILE_TEST && !QCOM_COMMAND_DB)
+	depends on OF || COMPILE_TEST
+	help
+	  Compile-testing RPMH drivers is possible on other platforms,
+	  but in order to avoid link failures, drivers must not be built-in
+	  when QCOM_RPMH or QCOM_COMMAND_DB are loadable modules
+
 config INTERCONNECT_QCOM_RPMH
 	tristate
 
 config INTERCONNECT_QCOM_SC7180
 	tristate "Qualcomm SC7180 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
@@ -57,8 +67,7 @@ config INTERCONNECT_QCOM_SC7180
 
 config INTERCONNECT_QCOM_SDM845
 	tristate "Qualcomm SDM845 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
@@ -67,8 +76,7 @@ config INTERCONNECT_QCOM_SDM845
 
 config INTERCONNECT_QCOM_SM8150
 	tristate "Qualcomm SM8150 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
@@ -77,8 +85,7 @@ config INTERCONNECT_QCOM_SM8150
 
 config INTERCONNECT_QCOM_SM8250
 	tristate "Qualcomm SM8250 interconnect driver"
-	depends on INTERCONNECT_QCOM
-	depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
+	depends on INTERCONNECT_QCOM_RPMH_POSSIBLE
 	select INTERCONNECT_QCOM_RPMH
 	select INTERCONNECT_QCOM_BCM_VOTER
 	help
Signature · 1 line
2.27.0
#2 lore raw
Quoted 10 lines · Bjorn Andersson

On Fri 04 Dec 10:50 CST 2020, Arnd Bergmann wrote:

Your patch looks correct to me, so:

But we're going to have to sprinkle a handful of these throughout the tree and we're not a lot of people who "understand" what it does (and at least I keep getting them wrong...)

Right, I already ran into another one.

Quoted 4 lines

Perhaps it would be more reasonable to maintain this long term if we drop the possibility of compile testing these drivers independently of rpmh and command db? (I.e. drop the function stubs and rely on RPMH/COMMAND_DB to enable building under COMPILE_TEST)?

Agreed, I think that would be best. As long as RPMH and COMMAND_DB can individually be enabled for CONFIG_COMPILE_TEST, all the drivers will get enabled in an allmodconfig build, and we can just list the dependencies.

I don't really like the headers too much that have an #ifdef CONFIG_SUBSYSTEM with an alternative set of inline functions, unless there is a reasonable expectations that drivers work fine if that subsystem is disabled.

E.g. having the option to disable IPv6 makes sense despite the complexity that adds, but compile-testing a driver without a required subsystem that can also be enabled for compile-testing hurts more than it helps.

Quoted 2 lines

And just to make it clear, I think we should merge your patch to fix v5.11; then consider to simplify this past that.

Yes, sounds good.

Arnd

#3 lore raw
In reply to Arnd Bergmann
This patch was applied to qcom/linux.git (refs/heads/for-next):

Hello:

This patch was applied to qcom/linux.git (refs/heads/for-next):

Quoted 15 lines · On Fri, 4 Dec 2020 17:50:03 +0100 you wrote:

From: Arnd Bergmann <[email protected]>

When CONFIG_COMPILE_TEST is set, it is possible to build some of the interconnect drivers into the kernel while their dependencies are loadable modules, which is bad:

arm-linux-gnueabi-ld: drivers/interconnect/qcom/bcm-voter.o: in function `qcom_icc_bcm_voter_commit':
(.text+0x1f8): undefined reference to `rpmh_invalidate'
arm-linux-gnueabi-ld: (.text+0x20c): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: (.text+0x2b0): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: (.text+0x2e8): undefined reference to `rpmh_write_batch'
arm-linux-gnueabi-ld: drivers/interconnect/qcom/icc-rpmh.o: in function `qcom_icc_bcm_init':
(.text+0x2ac): undefined reference to `cmd_db_read_addr'
arm-linux-gnueabi-ld: (.text+0x2c8): undefined reference to `cmd_db_read_aux_data'

Quoted 1 line

[...]

Here is the summary with links:
- interconnect: qcom: fix rpmh link failures

    https://git.kernel.org/qcom/c/512d4a26abdb

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html