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@xxxxxxxxxxxxxxx # 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@xxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/r/CA+-6iNwgaByXEYD3j=-+H_PKAxXRU78svPMRHDKKci8AGXAUPg@xxxxxxxxxxxxxx
>
> 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@xxxxxxxxxxxxxx
- Mani
--
மணிவண்ணன் சதாசிவம்
