linux-kernel - Re: [PATCH 4/5] PCI/pwrctrl: Add APIs to power on/off the pwrctrl devices

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: <rsfsl32fjfmke6ffvz6y3lvvh54rofnaetuxy4qo3niffjcaue@udb44lfwbfga>
Date: Tue, 25 Nov 2025 19:20:02 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: manivannan.sadhasivam@....qualcomm.com, 
	Manivannan Sadhasivam via B4 Relay <devnull+manivannan.sadhasivam.oss.qualcomm.com@...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>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH 4/5] PCI/pwrctrl: Add APIs to power on/off the pwrctrl
 devices

On Tue, Nov 25, 2025 at 05:34:04AM -0800, Bartosz Golaszewski wrote:
> On Mon, 24 Nov 2025 17:20:47 +0100, Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@...nel.org> said:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> >
> > To fix PCIe bridge resource allocation issues when powering PCIe
> > switches with the pwrctrl driver, introduce APIs to explicitly power
> > on and off all related devices simultaneously.
> >
> > Previously, the individual pwrctrl drivers powered on/off the PCIe devices
> > autonomously, without any control from the controller drivers. But to
> > enforce ordering w.r.t powering on the devices, these APIs will power
> > on/off all the devices at the same time.
> >
> > The pci_pwrctrl_power_on_devices() API recursively scans the PCI child
> > nodes, makes sure that pwrctrl drivers are bind to devices, and calls their
> > power_on() callbacks.
> >
> > Similarly, pci_pwrctrl_power_off_devices() API powers off devices
> > recursively via their power_off() callbacks.
> >
> > These APIs are expected to be called during the controller probe and
> > suspend/resume time to power on/off the devices. But before calling these
> > APIs, the pwrctrl devices should've been created beforehand using the
> > pci_pwrctrl_{create/destroy}_devices() APIs.
> >
> > Co-developed-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > ---
> >  drivers/pci/pwrctrl/core.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-pwrctrl.h |   4 ++
> >  2 files changed, 125 insertions(+)
> >
> > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > index 6eca54e0d540..e0a0cf015bd0 100644
> > --- a/drivers/pci/pwrctrl/core.c
> > +++ b/drivers/pci/pwrctrl/core.c
> > @@ -65,6 +65,7 @@ void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> >  {
> >  	pwrctrl->dev = dev;
> >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > +	dev_set_drvdata(dev, pwrctrl);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
> >
> > @@ -152,6 +153,126 @@ int devm_pci_pwrctrl_device_set_ready(struct device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(devm_pci_pwrctrl_device_set_ready);
> >
> > +static int __pci_pwrctrl_power_on_device(struct device *dev)
> 
> Both this and __pci_pwrctrl_power_off_device() are only used once each. Does
> it really make sense to split it out?
> 

I just find it neat to split it out. Otherwise, the else condition looks clumsy
in pci_pwrctrl_power_on_device().

> > +{
> > +	struct pci_pwrctrl *pwrctrl = dev_get_drvdata(dev);
> > +
> > +	if (!pwrctrl)
> > +		return 0;
> > +
> > +	return pwrctrl->power_on(pwrctrl);
> > +}
> > +
> > +/*
> > + * Power on the devices in a depth first manner. Before powering on the device,
> > + * make sure its driver is bound.
> > + */
> > +static int pci_pwrctrl_power_on_device(struct device_node *np)
> > +{
> > +	struct platform_device *pdev;
> > +	int ret;
> > +
> > +	for_each_available_child_of_node_scoped(np, child) {
> > +		ret = pci_pwrctrl_power_on_device(child);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	pdev = of_find_device_by_node(np);
> > +	if (pdev) {
> > +		if (!device_is_bound(&pdev->dev)) {
> > +			dev_err(&pdev->dev, "driver is not bound\n");
> 
> This is not an error though, is it? If there are multiple deferalls, we'll
> spam the kernel log.
> 

Good question. Initially, I made it as a debug log, but then realized that
people may wonder why their controller driver encounters probe deferral without
much clue, especially when the driver spits out other logs before calling this
API. So decided to make it dev_err() to give a visual indication.

If it is not preferred, I can demote it to debug log.

- Mani

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