linux-kernel - Re: [PATCH RFC 2/3] PCI/pwrctrl: Allow pwrctrl core to control PERST# GPIO if available

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: <k5rf5azftn4mpztcjtvdxiligngmaz7fecdryv244m726y5rfd@mobway4c4ueh>
Date: Sat, 12 Jul 2025 13:59:26 +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 2/3] PCI/pwrctrl: Allow pwrctrl core to control
 PERST# GPIO if available

On Fri, Jul 11, 2025 at 05:38:16PM GMT, Brian Norris wrote:
> Sorry for so many individual reviews, but I've passed over this a few
> times and had new questions/comments several times:
> 

That's fine. I'm happy to answer as someone other than me is interested in
pwrctrl :)

> On Mon, Jul 07, 2025 at 11:48:39PM +0530, Manivannan Sadhasivam wrote:
> > PERST# is an (optional) auxiliary signal provided by the PCIe host to
> > components for signalling 'Fundamental Reset' as per the PCIe spec r6.0,
> > sec 6.6.1.
> 
> >  void pci_pwrctrl_init(struct pci_pwrctrl *pwrctrl, struct device *dev)
> >  {
> > +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dev->parent);
> > +	int devfn;
> > +
> >  	pwrctrl->dev = dev;
> >  	INIT_WORK(&pwrctrl->work, rescan_work_func);
> > +
> > +	if (!host_bridge->perst)
> > +		return;
> > +
> > +	devfn = of_pci_get_devfn(dev_of_node(dev));
> > +	if (devfn >= 0 && host_bridge->perst[PCI_SLOT(devfn)])
> 
> This seems to imply a 1:1 correlation between slots and pwrctrl devices,
> almost as if you expect everyone is using drivers/pci/pwrctrl/slot.c.
> But there is also endpoint-specific pwrctrl support, and there's quite
> a bit of flexibility around what these hierarchies can look like.
> 
> How do you account for that?
> 
> For example, couldn't you have both a "port" and an "endpoint" pwrctrl? Would
> they both grab the same PERST# GPIO here? And might that incur excessive
> resets, possibly even clobbering each other?
> 

If both port and endpoint nodes are present, then only one will contain
'reset-gpios'. Right now, the DT binding only supports PERST#, WAKE#, CLKREQ#
properties in RP node, but that won't work if we have multiple lines per slot/
controller. Ideally, we would want the properties to be present in endpoint node
if available. But if we have only standard expansion slots, then it makes sense
to define them in the port node. But doing so, we can only expect the slot to
have only one instance of these properties as we cannot reliably map which
property corresponds to the endpoint.

I've opened a dtschema issue for this:
https://github.com/devicetree-org/dt-schema/issues/168

This is what I have in my mind:

1) For expansion slots:

pcie_port0{
	device_type = "pci";
	reg = <0x0 0x0 0x0 0x0 0x0>;
	bus-range = <0x01 0xff>

	reset-gpios = <>;
	wake-gpios = <>;
};

2) For endpoint nodes:

pcie_port0{
	device_type = "pci";
	reg = <0x0 0x0 0x0 0x0 0x0>;
	bus-range = <0x01 0xff>

	pcie@0 {
		reg = <0x10000 0x0 0x0 0x0 0x0>;
		reset-gpios = <>;
		wake-gpios = <>;
	};

	pcie@1 {
		reg = <0x10800 0x0 0x0 0x0 0x0>;
		reset-gpios = <>;
		wake-gpios = <>;
	};

	...
};

I don't think there is any usecase to have both slot and endpoint nodes defining
these properties.

But for your initial question on what if the platform doesn't define any supply
and uses non-GPIO based PERST#/WAKE#/CLKREQ#, I don't know how to let PCI core
create the pwrctrl device. This is something we need to brainstorm.

> Or what if multiple slots are governed by a single GPIO? Do you expect
> the bridge perst[] array to contain redundant GPIOs?
> 

It is common for devices within a slot/hierarchy to share these GPIOs (with some
drawbacks), but I'm not sure how all slots can share a single GPIO. That would
mean, if the host wants to reset one endpoint in a slot, it has to reset all the
endpoints connected to all slots. Sounds like a bizarre design to me.

- Mani

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