PCI/pwrctl: reduce the amount of Kconfig noise

Commit Message

Bartosz Golaszewski July 16, 2024, 3:23 p.m. UTC

From: Bartosz Golaszewski <[e-mail]>

Kconfig will ask the user twice about power sequencing: once for the QCom
WCN power sequencing driver and then again for the PCI power control
driver using it.

Let's remove the public menuconfig entry for PCI pwrctl and instead
default the relevant symbol to 'm' only for the architectures that
actually need it.

Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
Reported-by: Linus Torvalds <[e-mail]>
Closes: https://lore.kernel.org/lkml/CAHk-=[e-mail]/
Signed-off-by: Bartosz Golaszewski <[e-mail]>
---
 drivers/pci/pwrctl/Kconfig | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Manivannan Sadhasivam July 16, 2024, 3:59 p.m. UTC | #1

On Tue, Jul 16, 2024 at 05:23:18PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> Kconfig will ask the user twice about power sequencing: once for the QCom
> WCN power sequencing driver and then again for the PCI power control
> driver using it.
> 
> Let's remove the public menuconfig entry for PCI pwrctl and instead
> default the relevant symbol to 'm' only for the architectures that
> actually need it.
> 

Why can't you put it in defconfig instead?

- Mani

> Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
> Reported-by: Linus Torvalds <[e-mail]>
> Closes: https://lore.kernel.org/lkml/CAHk-=[e-mail]/
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> ---
>  drivers/pci/pwrctl/Kconfig | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> index f1b824955d4b..b8f289e6a185 100644
> --- a/drivers/pci/pwrctl/Kconfig
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -1,17 +1,10 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
> -menu "PCI Power control drivers"
> -
>  config PCI_PWRCTL
>  	tristate
>  
>  config PCI_PWRCTL_PWRSEQ
> -	tristate "PCI Power Control driver using the Power Sequencing subsystem"
> +	tristate
>  	select POWER_SEQUENCING
>  	select PCI_PWRCTL
>  	default m if ((ATH11K_PCI || ATH12K) && ARCH_QCOM)
> -	help
> -	  Enable support for the PCI power control driver for device
> -	  drivers using the Power Sequencing subsystem.
> -
> -endmenu
> -- 
> 2.43.0
> 
>

Bartosz Golaszewski July 16, 2024, 4:29 p.m. UTC | #2

On Tue, Jul 16, 2024 at 5:59 PM Manivannan Sadhasivam
<[e-mail]> wrote:
>
> On Tue, Jul 16, 2024 at 05:23:18PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > Kconfig will ask the user twice about power sequencing: once for the QCom
> > WCN power sequencing driver and then again for the PCI power control
> > driver using it.
> >
> > Let's remove the public menuconfig entry for PCI pwrctl and instead
> > default the relevant symbol to 'm' only for the architectures that
> > actually need it.
> >
>
> Why can't you put it in defconfig instead?
>

Only Qualcomm uses it right now. I don't think it's worth building it
for everyone just yet. Let's cross that bridge when we have more
platforms selecting it?

Bartosz

Manivannan Sadhasivam July 16, 2024, 4:33 p.m. UTC | #3

On Tue, Jul 16, 2024 at 06:29:04PM +0200, Bartosz Golaszewski wrote:
> On Tue, Jul 16, 2024 at 5:59 PM Manivannan Sadhasivam
> <[e-mail]> wrote:
> >
> > On Tue, Jul 16, 2024 at 05:23:18PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[e-mail]>
> > >
> > > Kconfig will ask the user twice about power sequencing: once for the QCom
> > > WCN power sequencing driver and then again for the PCI power control
> > > driver using it.
> > >
> > > Let's remove the public menuconfig entry for PCI pwrctl and instead
> > > default the relevant symbol to 'm' only for the architectures that
> > > actually need it.
> > >
> >
> > Why can't you put it in defconfig instead?
> >
> 
> Only Qualcomm uses it right now. I don't think it's worth building it
> for everyone just yet. Let's cross that bridge when we have more
> platforms selecting it?
> 

This is the same case for rest of the Qcom specific drivers as well, but we
enable it in ARM/ARM64 defconfig. So I think this driver should also follow the
same.

