| Message ID | 20230601213246.3271412-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency | expand |
Hi, Arnd, On Thu, 2023-06-01 at 23:32 +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > 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' > 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 <rui.zhang@intel.com> 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. 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' Fixes: 3382388d7148 ("intel_rapl: abstract RAPL common code") Reported-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/powercap/Kconfig | 3 ++- drivers/powercap/intel_rapl_msr.c | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig index e71399804c14..adefdd8a4e12 100644 --- a/drivers/powercap/Kconfig +++ b/drivers/powercap/Kconfig @@ -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) diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c index cff5c6e8d570..b536144726f9 100644 --- a/drivers/powercap/intel_rapl_msr.c +++ b/drivers/powercap/intel_rapl_msr.c @@ -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>
On Fri, Jun 2, 2023, at 10:04, Zhang, Rui wrote: > On Thu, 2023-06-01 at 23:32 +0200, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> 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' >> > > 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, > > --- a/drivers/powercap/Kconfig > +++ b/drivers/powercap/Kconfig > @@ -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 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. >> 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? 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
On Fri, Jun 2, 2023 at 11:11 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, Jun 2, 2023, at 10:04, Zhang, Rui wrote: > > On Thu, 2023-06-01 at 23:32 +0200, Arnd Bergmann wrote: > >> From: Arnd Bergmann <arnd@arndb.de> > >> > >> 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' > >> > > > > 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, > > > > --- a/drivers/powercap/Kconfig > > +++ b/drivers/powercap/Kconfig > > @@ -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 > > 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. 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).
Hi, Rafael, On Fri, 2023-06-02 at 18:55 +0200, Rafael J. Wysocki wrote: > On Fri, Jun 2, 2023 at 11:11 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Fri, Jun 2, 2023, at 10:04, Zhang, Rui wrote: > > > On Thu, 2023-06-01 at 23:32 +0200, Arnd Bergmann wrote: > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > > > 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' > > > > > > > > > > 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, > > > > > > --- a/drivers/powercap/Kconfig > > > +++ b/drivers/powercap/Kconfig > > > @@ -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 > > > > 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. > > 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). Agreed. Even the previous RAPL code does not select PCI or depend on it. Let me refresh the patch and resend. thanks, rui
diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig index e71399804c143..21ad50b22d6b9 100644 --- a/drivers/powercap/Kconfig +++ b/drivers/powercap/Kconfig @@ -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