Message-ID: <Z-0v2RAtDe0bSfWI@hovoldconsulting.com>
Date: Wed, 2 Apr 2025 14:38:49 +0200
From: Johan Hovold <johan@...nel.org>
To: Jonas Gorski <jonas.gorski@...il.com>
Cc: Johan Hovold <johan+linaro@...nel.org>,
	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

On Wed, Apr 02, 2025 at 01:54:36PM +0200, Jonas Gorski wrote:

> I have some nitpicks ...

Thanks for taking a look.

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

You're right, thanks for catching this.

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

Moving the arm64 patch last would take care of the temporary glitch, but
since it seems we need to take this through one tree (e.g. to avoid
merging it over three cycles) perhaps we can just squash them.

That said, it seems I would need to reverse the logic for HAVE_PWRCTL so
that it is selected by HAVE_PWRCTRL if we think we still need to worry
about new users of HAVE_PWRCTL being adding to other trees.

If we could get this renamed already in rc1, we probably do no need to
care about that though.

> > +
> > +# 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.

Yeah, this one should 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).

Right. Since PCI_PWRCTL_SLOT will be new in rc1 and there is nothing
selecting it, I think we can just rename this one. At least if we can
get this into rc1 and make sure that the pending defconfig update is
respun.

Otherwise it would indeed need to be user selectable.

Let me respin this. Since this is all quite new, we should be able to
just rip of the band-aid for rc1 and not need to worry about this going
forward.

Johan

Powered by blists - more mailing lists