- Mani

Bartosz Golaszewski July 16, 2024, 5:38 p.m. UTC | #4

On Tue, Jul 16, 2024 at 6:33 PM Manivannan Sadhasivam
<[e-mail]> wrote:
>
> On Tue, Jul 16, 2024 at 06:29:04PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Jul 16, 2024 at 5:59 PM Manivannan Sadhasivam
> > <[e-mail]> wrote:
> > >
> > > On Tue, Jul 16, 2024 at 05:23:18PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <[e-mail]>
> > > >
> > > > Kconfig will ask the user twice about power sequencing: once for the QCom
> > > > WCN power sequencing driver and then again for the PCI power control
> > > > driver using it.
> > > >
> > > > Let's remove the public menuconfig entry for PCI pwrctl and instead
> > > > default the relevant symbol to 'm' only for the architectures that
> > > > actually need it.
> > > >
> > >
> > > Why can't you put it in defconfig instead?
> > >
> >
> > Only Qualcomm uses it right now. I don't think it's worth building it
> > for everyone just yet. Let's cross that bridge when we have more
> > platforms selecting it?
> >
>
> This is the same case for rest of the Qcom specific drivers as well, but we
> enable it in ARM/ARM64 defconfig. So I think this driver should also follow the
> same.
>

Ok, so let's do it in the next cycle.

Bart

Linus Torvalds July 16, 2024, 6:08 p.m. UTC | #5

On Tue, 16 Jul 2024 at 08:23, Bartosz Golaszewski <[e-mail]> wrote:
>
> Let's remove the public menuconfig entry for PCI pwrctl and instead
> default the relevant symbol to 'm' only for the architectures that
> actually need it.

This feels like you should just use "select" instead.

IOW, don't make PCI_PWRCTL_PWRSEQ a question at all. Instead, have the
drivers that need it just select it automatically.

It's much better to ask people "do you have hardware XYZ" that they
can hopefully answer, and then we enable all the things that hardware
needs.

In contrast, asking people "do you need support for ABC?" when they
don't know what ABC is is _not_ helpful.

IOW, when you write Kconfig entries, your rules should be:

 - NOTHING is ever enabled by default, unless it's an old feature that
was enabled before and got split out (so that "make oldconfig" gives a
working kernel)

 - you NEVER ask questions that normal people can't answer.

For example, we have *way* too many questions that come about because
some developer went "I don't know what the answer is, I'll just make
it a Kconfig option". And I absolutely *HATE* those questions. Dammit,
if the developer doesn't know, then a user sure as hell doesn't
either.

I tend to keep on harping on Kconfig issues, because I really do think
that it's one of the biggest hurdles for normal users to just build
their own kernels. We make the rest of the build system pretty damn
simple, with a simple "make" and then as root "make modules_install
install".

But the Kconfig phase is a complete disaster, and it's because kernel
developers don't seem to think about users.

              Linus

Bartosz Golaszewski July 16, 2024, 6:48 p.m. UTC | #6

On Tue, Jul 16, 2024 at 8:08 PM Linus Torvalds
<[e-mail]> wrote:
>
> On Tue, 16 Jul 2024 at 08:23, Bartosz Golaszewski <[e-mail]> wrote:
> >
> > Let's remove the public menuconfig entry for PCI pwrctl and instead
> > default the relevant symbol to 'm' only for the architectures that
> > actually need it.
>
> This feels like you should just use "select" instead.
>
> IOW, don't make PCI_PWRCTL_PWRSEQ a question at all. Instead, have the
> drivers that need it just select it automatically.
>

But this patch does it. PCI_PWRCTL_PWRSEQ becomes a hidden symbol and
the entire submenu for PCI_PWRCTL disappears. There's no question in
Kconfig anymore.

On the other hand there isn't really any driver that would require
this. It's a specific platform that needs additional handling of
resources before the PCI devices can be detected. This is why we do:

    default m if ((ATH11K_PCI || ATH12K) && ARCH_QCOM)

If we selected it from the ATH1[12]K entry then we'd be building it
for many platforms that don't need it.

Bartosz

Linus Torvalds July 16, 2024, 7:02 p.m. UTC | #7

