Message-ID: <stvmzmcfsubtt2mvpzy4245qk5sr2afgle4gvgwcfiw33aqsbt@ttv5atldoy4q>
Date: Wed, 2 Jul 2025 12:50:03 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: bhelgaas@...gle.com, lukas@...ner.de, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org, Jim Quinlan <james.quinlan@...adcom.com>, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>
Subject: Re: [PATCH v2] PCI/pwrctrl: Skip creating pwrctrl device unless
 CONFIG_PCI_PWRCTRL is enabled

On Tue, Jul 01, 2025 at 04:01:38PM -0500, Bjorn Helgaas wrote:
> [+cc Bart, Krzysztof, update Mani's addr to kernel.org]
> 
> On Tue, Jul 01, 2025 at 12:17:31PM +0530, Manivannan Sadhasivam wrote:
> > If devicetree describes power supplies related to a PCI device, we
> > previously created a pwrctrl device even if CONFIG_PCI_PWRCTL was
> > not enabled.
> > 
> > When pci_pwrctrl_create_device() creates and returns a pwrctrl device,
> > pci_scan_device() doesn't enumerate the PCI device. It assumes the pwrctrl
> > core will rescan the bus after turning on the power. However, if
> > CONFIG_PCI_PWRCTL is not enabled, the rescan never happens.
> > 
> > This may break PCI enumeration on any system that describes power supplies
> > in devicetree but does not use pwrctrl. Jim reported that some brcmstb
> > platforms break this way.
> > 
> > While the actual fix would be to convert all the platforms to use pwrctrl
> > framework, we also need to skip creating the pwrctrl device if
> > CONFIG_PCI_PWRCTL is not enabled and let the PCI core scan the device
> > normally (assuming it is already powered on or by the controller driver).
> 
> I'm fine with this change, but I think the commit log leaves the wrong
> impression.  If CONFIG_PCI_PWRCTRL is not enabled, we shouldn't do
> anything related to it, independent of what other platforms or drivers
> do.
> 
> So I wouldn't describe this as "the actual fix is converting all
> platforms to use pwrctrl."  Even if all platforms use pwrctrl, we
> *still* shouldn't run pci_pwrctrl_create_device() unless
> CONFIG_PCI_PWRCTRL is enabled.
> 
> I think all we need to say is something like this:
> 
>   We only need pci_pwrctrl_create_device() when CONFIG_PCI_PWRCTRL is
>   enabled.  Compile it out when CONFIG_PCI_PWRCTRL is not enabled.
> 

Sounds fair.

> > Cc: stable@...r.kernel.org # 6.15
> > Fixes: 957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> 
> Not sure about this.  If the problem we're solving is "we run pwrctrl
> code when CONFIG_PCI_PWRCTRL is not enabled," 957f40d039a9 is not the
> commit that added that behavior.
> 

Well, this exact commit causes breakage on Jim's platform. That's why I've added
it as the fixes tag irrespective of the pwrctrl functionality.

> Maybe 8fb18619d910 ("PCI/pwrctl: Create platform devices for child OF
> nodes of the port node") would be more appropriate?
> 
> > Reported-by: Jim Quinlan <james.quinlan@...adcom.com>
> > Closes: https://lore.kernel.org/r/CA+-6iNwgaByXEYD3j=-+H_PKAxXRU78svPMRHDKKci8AGXAUPg@mail.gmail.com
> 
> I'm also not sure this really merits a "Closes:" tag.  All this does
> is enable a workaround (disable CONFIG_PCI_PWRCTRL when brcmstb is
> enabled).  That's not a fix because we *should* be able to enable both
> pwrctrl and brcmstb at the same time.
> 

Hmm, yeah. For this patch to work, one has to make sure that CONFIG_PCI_PWRCTRL
is not set. This requires not supporting the ATH11K/12K chipsets and Jim said
that they don't have a usecase for supporting these chipsets yet on the brcmstb
platform (which only the internal team uses to run mainline).

> If 2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if
> pwrctrl device is created") was purely an optimization (see
> https://lore.kernel.org/r/20250701203526.GA1849466@bhelgaas), I think
> I would:
> 
>   - Revert 2489eeb777af with a stable tag for v6.15, and
> 

But reverting 2489eeb777af alone wouldn't fix this regression [1].

>   - Apply this patch with a Fixes: 8fb18619d910 ("PCI/pwrctl: Create
>     platform devices for child OF nodes of the port node") but no
>     stable tag.  8fb18619d910 appeared in v6.11 and the "don't enable
>     CONFIG_PCI_PWRCTRL" workaround was enough for brcmstb until
>     2489eeb777af, so if we revert 2489eeb777af, we wouldn't need to
>     backport *this* patch.
> 

Which means, this patch will only get applied for v6.16. I don't understand how
that will ensure v6.15 is not broken (even after the revert).

- Mani

[1] https://lore.kernel.org/all/CA+-6iNxkYumAvk5G6KhYqON9K3bwxGn+My-22KZnGF5Pg8cgfA@mail.gmail.com

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists