linux-kernel - Re: [PATCH v2 5/5] PCI/pwrctrl: Switch to the new pwrctrl APIs

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] [day] [month] [year] [list]
Message-ID: <tutxwjciedqoje5wxvtin4h637auni5zzpvb7rtfg4uticxoux@yfl6xg7oht7t>
Date: Tue, 23 Dec 2025 19:41:30 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Sean Anderson <sean.anderson@...o.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Bartosz Golaszewski <brgl@...nel.org>, linux-pci@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, Chen-Yu Tsai <wens@...nel.org>, 
	Brian Norris <briannorris@...omium.org>, Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, 
	Niklas Cassel <cassel@...nel.org>, Alex Elder <elder@...cstar.com>, 
	Chen-Yu Tsai <wenst@...omium.org>
Subject: Re: [PATCH v2 5/5] PCI/pwrctrl: Switch to the new pwrctrl APIs

On Fri, Dec 19, 2025 at 01:35:39PM -0500, Sean Anderson wrote:
> On 12/16/25 07:51, Manivannan Sadhasivam wrote:
> > Adopt the recently introduced pwrctrl APIs to create, power on, destroy,
> > and power off pwrctrl devices. In qcom_pcie_host_init(), call
> > pci_pwrctrl_create_devices() to create devices, then
> > pci_pwrctrl_power_on_devices() to power them on, both after controller
> > resource initialization. Once successful, deassert PERST# for all devices.
> >
> > In qcom_pcie_host_deinit(), call pci_pwrctrl_power_off_devices() after
> > asserting PERST#. Note that pci_pwrctrl_destroy_devices() is not called
> > here, as deinit is only invoked during system suspend where device
> > destruction is unnecessary. If the driver becomes removable in future,
> > pci_pwrctrl_destroy_devices() should be called in the remove() handler.
> >
> > At last, remove the old pwrctrl framework code from the PCI core, as the
> > new APIs are now the sole consumer of pwrctrl functionality. And also do
> > not power on the pwrctrl drivers during probe() as this is now handled by
> > the APIs.
> >
> > Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> > Tested-by: Chen-Yu Tsai <wenst@...omium.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c   | 22 ++++++++++--
> >  drivers/pci/probe.c                      | 59 --------------------------------
> >  drivers/pci/pwrctrl/core.c               | 15 --------
> >  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c |  5 ---
> >  drivers/pci/pwrctrl/slot.c               |  2 --
> >  drivers/pci/remove.c                     | 20 -----------
> >  6 files changed, 20 insertions(+), 103 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 73032449d289..7c0c66480f12 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pci-ecam.h>
> > +#include <linux/pci-pwrctrl.h>
> >  #include <linux/pm_opp.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/platform_device.h>
> > @@ -1318,10 +1319,18 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> >       if (ret)
> >               goto err_deinit;
> >
> > +     ret = pci_pwrctrl_create_devices(pci->dev);
> > +     if (ret)
> > +             goto err_disable_phy;
> > +
> > +     ret = pci_pwrctrl_power_on_devices(pci->dev);
> 
> Won't this result in a deferred probe loop if there is more than one
> pwrctrl device and one has a driver loaded but the other does not?
> 
> deferred_probe_work_func()
>   driver_probe_device(controller)
>     qcom_pcie_probe(controller)
>       pci_pwrctrl_create_devices()
>         device_add(pwrctrl1)
>           (probe successful)
>           driver_deferred_probe_trigger()
>         device_add(pwrctrl2)
>           (driver not loaded)
>       pci_pwrctrl_power_on_devices()
>         return -EPROBE_DEFER
>       pci_pwrctrl_destroy_devices()
>         device_unregister(pwrctrl1)
>         device_unregister(pwrctrl2)
>     driver_deferred_probe_add(controller)
>     // deferred_trigger_count changed, so...
>     driver_deferred_probe_trigger()
> 
> And now you will continually probe the controller until all of the
> drivers are loaded.
> 
> There is a non-obvious property of the deferred probe infrastructure
> which is:
> 
>         Once a device creates children, it must never fail with
>         EPROBE_DEFER.
> 
> So if you want to have something like this, the pwrctrl devices need to
> be created before the controller is probed. Or you can use the current
> system where the pwrctrl devices are probed asynchronously.
> 

You are right and it is an oversight from me. If the pwrctrl driver is not
found, the pwrctrl devices should not be destroyed in the error path, but the
controller driver can still return -EPROBE_DEFER. This will allow the controller
driver to get reprobed later and by that time, pwrctrl device creation will be
skipped. I believe this satisfies the comment you quoted above.

I found this issue while testing the series with one of our Qcom switches and I
fixed it in yet to be submitted v3.

- Mani

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