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;
MIME-Version: 1.0
References: <20230601213246.3271412-1-arnd@kernel.org> <ee67348af01d729a959563f5cb2ecab7534f2e53.camel@intel.com>
 <0d627109-483d-42c2-86c7-337c2d38fadb@app.fastmail.com>
In-Reply-To: <0d627109-483d-42c2-86c7-337c2d38fadb@app.fastmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 2 Jun 2023 18:55:12 +0200
Message-ID: <CAJZ5v0iP+cmcoigiGwv58Hf8_3iM-+_5KZbAqiZyjqZxfBQR6A@mail.gmail.com>
Subject: Re: [PATCH] powercap: intel_rapl: fix CONFIG_IOSF_MBI dependency
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Zhang Rui <rui.zhang@intel.com>, Arnd Bergmann <arnd@kernel.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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