linux-kernel - Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control PERST#

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: <btxanvs4enrenhowrf47llnvu6az3jx5gjzba5mulxb5jyqgtp@e5tdobflcyxz>
Date: Wed, 9 Jul 2025 13:53:53 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Brian Norris <briannorris@...omium.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Jingoo Han <jingoohan1@...il.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Subject: Re: [PATCH RFC 3/3] PCI: qcom: Allow pwrctrl framework to control
 PERST#

On Tue, Jul 08, 2025 at 08:18:06PM GMT, Brian Norris wrote:
> Hi,
> 
> On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote:
> > Since the Qcom platforms rely on pwrctrl framework to control the power
> > supplies, allow it to control PERST# also. PERST# should be toggled during
> > the power-on and power-off scenarios.
> > 
> > But the controller driver still need to assert PERST# during the controller
> > initialization. So only skip the deassert if pwrctrl usage is detected. The
> > pwrctrl framework will deassert PERST# after turning on the supplies.
> > 
> > The usage of pwrctrl framework is detected based on the new DT binding
> > i.e., with the presence of PERST# and PHY properties in the Root Port node
> > instead of the host bridge node.
> > 
> > When the legacy binding is used, PERST# is only controlled by the
> > controller driver since it is not reliable to detect whether pwrctrl is
> > used or not. So the legacy platforms are untouched by this commit.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@...nel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c |  1 +
> >  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
> >  drivers/pci/controller/dwc/pcie-qcom.c            | 26 ++++++++++++++++++++++-
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 620ac7cf09472b84c37e83ee3ce40e94a1d9d878..61e1d0d6469030c549328ab4d8c65d5377d525e3 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> 
> > @@ -1724,6 +1730,12 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
> >  	if (ret)
> >  		return ret;
> >  
> > +	devfn = of_pci_get_devfn(node);
> > +	if (devfn < 0)
> > +		return -ENOENT;
> > +
> > +	pp->perst[PCI_SLOT(devfn)] = reset;
> 
> It seems like you assume a well-written device tree, such that this
> PCI_SLOT(devfn) doesn't overflow the perst[] array. It seems like we
> should guard against that somehow.
> 

Sure. I will add a check.

> Also see my comment below, where I believe even a well-written device
> tree could trip this up.
> 
> > +
> >  	port->reset = reset;
> >  	port->phy = phy;
> >  	INIT_LIST_HEAD(&port->list);
> > @@ -1734,10 +1746,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
> >  
> >  static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >  {
> > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> >  	struct device *dev = pcie->pci->dev;
> >  	struct qcom_pcie_port *port, *tmp;
> > +	int child_cnt;
> >  	int ret = -ENOENT;
> >  
> > +	child_cnt = of_get_available_child_count(dev->of_node);
> 
> I think you're assuming "available children" correlate precisely with a
> 0-indexed array of ports. But what if, e.g., port 0 is disabled in the
> device tree, and only port 1 is available? Then you'll overflow.
> 

Right. I will take care it in next version.

> > +	if (!child_cnt)
> > +		return ret;
> > +
> > +	pp->perst = kcalloc(child_cnt, sizeof(struct gpio_desc *), GFP_KERNEL);
> 
> IIUC, you kfree() this on error, but otherwise, you never free it. I
> also see that this driver can't actually be unbound (commit f9a666008338
> ("PCI: qcom: Make explicitly non-modular")), so technically there's no
> way to "leak" this other than by probe errors...
> ...but it still seems like devm_*() would fit better.
> 

Even if we use devm_(), we need to free the array when qcom_pcie_parse_port()
fails. And as you spotted, we don't remove the driver currently. So I decided to
use kfree(). Someone would argue that if we manually free the memory, then it
defeats the purpose of devm_() variants.

> (NB: I'm not sure I agree with commit f9a666008338 that "[driver unbind]
> doesn't have a sensible use case anyway". That just sounds like
> laziness. And it *can* have a useful purpose for testing.)
> 

There was a whole debate on it. It is mostly due to the fact that this driver
implements the MSI controller and the IRQCHIP drivers are not expected to go
away during runtime. But atleast, I would like to build this driver as a module
and not remove it. The patch is pending for some time.

- Mani

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