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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aG3e26yjO4I1WSnG@google.com>
Date: Tue, 8 Jul 2025 20:15:39 -0700
From: Brian Norris <briannorris@...omium.org>
To: Manivannan Sadhasivam <mani@...nel.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

Hi Manivannan,

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.
> 
> If PERST# is available, it's state will be toggled during the component
> power-up and power-down scenarios as per the PCI Express Card
> Electromechanical Spec v4.0, sec 2.2.
> 
> Historically, the PCIe controller drivers were directly controlling the
> PERST# signal together with the power supplies. But with the advent of the
> pwrctrl framework, the power supply control is now moved to the pwrctrl,
> but controller drivers still ended up toggling the PERST# signal.

[reflowed:]
> This only happens on Qcom platforms where pwrctrl framework is being
> used.

What do you mean by this sentence? That this problem only occurs on Qcom
platforms? (I believe that's false.) Or that the problem doesn't occur
if the platform is not using pwrctrl? (i.e., it maintained power in some
other way, before the controller driver gets involved. I believe this
variation is correct.)

> But
> nevertheseless, it is wrong to toggle PERST# (especially deassert) without
> controlling the power supplies.
> 
> So allow the pwrctrl core to control the PERST# GPIO is available. The

s/is/if/

?

> controller drivers still need to parse them and populate the
> 'host_bridge->perst' GPIO descriptor array based on the available slots.
> Unfortunately, we cannot just move the PERST# handling from controller
> drivers as most of the controller drivers need to assert PERST# during the
> controller initialization.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@...nel.org>
> ---
>  drivers/pci/pwrctrl/core.c  | 39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-pwrctrl.h |  2 ++
>  include/linux/pci.h         |  2 ++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> index 6bdbfed584d6d79ce28ba9e384a596b065ca69a4..abdb46399a96c8281916f971329d5460fcff3f6e 100644
> --- a/drivers/pci/pwrctrl/core.c
> +++ b/drivers/pci/pwrctrl/core.c

>  static int pci_pwrctrl_notify(struct notifier_block *nb, unsigned long action,
>  			      void *data)
>  {
> @@ -56,11 +61,42 @@ static void rescan_work_func(struct work_struct *work)
>   */
>  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)])
> +		pwrctrl->perst = host_bridge->perst[PCI_SLOT(devfn)];

It seems a little suspect that we trust the device tree slot
specification to not overflow the perst[] array. I think we can
reasonably mitigate that in the controller driver (so, patch 3 in this
series), but I want to call that out, in case there's something we can
do here too.

>  }
>  EXPORT_SYMBOL_GPL(pci_pwrctrl_init);
>  
> +static void pci_pwrctrl_perst_deassert(struct pci_pwrctrl *pwrctrl)
> +{
> +	/* Bail out early to avoid the delay if PERST# is not available */
> +	if (!pwrctrl->perst)
> +		return;
> +
> +	msleep(PCIE_T_PVPERL_MS);
> +	gpiod_set_value_cansleep(pwrctrl->perst, 0);

What if PERST# was already deasserted? On one hand, we're wasting time
here if so. On the other, you're not accomplishing your spec-compliance
goal if it was.

> +	/*
> +	 * FIXME: The following delay is only required for downstream ports not
> +	 * supporting link speed greater than 5.0 GT/s.
> +	 */
> +	msleep(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);

Should this be PCIE_RESET_CONFIG_DEVICE_WAIT_MS or PCIE_T_RRS_READY_MS?
Or are those describing the same thing? It seems like they were added
within a month or two of each other, so maybe they're just duplicates.

BTW, I see you have a FIXME here, but anyway, I wonder if both of the
msleep() delays in this function will need some kind of override (e.g.,
via Device Tree), since there's room for implementation or form factor
differences, if I'm reading the spec correctly. Maybe that's a question
for another time, with actual proof / use case.

> +}
> +

Brian