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

[Zhang, Rui]

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