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

[Arnd Bergmann]

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