powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency

PATCH In review Arnd Bergmann 4 participants 5 messages Jun 1, 2023 – Jun 6, 2023 Power management core

lore

Share & tools

lore

Arnd Bergmann

author

2023-06-01 21:32 UTC #0 lore raw

PATCH 1/1 powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency

From: Arnd Bergmann <[email protected]>

When the intel_rapl driver is built-in, but iosf_mbi is a loadable module, the kernel fails to link:

x86_64-linux-ld: vmlinux.o: in function `set_floor_freq_atom':
intel_rapl_common.c:(.text+0x2dac9b8): undefined reference to `iosf_mbi_write'
x86_64-linux-ld: intel_rapl_common.c:(.text+0x2daca66): undefined reference to `iosf_mbi_read'

The driver can work with iosf_mbi completely disabled, so add a dependency that still allows this configuration, but otherwise forces it to not be built-in when iosf_mbi is a loadable module.

Fixes 9eef7f9da928c ("powercap: intel_rapl: Introduce RAPL TPMI interface driver")

Signed-off-by Arnd Bergmann <[email protected]>

---
 drivers/powercap/Kconfig | 1 +
 1 file changed, 1 insertion(+)

drivers/powercap/Kconfig +1 -0

index e71399804c143..21ad50b22d6b9 100644
@@ -36,6 +36,7 @@ config INTEL_RAPL
 config INTEL_RAPL_TPMI
 	tristate "Intel RAPL Support via TPMI Interface"
 	depends on X86
+	depends on IOSF_MBI || IOSF_MBI=n
 	depends on INTEL_TPMI
 	select INTEL_RAPL_CORE
 	help

Signature · 1 line

2.39.2

Zhang, Rui

2023-06-02 08:04 UTC #1 lore raw

Hi, Arnd,

Quoted 6 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
When the intel_rapl driver is built-in, but iosf_mbi is a loadable module,
the kernel fails to link:

Quoted 6 lines

x86_64-linux-ld: vmlinux.o: in function `set_floor_freq_atom':
intel_rapl_common.c:(.text+0x2dac9b8): undefined reference to `iosf_mbi_write'
x86_64-linux-ld: intel_rapl_common.c:(.text+0x2daca66): undefined reference to `iosf_mbi_read'

IMO, it is the intel_rapl_common.c that calls IOSF APIs without specifying the dependency. Thus it should be fixed by something like below,

From 28de4c7d3d4f9fed75a7ecdcf5eea5b89ed77bab Mon Sep 17 00:00:00 2001
From: Zhang Rui <[email protected]>
Date: Fri, 2 Jun 2023 09:02:15 +0800
Subject: [PATCH] powercap/intel_rapl: Fix CONFIG_IOSF_MBI dependency

After commit 3382388d7148 ("intel_rapl: abstract RAPL common code"), accessing to IOSF interface is done in the RAPL common code.

Thus it is the CONFIG_INTEL_RAPL_CORE that has dependency of CONFIG_IOSF_MBI, while CONFIG_INTEL_RAPL_MSR does not.

This problem was not exposed previously because all the previous RAPL common code users, aka, the RAPL MSR and MMIO I/F drivers, have CONFIG_IOSF_MBI selected.

Fix the CONFIG_IOSF_MBI dependency in RAPL code. This also fixes a build time failure when the RAPL TPMI I/F driver is introduced without selecting CONFIG_IOSF_MBI.

Fixes 3382388d7148 ("intel_rapl: abstract RAPL common code")

Reported-by Arnd Bergmann <[email protected]>

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/powercap/Kconfig          | 3 ++-
 drivers/powercap/intel_rapl_msr.c | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/powercap/Kconfig +2 -1

index e71399804c14..adefdd8a4e12 100644
@@ -18,10 +18,11 @@ if POWERCAP
 # Client driver configurations go here.
 config INTEL_RAPL_CORE
 	tristate
+	select IOSF_MBI
 
 config INTEL_RAPL
 	tristate "Intel RAPL Support via MSR Interface"
-	depends on X86 && IOSF_MBI
+	depends on X86
 	select INTEL_RAPL_CORE
 	help
 	  This enables support for the Intel Running Average Power Limit (RAPL)

