Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-447-ge2460e13b3-fm-20230525.001-ge2460e13
Mime-Version: 1.0
Message-Id: <0d627109-483d-42c2-86c7-337c2d38fadb@app.fastmail.com>
In-Reply-To: <ee67348af01d729a959563f5cb2ecab7534f2e53.camel@intel.com>
References: <20230601213246.3271412-1-arnd@kernel.org>
 <ee67348af01d729a959563f5cb2ecab7534f2e53.camel@intel.com>
Date:   Fri, 02 Jun 2023 11:11:01 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Zhang Rui" <rui.zhang@intel.com>,
        "Arnd Bergmann" <arnd@kernel.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>
Cc:     "Sudeep Holla" <sudeep.holla@arm.com>,
        "lukasz.luba@arm.com" <lukasz.luba@arm.com>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Cristian Marussi" <cristian.marussi@arm.com>
Subject: Re: [PATCH] powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency
Content-Type: text/plain
Precedence: bulk

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