linux-kernel - Re: [RFC PATCH] PCI: pciehp: Replace fixed delay with polling for slot power-off

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: <aFP598Yyl0el1uKh@wunner.de>
Date: Thu, 19 Jun 2025 13:52:23 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Hongbo Yao <andy.xu@...micro.com>
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	peter.du@...micro.com, jemma.zhang@...micro.com
Subject: Re: [RFC PATCH] PCI: pciehp: Replace fixed delay with polling for
 slot power-off

On Thu, Jun 19, 2025 at 05:32:28PM +0800, Hongbo Yao wrote:
> Fixed 1-second delay in remove_board() fails to accommodate certain
> hardware like multi-host OCP cards, which exhibit longer power-off
> latencies.

Please name the affected product(s).

They don't seem to comply to the spec.  How prevalent are they?
If there are only few deployed, quirks like this are probably
best addressed by an out-of-tree patch.

> Logs before fix:
> [157.778307] pcieport 0003:00:00.0: pciehp: pending interrupts 0x0001 from Slot Status
> [157.778321] pcieport 0003:00:00.0: pciehp: Slot(31): Attention button pressed
> [157.785445] pcieport 0003:00:00.0: pciehp: Slot(31): Powering off due to button press
> [157.798931] pcieport 000b:00:02.0: pciehp: pending interrupts 0x0001 from Slot Status

This log excerpt mixes messages from two separate hotplug ports
(0003:00:00.0 and 000b:00:02.0).  Are these hotplug ports related?
If not, please reduce the log excerpt to a single hotplug port
to avoid confusion.

> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -30,6 +30,25 @@
>  #define SAFE_REMOVAL	 true
>  #define SURPRISE_REMOVAL false
>  
> +static void pciehp_wait_for_link_inactive(struct controller *ctrl)
> +{
> +	u16 lnk_status;
> +	int timeout = 10000, step = 20;
> +
> +	do {
> +		pcie_capability_read_word(ctrl->pcie->port, PCI_EXP_LNKSTA,
> +					  &lnk_status);
> +
> +		if (!(lnk_status & PCI_EXP_LNKSTA_DLLLA))
> +			return;
> +
> +		msleep(step);
> +		timeout -= step;
> +	} while (timeout >= 0);
> +
> +	ctrl_dbg(ctrl, "Timeout waiting for link inactive state\n");
> +}

Any chance you can use one of the existing helpers, such as
pcie_wait_for_link()?

Is the 10 second delay chosen arbitrarily or how did you come up
with it?  How much time do the affected products really need?

> @@ -119,8 +138,11 @@ static void remove_board(struct controller *ctrl, bool safe_removal)
>  		 * After turning power off, we must wait for at least 1 second
>  		 * before taking any action that relies on power having been
>  		 * removed from the slot/adapter.
> +		 *
> +		 * Extended wait with polling to ensure hardware has completed
> +		 * power-off sequence.
>  		 */
> -		msleep(1000);
> +		pciehp_wait_for_link_inactive(ctrl);
>  
>  		/* Ignore link or presence changes caused by power off */
>  		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),

Please keep the msleep(1000), that's the minimum we need to wait
per PCIe r6.3 sec 6.7.1.8.

Please make the extra wait for link down conditional on
ctrl->pcie->port->link_active_reporting.  (DLLLA reporting is
optional for hotplug ports conforming to older spec revisions.)

Thanks,

Lukas