drivers/powercap/intel_rapl_msr.c +0 -1

index cff5c6e8d570..b536144726f9 100644
@@ -22,7 +22,6 @@
 #include <linux/processor.h>
 #include <linux/platform_device.h>
 
-#include <asm/iosf_mbi.h>
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>

Signature · 1 line

2.25.1

Quoted 5 lines

The driver can work with iosf_mbi completely disabled, so add a dependency
that still allows this configuration, but otherwise forces it to not be
built-in when iosf_mbi is a loadable module.

On the other side, I agree with you that the TPMI driver should work with iosf_mbi completely disabled.

A cleaner way to do this is to move the rapl_defaults setting (even the rapl_primitive_info setting) from intel_rapl_common.c to the I/F drivers, as this is really interface specific.

Maybe we can use the above patch as a quick fix, and remove the IOSF_MBI dependency from RAPL common code as a long term solution?

thanks,
rui

Arnd Bergmann

2023-06-02 09:12 UTC #2 lore raw

Quoted 23 lines · Rui

Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When the intel_rapl driver is built-in, but iosf_mbi is a loadable module,
the kernel fails to link:
Quoted 6 lines
x86_64-linux-ld: vmlinux.o: in function `set_floor_freq_atom':
intel_rapl_common.c:(.text+0x2dac9b8): undefined reference to `iosf_mbi_write'
x86_64-linux-ld: intel_rapl_common.c:(.text+0x2daca66): undefined reference to `iosf_mbi_read'
IMO, it is the intel_rapl_common.c that calls IOSF APIs without specifying the dependency. Thus it should be fixed by something like below,
drivers/powercap/Kconfig +0 -0
@@ -18,10 +18,11 @@ if POWERCAP
 # Client driver configurations go here.
 config INTEL_RAPL_CORE

drivers/powercap/Kconfig if POWERCAP · line 28

Quoted 10 lines

drivers/powercap/Kconfig +2 -1

 	tristate
+	select IOSF_MBI
 
 config INTEL_RAPL
 	tristate "Intel RAPL Support via MSR Interface"
-	depends on X86 && IOSF_MBI
+	depends on X86
 	select INTEL_RAPL_CORE
 	help
 	  This enables support for the Intel Running Average Power Limit

I think that has the logic slightly backwards from a usability point of view: The way I read the arch/x86/Kconfig description, IOSF_MBI is a feature of specific Intel hardware implementations, which gets enabled when any of these SoC platforms are enabled in the build, and the INTEL_RAPL driver specifically only works on those, while the new INTEL_RAPL_TPMI driver works on other hardware.

More generally speaking, I think it is a mistake for a device driver in one subsystem to use 'select' to enforce a build dependency on a driver in another subsystem when the other symbol is user-visible.

Quoted 9 lines

Quoted 5 lines
The driver can work with iosf_mbi completely disabled, so add a dependency
that still allows this configuration, but otherwise forces it to not be
built-in when iosf_mbi is a loadable module.
On the other side, I agree with you that the TPMI driver should work with iosf_mbi completely disabled.

Quoted 6 lines

A cleaner way to do this is to move the rapl_defaults setting (even the rapl_primitive_info setting) from intel_rapl_common.c to the I/F drivers, as this is really interface specific.
Maybe we can use the above patch as a quick fix, and remove the IOSF_MBI dependency from RAPL common code as a long term solution?

I agree that your long-term solution is the best way to avoid the build dependency, but for the short-term fix I think my patch makes a little more sense than yours.

Either approach is of course enough to address the build regression, so no objections to your patch if you still prefer that.

Arnd

Rafael J. Wysocki

maintainer

2023-06-02 16:55 UTC #3 lore raw

Quoted 44 lines · Arnd Bergmann

Quoted 23 lines · Rui
Quoted 6 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When the intel_rapl driver is built-in, but iosf_mbi is a loadable module,
the kernel fails to link:
Quoted 6 lines
x86_64-linux-ld: vmlinux.o: in function `set_floor_freq_atom':
intel_rapl_common.c:(.text+0x2dac9b8): undefined reference to `iosf_mbi_write'
x86_64-linux-ld: intel_rapl_common.c:(.text+0x2daca66): undefined reference to `iosf_mbi_read'
IMO, it is the intel_rapl_common.c that calls IOSF APIs without specifying the dependency. Thus it should be fixed by something like below,
drivers/powercap/Kconfig +0 -0
@@ -18,10 +18,11 @@ if POWERCAP
 # Client driver configurations go here.
 config INTEL_RAPL_CORE
