linux-kernel - Re: [PATCH] powercap: intel_rapl: fix CONFIG_IOSF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <0d627109-483d-42c2-86c7-337c2d38fadb@app.fastmail.com>
Date:   Fri, 02 Jun 2023 11:11:01 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Zhang Rui" <rui.zhang@...el.com>,
        "Arnd Bergmann" <arnd@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>
Cc:     "Sudeep Holla" <sudeep.holla@....com>,
        "lukasz.luba@....com" <lukasz.luba@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Cristian Marussi" <cristian.marussi@....com>
Subject: Re: [PATCH] powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency

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@...db.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