linux-kernel - Re: [PATCH 1/4] PCI/pwrctrl: Rename pwrctrl Kconfig symbols and slot module

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: <CAOiHx=mo6Qd+7WrO2JvBLhqjGR7oHds14FwFFAVoEkVWLnbhdA@mail.gmail.com>
Date: Wed, 2 Apr 2025 13:54:36 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Johan Hovold <johan+linaro@...nel.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
	Jeff Johnson <jjohnson@...nel.org>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, linux-pci@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, ath11k@...ts.infradead.org, 
	ath12k@...ts.infradead.org, linux-wireless@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] PCI/pwrctrl: Rename pwrctrl Kconfig symbols and slot module

Hi,

I have some nitpicks ...

On Fri, Mar 28, 2025 at 3:41 PM Johan Hovold <johan+linaro@...nel.org> wrote:
>
> Commits b88cbaaa6fa1 ("PCI/pwrctrl: Rename pwrctl files to pwrctrl") and
> 3f925cd62874 ("PCI/pwrctrl: Rename pwrctrl functions and structures")
> renamed the "pwrctl" framework to "pwrctrl" for consistency reasons.
>
> Rename also the Kconfig symbols so that they reflect the new name while
> adding entries for the deprecated ones. The old symbols can be removed
> once everything that depends on them has been updated.
>
> The new slot module is also renamed to reflect the framework name and
> match the other pwrctrl modules.
>
> Signed-off-by: Johan Hovold <johan+linaro@...nel.org>
> ---
>  drivers/pci/pwrctrl/Kconfig  | 27 +++++++++++++++++++++------
>  drivers/pci/pwrctrl/Makefile |  8 ++++----
>  2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/pwrctrl/Kconfig b/drivers/pci/pwrctrl/Kconfig
> index 990cab67d413..62f176e42e33 100644
> --- a/drivers/pci/pwrctrl/Kconfig
> +++ b/drivers/pci/pwrctrl/Kconfig
> @@ -1,19 +1,19 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>
> -config HAVE_PWRCTL
> +config HAVE_PWRCTRL
>         bool
>
> -config PCI_PWRCTL
> +config PCI_PWRCTRL
>         tristate
>
> -config PCI_PWRCTL_PWRSEQ
> +config PCI_PWRCTRL_PWRSEQ
>         tristate
>         select POWER_SEQUENCING
> -       select PCI_PWRCTL
> +       select PCI_PWRCTRL
>
> -config PCI_PWRCTL_SLOT
> +config PCI_PWRCTRL_SLOT
>         tristate "PCI Power Control driver for PCI slots"
> -       select PCI_PWRCTL
> +       select PCI_PWRCTRL
>         help
>           Say Y here to enable the PCI Power Control driver to control the power
>           state of PCI slots.
> @@ -21,3 +21,18 @@ config PCI_PWRCTL_SLOT
>           This is a generic driver that controls the power state of different
>           PCI slots. The voltage regulators powering the rails of the PCI slots
>           are expected to be defined in the devicetree node of the PCI bridge.
> +
> +# deprecated
> +config HAVE_PWRCTL
> +       bool
> +       select HAVE_PWRCTRL

I'm not sure this will work as intended. This symbol can only be != n
if anything selects it, but there may also be (outdated) config
symbols that depend on its value. E.g. ath1*k has "select
PCI_PWRCTL_PWRSEQ if HAVE_PWRCTL", and if there is nothing selecting
HAVE_PWRCTL, but HAVE_PWRCTRL directly instead, HAVE_PWRCTL will be =n
and the condition will fail.

Since you rename the only one selecting HAVE_PWRCTL in patch 2/4, but
update ath1*k in 3/4 and 4/4, their select PCI_PWRCT(R)L_PWRSEQ use is
temporarily ineffective. Moving the arm64 patch last would avoid that
though, at least for the current state.

The alternative would be split this by config symbol instead of per
tree, so all users would be atomatically updated as well. These
patches need to go through the same tree anyways, so I see no issue
doing it that way.

> +
> +# deprecated
> +config PCI_PWRCTL_PWRSEQ
> +       tristate
> +       select PCI_PWRCTRL_PWRSEQ

Similar issue, but there are no conditionals based on this, so this may be fine.

> +
> +# deprecated
> +config PCI_PWRCTL_SLOT
> +       tristate
> +       select PCI_PWRCTRL_SLOT

This one won't work. Its value will be automatically calculated based
on other symbols selecting it, and since there is nothing selecting
it, it will always be n, regardless what any existing .config says.

So unless you make this a user selectable symbol as well, this will
(potentially) break existing .configs since its value will be then
automatically calculated as =n, and the new symbol takes the default
=n (unless explicitly enabled, or selected by ath1*k).

Best regards,
Jonas