Re: [PATCH] powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency

[Zhang, Rui]

Hi, Arnd,

On Thu, 2023-06-01 at 23:32 +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@xxxxxxxx>
> 
> 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@xxxxxxxxx>
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@xxxxxxxx>
Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
---
 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>
 
-- 
2.25.1


> 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