drivers/powercap/Kconfig if POWERCAP · line 28
Quoted 10 lines
drivers/powercap/Kconfig +2 -1
      tristate
+     select IOSF_MBI
 
 config INTEL_RAPL
      tristate "Intel RAPL Support via MSR Interface"
-     depends on X86 && IOSF_MBI
+     depends on X86
      select INTEL_RAPL_CORE
      help
        This enables support for the Intel Running Average Power Limit
I think that has the logic slightly backwards from a usability point of view: The way I read the arch/x86/Kconfig description, IOSF_MBI is a feature of specific Intel hardware implementations, which gets enabled when any of these SoC platforms are enabled in the build, and the INTEL_RAPL driver specifically only works on those, while the new INTEL_RAPL_TPMI driver works on other hardware.

Quoted 4 lines

More generally speaking, I think it is a mistake for a device driver in one subsystem to use 'select' to enforce a build dependency on a driver in another subsystem when the other symbol is user-visible.

IOSF_MBI is already selected from multiple places and while you can argue that they are all mistakes, this particular new one would not be worse than any of them.

IMO it would be better if IOSF_MBI were not user-visible (and interestingly enough, whoever selects it should also select PCI or depend on it - I'm not really sure if that dependency is taken care of in all cases).

Zhang, Rui

2023-06-06 12:41 UTC #4 lore raw

Hi, Rafael,

Quoted 60 lines · Rafael J. Wysocki

Quoted 49 lines · Arnd Bergmann
Quoted 29 lines · Rui
Quoted 7 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When the intel_rapl driver is built-in, but iosf_mbi is a loadable
module,
the kernel fails to link:
Quoted 7 lines
x86_64-linux-ld: vmlinux.o: in function `set_floor_freq_atom':
intel_rapl_common.c:(.text+0x2dac9b8): undefined reference to `iosf_mbi_write'
x86_64-linux-ld: intel_rapl_common.c:(.text+0x2daca66):
undefined
reference to `iosf_mbi_read'
IMO, it is the intel_rapl_common.c that calls IOSF APIs without specifying the dependency. Thus it should be fixed by something like
below,
drivers/powercap/Kconfig +0 -0
@@ -18,10 +18,11 @@ if POWERCAP
  # Client driver configurations go here.
config INTEL_RAPL_CORE
tristate
+ select IOSF_MBI
Quoted 8 lines
config INTEL_RAPL
      tristate "Intel RAPL Support via MSR Interface" -     depends on X86 && IOSF_MBI
+     depends on X86
      select INTEL_RAPL_CORE
      help
        This enables support for the Intel Running Average Power Limit
I think that has the logic slightly backwards from a usability point
of view: The way I read the arch/x86/Kconfig description, IOSF_MBI is a feature of specific Intel hardware implementations, which gets enabled when any of these SoC platforms are enabled in the build, and the INTEL_RAPL driver specifically only works on those, while the new INTEL_RAPL_TPMI driver works on other hardware.
Quoted 4 lines
More generally speaking, I think it is a mistake for a device driver in one subsystem to use 'select' to enforce a build dependency on a driver in another subsystem when the other symbol is user-visible.
IOSF_MBI is already selected from multiple places and while you can argue that they are all mistakes, this particular new one would not be
worse than any of them.

Quoted 5 lines

IMO it would be better if IOSF_MBI were not user-visible (and interestingly enough, whoever selects it should also select PCI or depend on it - I'm not really sure if that dependency is taken care of
in all cases).

Agreed.
Even the previous RAPL code does not select PCI or depend on it.

Let me refresh the patch and resend.

thanks,
rui