powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency

  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'

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 <arnd@arndb.de>
---
 drivers/powercap/Kconfig | 1 +
 1 file changed, 1 insertion(+)
  

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>
 
-- 
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
  

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