On Tue, 16 Jul 2024 at 11:48, Bartosz Golaszewski <[e-mail]> wrote:
>
> But this patch does it. PCI_PWRCTL_PWRSEQ becomes a hidden symbol and
> the entire submenu for PCI_PWRCTL disappears. There's no question in
> Kconfig anymore.

Yes, but look here:

        default m if ((ATH11K_PCI || ATH12K) && ARCH_QCOM)

That has at least two issues:

 - what if ATH11K_PCI or ATH12K is built-in and needs this driver?
"default m" is *NOT* valid.

 - what happens when you add new drivers? You keep making this line
longer and more complicated?

See why I say "use select" instead? It means that the drivers that
need it can select it, and you avoid complicated "list X drivers"
things, but you can also get the *right* selection criteria, so that a
built-in driver will select a built-in PCI_PWRCTL_PWRSEQ option.

           Linus

Bartosz Golaszewski July 16, 2024, 8:10 p.m. UTC | #8

On Tue, 16 Jul 2024 at 21:02, Linus Torvalds
<[e-mail]> wrote:
>
> On Tue, 16 Jul 2024 at 11:48, Bartosz Golaszewski <[e-mail]> wrote:
> >
> > But this patch does it. PCI_PWRCTL_PWRSEQ becomes a hidden symbol and
> > the entire submenu for PCI_PWRCTL disappears. There's no question in
> > Kconfig anymore.
>
> Yes, but look here:
>
>         default m if ((ATH11K_PCI || ATH12K) && ARCH_QCOM)
>
> That has at least two issues:
>
>  - what if ATH11K_PCI or ATH12K is built-in and needs this driver?
> "default m" is *NOT* valid.
>
>  - what happens when you add new drivers? You keep making this line
> longer and more complicated?
>
> See why I say "use select" instead? It means that the drivers that
> need it can select it, and you avoid complicated "list X drivers"
> things, but you can also get the *right* selection criteria, so that a
> built-in driver will select a built-in PCI_PWRCTL_PWRSEQ option.
>
>            Linus

Should we do:

    select PCI_PWRCTL_PWRSEQ if ARCH_QCOM

in the ATH11K_PCI and ATH12K Kconfig entries? Am I getting this right?

Because unconditional select here makes no sense - 99,9% users of ATH
drivers don't need PCI_PWRCTL_PWRSEQ.

Or add a new symbol like ARCH_NEED_PWRCTL_PWRSEQ and select it from
ARCH_QCOM (and later from any other arch that will use it) and do:

    select PCI_PWRCTL_PWRSEQ if ARCH_NEED_PWRCTL_PWRSEQ

in ATH entries?

Bartosz

Linus Torvalds July 16, 2024, 8:29 p.m. UTC | #9

On Tue, 16 Jul 2024 at 13:10, Bartosz Golaszewski
<[e-mail]> wrote:
>
>     select PCI_PWRCTL_PWRSEQ if ARCH_QCOM
>
> in the ATH11K_PCI and ATH12K Kconfig entries? Am I getting this right?

So on the whole, I'd prefer these things to be done where they are
actually required.

But I'm not actually entirely sure what the hard _requirements_ from a
hardware - or a kernel build - standpoint actually are.

If there aren't any hard requirements at all, then maybe the whole "do
you want pweseq" should be an actual question (but limited only to
situations where it makes sense).

If the hard requirement is at the level of the driver itself, then the
"select" should be in the driver.

That doesn't seem to be the case here, since ATH11K_PCI certainly
works without it, but if that driver requires power sequencing on
ARCH_QCOM platforms, then maybe that is indeed the right thing.

And if the hard requirement comes from some SoC setup, then optimally
I think the "select" should be at that level, but we don't actually
seem to have that level of questions (but maybe something in

   drivers/soc/qcom/Kconfig

might make sense?)

Anyway, this is not necessarily something where there is "one correct
answer". This may be a somewhat gray area, and it looks like ARCH_QCOM
is a very big "any Qualcomm SoC" thing and not very specific.

So I'm not sure what the right answer is, but I *am* pretty sure of
what the wrong answer is. And this:

        default m if ((ATH11K_PCI || ATH12K) && ARCH_QCOM)

