linux-kernel - Re: [PATCH v3 1/2] PCI: Add flr_delay parameter to pci

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: <20250806220653.GA20136@bhelgaas>
Date: Wed, 6 Aug 2025 17:06:53 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: grwhyte@...ux.microsoft.com
Cc: linux-pci@...r.kernel.org, shyamsaini@...ux.microsoft.com,
	code@...icks.com, Okaya@...nel.org, bhelgaas@...gle.com,
	linux-kernel@...r.kernel.org,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Lukas Wunner <lukas@...ner.de>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Christoph Hellwig <hch@...radead.org>,
	Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v3 1/2] PCI: Add flr_delay parameter to pci_dev struct

On Wed, Jun 11, 2025 at 12:05:51AM +0000, grwhyte@...ux.microsoft.com wrote:
> From: Graham Whyte <grwhyte@...ux.microsoft.com>
> 
> Add a new flr_delay member of the pci_dev struct to allow customization of
> the delay after FLR for devices that do not support immediate readiness.
> 
> Signed-off-by: Graham Whyte <grwhyte@...ux.microsoft.com>
> ---
>  drivers/pci/pci.c   | 8 ++++++--
>  drivers/pci/pci.h   | 2 ++
>  include/linux/pci.h | 1 +
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e9448d55113b..04f2660df7c4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3233,6 +3233,8 @@ void pci_pm_init(struct pci_dev *dev)
>  	dev->bridge_d3 = pci_bridge_d3_possible(dev);
>  	dev->d3cold_allowed = true;
>  
> +	dev->flr_delay = PCI_FLR_DELAY;

There are some delays mentioned in pci_pm_init(), but I don't think
this one has anything to do with the PM Capability, so it should go
elsewhere.  Maybe somewhere related to this recent change:
https://git.kernel.org/linus/5c0d0ee36f16 ("PCI: Support Immediate
Readiness on devices without PM cap abilities").

This would be more attractive if we actually added support for the
Readiness Time Reporting Capability (PCIe r7.0, sec 7.9.16).  Then
devices that implement that would get actual benefit from this.

I'm not committing to merging a quirk just for the Microsoft devices,
but I definitely wouldn't merge it unless devices that did the work of
supporting the standard mechanism also got the benefit.  The hardest
part might be *finding* a device that supports Readiness Time
Reporting so we could test it.

>  	dev->d1_support = false;
>  	dev->d2_support = false;
>  	if (!pci_no_d1d2(dev)) {
> @@ -4529,9 +4531,11 @@ int pcie_flr(struct pci_dev *dev)
>  	/*
>  	 * Per PCIe r4.0, sec 6.6.2, a device must complete an FLR within
>  	 * 100ms, but may silently discard requests while the FLR is in
> -	 * progress.  Wait 100ms before trying to access the device.
> +	 * progress.  Wait 100ms before trying to access the device, unless
> +	 * otherwise modified if the device supports a faster reset.
> +	 * Use usleep_range to support delays under 20ms.
>  	 */
> -	msleep(100);
> +	usleep_range(dev->flr_delay, dev->flr_delay+1);

As Ilpo suggested, I think fsleep() is the right thing for this.
Readiness Time Reporting supports values down to 1ns, but I suspect we
can live with microsecond resolution for now.

>  	return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
>  }
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 12215ee72afb..abc1cf6e6d9b 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -135,6 +135,8 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
>  #define PCI_PM_D3HOT_WAIT       10	/* msec */
>  #define PCI_PM_D3COLD_WAIT      100	/* msec */
>  
> +#define PCI_FLR_DELAY           100000 /* usec */

Isn't PCIE_RESET_CONFIG_WAIT_MS the right value for the default delay?
(I think that name was probably settled on after you posted this.)

Maybe it's not; I see that Readiness Time Reporting includes separate
values for Reset Time, DL_Up Time, FLR Time, and D3Hot to D0 Time.
I'm not sure Linux comprehends those differences yet.

>  void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>  void pci_refresh_power_state(struct pci_dev *dev);
>  int pci_power_up(struct pci_dev *dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f392..4c9989037ed1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -402,6 +402,7 @@ struct pci_dev {
>  						   bit manually */
>  	unsigned int	d3hot_delay;	/* D3hot->D0 transition time in ms */
>  	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
> +	unsigned int    flr_delay;      /* pci post flr sleep time in us */

I think this should be named to correspond with the Readiness Time
Reporting Capability.  

>  	u16		l1ss;		/* L1SS Capability pointer */
>  #ifdef CONFIG_PCIEASPM
> -- 
> 2.25.1
>