looks like it cannot possibly be right if ATH11K_PCI is built-in,
since then the probing of the device will happen long before that
PCI_PWRCTL_PWRSEQ module has been loaded.

And that doesn't sound sensible to me. Does it?

TL;DR:  I do think that the

      select PCI_PWRCTL_PWRSEQ if ARCH_QCOM

in the ATH11K_PCI and ATH12K Kconfig entries *may* be the right thing.
But again, I'm not actually 100% sure of the hard requirements here.

           Linus

Bartosz Golaszewski July 17, 2024, 12:31 p.m. UTC | #10

On Tue, Jul 16, 2024 at 10:29 PM Linus Torvalds
<[e-mail]> wrote:
>
> On Tue, 16 Jul 2024 at 13:10, Bartosz Golaszewski
> <[e-mail]> wrote:
> >
> >     select PCI_PWRCTL_PWRSEQ if ARCH_QCOM
> >
> > in the ATH11K_PCI and ATH12K Kconfig entries? Am I getting this right?
>
> So on the whole, I'd prefer these things to be done where they are
> actually required.
>
> But I'm not actually entirely sure what the hard _requirements_ from a
> hardware - or a kernel build - standpoint actually are.
>
> If there aren't any hard requirements at all, then maybe the whole "do
> you want pweseq" should be an actual question (but limited only to
> situations where it makes sense).
>
> If the hard requirement is at the level of the driver itself, then the
> "select" should be in the driver.
>
> That doesn't seem to be the case here, since ATH11K_PCI certainly
> works without it, but if that driver requires power sequencing on
> ARCH_QCOM platforms, then maybe that is indeed the right thing.
>
> And if the hard requirement comes from some SoC setup, then optimally
> I think the "select" should be at that level, but we don't actually
> seem to have that level of questions (but maybe something in
>
>    drivers/soc/qcom/Kconfig
>
> might make sense?)
>

The hard requirement really comes from the board level - not SoC. It's
the board that has the BT/WLAN module hardwired and - depending on how
the module is powered - may or may not require power sequencing. But I
don't think we have any per-board infrastructure in Kconfig.

> Anyway, this is not necessarily something where there is "one correct
> answer". This may be a somewhat gray area, and it looks like ARCH_QCOM
> is a very big "any Qualcomm SoC" thing and not very specific.
>
> So I'm not sure what the right answer is, but I *am* pretty sure of
> what the wrong answer is. And this:
>
>         default m if ((ATH11K_PCI || ATH12K) && ARCH_QCOM)
>
> looks like it cannot possibly be right if ATH11K_PCI is built-in,
> since then the probing of the device will happen long before that
> PCI_PWRCTL_PWRSEQ module has been loaded.
>
> And that doesn't sound sensible to me. Does it?
>
> TL;DR:  I do think that the
>
>       select PCI_PWRCTL_PWRSEQ if ARCH_QCOM
>
> in the ATH11K_PCI and ATH12K Kconfig entries *may* be the right thing.
> But again, I'm not actually 100% sure of the hard requirements here.
>
>            Linus

After sleeping on it I really think that it may be better to introduce
a more generic ARCH_HAVE_PCI_PWRCTL symbol so that we don't have to
update the ATH Kconfig entires for every new platform that needs it.
For want of a more fine-grained solution, we would select it from
ARCH_QCOM.

Bart

Patch

diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
index f1b824955d4b..b8f289e6a185 100644
--- a/drivers/pci/pwrctl/Kconfig
+++ b/drivers/pci/pwrctl/Kconfig
@@ -1,17 +1,10 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 
-menu "PCI Power control drivers"
-
 config PCI_PWRCTL
 	tristate
 
 config PCI_PWRCTL_PWRSEQ
-	tristate "PCI Power Control driver using the Power Sequencing subsystem"
+	tristate
 	select POWER_SEQUENCING
 	select PCI_PWRCTL
 	default m if ((ATH11K_PCI || ATH12K) && ARCH_QCOM)
-	help
-	  Enable support for the PCI power control driver for device
-	  drivers using the Power Sequencing subsystem.
-
-endmenu