[PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[grwhyte]

From: Graham Whyte <grwhyte@linux.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
or readiness time reporting. The main scenario this addresses is VF
removal and rescan during runtime repairs and driver updates, which,
if fixed to 100ms, introduces significant delays across multiple VFs.
These delays are unnecessary for devices that complete the FLR well
within this timeframe.

Patch 1 adds the flr_delay member to the pci_dev struct
Patch 2 adds the msft device specific quirk to utilize the flr_delay

---
v2->v3:
- Removed Microsoft specific pcie reset reset, replaced with customizable flr_delay parameter
- Changed msleep in pcie_flr to usleep_range to support flr delays of under 20ms 
v1->v2:
- Removed unnecessary EXPORT_SYMBOL_GPL for function pci_dev_wait
- Link to thread:https://lore.kernel.org/linux-pci/?q=f%3Agrwhyte&x=t#m7453647902a1b22840f5e39434a631fd7b2515ce'

Link to V1: https://lore.kernel.org/linux-pci/20250522085253.GN7435@unreal/T/#m7453647902a1b22840f5e39434a631fd7b2515ce  

Graham Whyte (2):
  PCI: Add flr_delay parameter to pci_dev struct
  PCI: Reduce FLR delay to 10ms for MSFT devices

 drivers/pci/pci.c    |  8 ++++++--
 drivers/pci/pci.h    |  2 ++
 drivers/pci/quirks.c | 20 ++++++++++++++++++++
 include/linux/pci.h  |  1 +
 4 files changed, 29 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] PCI: Add flr_delay parameter to pci_dev struct

[grwhyte]

From: Graham Whyte <grwhyte@linux.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@linux.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;
+
 	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);
 
 	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 */
+
 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 */
 
 	u16		l1ss;		/* L1SS Capability pointer */
 #ifdef CONFIG_PCIEASPM
-- 
2.25.1

[PATCH v3 2/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[grwhyte]

From: Graham Whyte <grwhyte@linux.microsoft.com>

Add a new quirk to reduce the delay after a FLR to 10ms
for MSFT devices. These devices complete the FLR well within the default
100ms timeframe and this path can be optimized for VF removal during
runtime repairs and driver updates. These devices do not support immediate
readiness or readiness time reporting

Signed-off-by: Graham Whyte <grwhyte@linux.microsoft.com>
---
 drivers/pci/quirks.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d7f4ee634263..d704606330bd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6335,3 +6335,23 @@ static void pci_mask_replay_timer_timeout(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
+
+#define MICROSOFT_2051_SVC 0xb210
+#define MICROSOFT_2051_MANA_MGMT 0x00b8
+#define MICROSOFT_2051_MANA_MGMT_GFT 0xb290
+
+/*
+ * For devices that don't require the full 100ms sleep
+ * after FLR and do not support immediate readiness or readiness
+ * time reporting
+ */
+static void pci_fixup_pci_flr_10msec(struct pci_dev *pdev)
+{
+	pdev->flr_delay = 10000;
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MICROSOFT, MICROSOFT_2051_SVC,
+	pci_fixup_pci_flr_10msec);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MICROSOFT, MICROSOFT_2051_MANA_MGMT,
+	pci_fixup_pci_flr_10msec);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MICROSOFT, MICROSOFT_2051_MANA_MGMT_GFT,
+	pci_fixup_pci_flr_10msec);
-- 
2.25.1

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Christoph Hellwig]

On Wed, Jun 11, 2025 at 12:05:50AM +0000, grwhyte@linux.microsoft.com wrote:
> From: Graham Whyte <grwhyte@linux.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
> or readiness time reporting. The main scenario this addresses is VF
> removal and rescan during runtime repairs and driver updates, which,
> if fixed to 100ms, introduces significant delays across multiple VFs.
> These delays are unnecessary for devices that complete the FLR well
> within this timeframe.

Please work with the PCIe SIG to have a standard capability for this
instead of piling up hacks like this quirk.

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Niklas Cassel]

On Tue, Jun 10, 2025 at 08:27:12PM -0700, Christoph Hellwig wrote:
> On Wed, Jun 11, 2025 at 12:05:50AM +0000, grwhyte@linux.microsoft.com wrote:
> > From: Graham Whyte <grwhyte@linux.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
> > or readiness time reporting. The main scenario this addresses is VF
> > removal and rescan during runtime repairs and driver updates, which,
> > if fixed to 100ms, introduces significant delays across multiple VFs.
> > These delays are unnecessary for devices that complete the FLR well
> > within this timeframe.
> 
> Please work with the PCIe SIG to have a standard capability for this
> instead of piling up hacks like this quirk.

There is already some support in PCIe for this.

For Conventional Reset, see PCIe 6.0, section 6.6.1, there is the
"Readiness Time Reporting Extended Capability":
"For a Device that implements the Readiness Time Reporting Extended Capability,
and has reported a Reset Time shorter than 100ms, software is permitted to send
a Configuration Request to the Device after waiting the reported Reset Time
from Conventional Reset."


For FLR, see PCIe 6.0, section 6.6.2, there is the "Function Readiness Status":
"After an FLR has been initiated by writing a 1b to the Initiate Function Level
Reset bit, the Function must complete the FLR within 100 ms. [...] If Function
Readiness Status (FRS - see § Section 6.22.2 ) is implemented, then system
software is permitted to issue Configuration Requests to the Function
immediately following receipt of an FRS Message indicating Configuration-Ready,
however, this does not necessarily indicate that outstanding Requests initiated
by the Function have completed."


Kind regards,
Niklas

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Graham Whyte]

On 6/11/2025 12:23 AM, Niklas Cassel wrote:
> On Tue, Jun 10, 2025 at 08:27:12PM -0700, Christoph Hellwig wrote:
>> On Wed, Jun 11, 2025 at 12:05:50AM +0000, grwhyte@linux.microsoft.com wrote:
>>> From: Graham Whyte <grwhyte@linux.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
>>> or readiness time reporting. The main scenario this addresses is VF
>>> removal and rescan during runtime repairs and driver updates, which,
>>> if fixed to 100ms, introduces significant delays across multiple VFs.
>>> These delays are unnecessary for devices that complete the FLR well
>>> within this timeframe.
>>
>> Please work with the PCIe SIG to have a standard capability for this
>> instead of piling up hacks like this quirk.
> 
> There is already some support in PCIe for this.
> 
> For Conventional Reset, see PCIe 6.0, section 6.6.1, there is the
> "Readiness Time Reporting Extended Capability":
> "For a Device that implements the Readiness Time Reporting Extended Capability,
> and has reported a Reset Time shorter than 100ms, software is permitted to send
> a Configuration Request to the Device after waiting the reported Reset Time
> from Conventional Reset."
> 
> 
> For FLR, see PCIe 6.0, section 6.6.2, there is the "Function Readiness Status":
> "After an FLR has been initiated by writing a 1b to the Initiate Function Level
> Reset bit, the Function must complete the FLR within 100 ms. [...] If Function
> Readiness Status (FRS - see § Section 6.22.2 ) is implemented, then system
> software is permitted to issue Configuration Requests to the Function
> immediately following receipt of an FRS Message indicating Configuration-Ready,
> however, this does not necessarily indicate that outstanding Requests initiated
> by the Function have completed."
> 
> 
> Kind regards,
> Niklas

We can ask our HW engineers to implement function readiness but we need to be
able to support exiting products, hence why posting it as a quirk. 

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Christoph Hellwig]

On Wed, Jun 11, 2025 at 01:08:21PM -0700, Graham Whyte wrote:
> We can ask our HW engineers to implement function readiness but we need
> to be able to support exiting products, hence why posting it as a quirk.

Your report sounds like it works perfectly fine, it's just that you
want to reduce the delay.  For that you'll need to stick to the standard
methods instead of adding quirks, which are for buggy hardware that does
not otherwise work.

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Graham Whyte]

On 6/11/2025 11:31 PM, Christoph Hellwig wrote:
> On Wed, Jun 11, 2025 at 01:08:21PM -0700, Graham Whyte wrote:
>> We can ask our HW engineers to implement function readiness but we need
>> to be able to support exiting products, hence why posting it as a quirk.
> 
> Your report sounds like it works perfectly fine, it's just that you
> want to reduce the delay.  For that you'll need to stick to the standard
> methods instead of adding quirks, which are for buggy hardware that does
> not otherwise work.

Bjorn, what would you recommend as next steps here?

Re: [PATCH v3 1/2] PCI: Add flr_delay parameter to pci_dev struct

[Ilpo Järvinen]

On Wed, 11 Jun 2025, grwhyte@linux.microsoft.com wrote:

> From: Graham Whyte <grwhyte@linux.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@linux.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;
> +
>  	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);

Missing spaces around +.

Are you sure + 1us is really useful as the range? Usually much bigger 
numbers are used.

There's also fsleep() which would autoselect the sleep mechanism.

>  	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 */

Please put the unit into the define name (_US).

> +
>  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 */

Please follow how the spec writes things in capitalization of letters.

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

-- 
 i.

Re: [PATCH v3 2/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Ilpo Järvinen]

On Wed, 11 Jun 2025, grwhyte@linux.microsoft.com wrote:

> From: Graham Whyte <grwhyte@linux.microsoft.com>
> 
> Add a new quirk to reduce the delay after a FLR to 10ms
> for MSFT devices. These devices complete the FLR well within the default
> 100ms timeframe and this path can be optimized for VF removal during

What is "this path" in this context? Try to avoid vague references like 
that.

> runtime repairs and driver updates. These devices do not support immediate
> readiness or readiness time reporting

When talking about something that relates to PCIe spec, please also refer 
to PCIe spec and use the terminology matching to the spec (+ capitalization).

Missing .

Please also reflow the paragraph as the first line is clearly not full.


This probably belongs more to the previous patch changelog than this one, 
as the justification: I suggest you start by stating the problem. So state 
the spec defined wait (+ spec reference), and why that is problem. Then 
explain the solution in another paragraph.

> Signed-off-by: Graham Whyte <grwhyte@linux.microsoft.com>
> ---
>  drivers/pci/quirks.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index d7f4ee634263..d704606330bd 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6335,3 +6335,23 @@ static void pci_mask_replay_timer_timeout(struct pci_dev *pdev)
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
>  #endif
> +
> +#define MICROSOFT_2051_SVC 0xb210
> +#define MICROSOFT_2051_MANA_MGMT 0x00b8
> +#define MICROSOFT_2051_MANA_MGMT_GFT 0xb290
> +
> +/*
> + * For devices that don't require the full 100ms sleep
> + * after FLR and do not support immediate readiness or readiness
> + * time reporting
> + */
> +static void pci_fixup_pci_flr_10msec(struct pci_dev *pdev)
> +{
> +	pdev->flr_delay = 10000;

10 * USEC_PER_MSEC

> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MICROSOFT, MICROSOFT_2051_SVC,
> +	pci_fixup_pci_flr_10msec);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MICROSOFT, MICROSOFT_2051_MANA_MGMT,
> +	pci_fixup_pci_flr_10msec);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MICROSOFT, MICROSOFT_2051_MANA_MGMT_GFT,
> +	pci_fixup_pci_flr_10msec);
> 

-- 
 i.

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Manivannan Sadhasivam]

On Wed, Jun 11, 2025 at 12:05:50AM +0000, grwhyte@linux.microsoft.com wrote:
> From: Graham Whyte <grwhyte@linux.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
> or readiness time reporting. The main scenario this addresses is VF
> removal and rescan during runtime repairs and driver updates, which,
> if fixed to 100ms, introduces significant delays across multiple VFs.
> These delays are unnecessary for devices that complete the FLR well
> within this timeframe.
> 

I don't think it is acceptable to *reduce* the standard delay just because your
device completes it more quickly. Proper way to reduce the timing would be to
support FRS as you said, but we cannot have arbitrary delays for random devices.

- Mani

> Patch 1 adds the flr_delay member to the pci_dev struct
> Patch 2 adds the msft device specific quirk to utilize the flr_delay
> 
> ---
> v2->v3:
> - Removed Microsoft specific pcie reset reset, replaced with customizable flr_delay parameter
> - Changed msleep in pcie_flr to usleep_range to support flr delays of under 20ms 
> v1->v2:
> - Removed unnecessary EXPORT_SYMBOL_GPL for function pci_dev_wait
> - Link to thread:https://lore.kernel.org/linux-pci/?q=f%3Agrwhyte&x=t#m7453647902a1b22840f5e39434a631fd7b2515ce'
> 
> Link to V1: https://lore.kernel.org/linux-pci/20250522085253.GN7435@unreal/T/#m7453647902a1b22840f5e39434a631fd7b2515ce  
> 
> Graham Whyte (2):
>   PCI: Add flr_delay parameter to pci_dev struct
>   PCI: Reduce FLR delay to 10ms for MSFT devices
> 
>  drivers/pci/pci.c    |  8 ++++++--
>  drivers/pci/pci.h    |  2 ++
>  drivers/pci/quirks.c | 20 ++++++++++++++++++++
>  include/linux/pci.h  |  1 +
>  4 files changed, 29 insertions(+), 2 deletions(-)
> 
> -- 
> 2.25.1
> 
> 

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

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Lukas Wunner]

On Fri, Jun 13, 2025 at 05:12:48PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 11, 2025 at 12:05:50AM +0000, grwhyte@linux.microsoft.com wrote:
> > 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
> > or readiness time reporting. The main scenario this addresses is VF
> > removal and rescan during runtime repairs and driver updates, which,
> > if fixed to 100ms, introduces significant delays across multiple VFs.
> > These delays are unnecessary for devices that complete the FLR well
> > within this timeframe.
> > 
> 
> I don't think it is acceptable to *reduce* the standard delay just
> because your device completes it more quickly. Proper way to reduce
> the timing would be to support FRS as you said, but we cannot have
> arbitrary delays for random devices.

To be fair, we already have that for certain devices:

The quirk delay_250ms_after_flr() is referenced by three different
Vendor ID / Device ID combos and *lengthens* the delay after FLR.

It's probably difficult to justify rejecting custom delays for
certain MANA devices, even though we allowed them for three other
devices.

The proposed patch introduces a generic solution which avoids
further cluttering up pci_dev_reset_methods[] with extra entries,
so I think it's an approach worth considering.

There are a bunch of nits in the proposed patches, such as "pci"
not being capitalized, but the general approach seems fine to me.

Thanks,

Lukas

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Manivannan Sadhasivam]

On Fri, Jun 13, 2025 at 03:45:35PM +0200, Lukas Wunner wrote:
> On Fri, Jun 13, 2025 at 05:12:48PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 11, 2025 at 12:05:50AM +0000, grwhyte@linux.microsoft.com wrote:
> > > 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
> > > or readiness time reporting. The main scenario this addresses is VF
> > > removal and rescan during runtime repairs and driver updates, which,
> > > if fixed to 100ms, introduces significant delays across multiple VFs.
> > > These delays are unnecessary for devices that complete the FLR well
> > > within this timeframe.
> > > 
> > 
> > I don't think it is acceptable to *reduce* the standard delay just
> > because your device completes it more quickly. Proper way to reduce
> > the timing would be to support FRS as you said, but we cannot have
> > arbitrary delays for random devices.
> 
> To be fair, we already have that for certain devices:
> 
> The quirk delay_250ms_after_flr() is referenced by three different
> Vendor ID / Device ID combos and *lengthens* the delay after FLR.
> 

This quirk is fine as it works around an issue in the device. But this patch is
not fixing/working around an issue in the device, but rather optimizing the
delay for performance, which is not what quirks are used for AFAIK.

> It's probably difficult to justify rejecting custom delays for
> certain MANA devices, even though we allowed them for three other
> devices.
> 

If the MANA devices require extended delay, then a quirk indeed makes sense.
But it is the other way around.

> The proposed patch introduces a generic solution which avoids
> further cluttering up pci_dev_reset_methods[] with extra entries,
> so I think it's an approach worth considering.
> 
> There are a bunch of nits in the proposed patches, such as "pci"
> not being capitalized, but the general approach seems fine to me.
> 

I honestly don't know if there is any other way to handle this. So I think it is
upto Bjorn to take a call.

- Mani

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

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Bjorn Helgaas]

On Thu, Jun 12, 2025 at 09:41:45AM -0700, Graham Whyte wrote:
> On 6/11/2025 11:31 PM, Christoph Hellwig wrote:
> > On Wed, Jun 11, 2025 at 01:08:21PM -0700, Graham Whyte wrote:
> >> We can ask our HW engineers to implement function readiness but we need
> >> to be able to support exiting products, hence why posting it as a quirk.
> > 
> > Your report sounds like it works perfectly fine, it's just that you
> > want to reduce the delay.  For that you'll need to stick to the standard
> > methods instead of adding quirks, which are for buggy hardware that does
> > not otherwise work.
> 
> Bjorn, what would you recommend as next steps here?

This is a tough call and I don't pretend to have an obvious answer.  I
understand the desire to improve performance.  On the other hand, PCI
has been successful over the long term because devices adhere to
standardized ways of doing things, which makes generic software
possible.  Quirks degrade that story, of course, especially when there
is an existing standardized solution that isn't being used.  I'm not
at all happy about vendors that decide against the standard solution
and then ask OS folks to do extra work to compensate.

Bjorn

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Graham Whyte]

On 6/13/2025 8:33 AM, Bjorn Helgaas wrote:
> On Thu, Jun 12, 2025 at 09:41:45AM -0700, Graham Whyte wrote:
>> On 6/11/2025 11:31 PM, Christoph Hellwig wrote:
>>> On Wed, Jun 11, 2025 at 01:08:21PM -0700, Graham Whyte wrote:
>>>> We can ask our HW engineers to implement function readiness but we need
>>>> to be able to support exiting products, hence why posting it as a quirk.
>>>
>>> Your report sounds like it works perfectly fine, it's just that you
>>> want to reduce the delay.  For that you'll need to stick to the standard
>>> methods instead of adding quirks, which are for buggy hardware that does
>>> not otherwise work.
>>
>> Bjorn, what would you recommend as next steps here?
> 
> This is a tough call and I don't pretend to have an obvious answer.  I
> understand the desire to improve performance.  On the other hand, PCI
> has been successful over the long term because devices adhere to
> standardized ways of doing things, which makes generic software
> possible.  Quirks degrade that story, of course, especially when there
> is an existing standardized solution that isn't being used.  I'm not
> at all happy about vendors that decide against the standard solution
> and then ask OS folks to do extra work to compensate.
> 
> Bjorn

Hi Bjorn,

Should someone want to implement readiness time reporting down the road,
they'll need to do the same work as patch 1 in this series (making the
flr delay a configurable parameter). This change lays the groundwork for
that work, while also supporting devices that can't use readiness time
reporting.

Alternatively could we use a sysfs file to make this parameter
configurable via the user space application? Similar to switching between
d3hot and d3cold by writing to /sys/bus/pci/slots/$DEVICE/power.

Thanks,
Graham

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Bjorn Helgaas]

On Mon, Jun 16, 2025 at 12:02:41PM -0700, Graham Whyte wrote:
> On 6/13/2025 8:33 AM, Bjorn Helgaas wrote:
> > On Thu, Jun 12, 2025 at 09:41:45AM -0700, Graham Whyte wrote:
> >> On 6/11/2025 11:31 PM, Christoph Hellwig wrote:
> >>> On Wed, Jun 11, 2025 at 01:08:21PM -0700, Graham Whyte wrote:
> >>>> We can ask our HW engineers to implement function readiness but we need
> >>>> to be able to support exiting products, hence why posting it as a quirk.
> >>>
> >>> Your report sounds like it works perfectly fine, it's just that you
> >>> want to reduce the delay.  For that you'll need to stick to the standard
> >>> methods instead of adding quirks, which are for buggy hardware that does
> >>> not otherwise work.
> >>
> >> Bjorn, what would you recommend as next steps here?
> > 
> > This is a tough call and I don't pretend to have an obvious answer.  I
> > understand the desire to improve performance.  On the other hand, PCI
> > has been successful over the long term because devices adhere to
> > standardized ways of doing things, which makes generic software
> > possible.  Quirks degrade that story, of course, especially when there
> > is an existing standardized solution that isn't being used.  I'm not
> > at all happy about vendors that decide against the standard solution
> > and then ask OS folks to do extra work to compensate.
> 
> Should someone want to implement readiness time reporting down the road,
> they'll need to do the same work as patch 1 in this series (making the
> flr delay a configurable parameter).

Sure.  That's a trivial change.  The problem is the quirk itself.

The Readiness Time Reporting Extended Capability is read-only with no
control bits in it so it requires no actual logic in the device.
Maybe you can just implement that capability with a firmware change on
the device and add the corresponding Linux support for it.

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Graham Whyte]

On 6/16/2025 2:05 PM, Bjorn Helgaas wrote:
> On Mon, Jun 16, 2025 at 12:02:41PM -0700, Graham Whyte wrote:
>> On 6/13/2025 8:33 AM, Bjorn Helgaas wrote:
>>> On Thu, Jun 12, 2025 at 09:41:45AM -0700, Graham Whyte wrote:
>>>> On 6/11/2025 11:31 PM, Christoph Hellwig wrote:
>>>>> On Wed, Jun 11, 2025 at 01:08:21PM -0700, Graham Whyte wrote:
>>>>>> We can ask our HW engineers to implement function readiness but we need
>>>>>> to be able to support exiting products, hence why posting it as a quirk.
>>>>>
>>>>> Your report sounds like it works perfectly fine, it's just that you
>>>>> want to reduce the delay.  For that you'll need to stick to the standard
>>>>> methods instead of adding quirks, which are for buggy hardware that does
>>>>> not otherwise work.
>>>>
>>>> Bjorn, what would you recommend as next steps here?
>>>
>>> This is a tough call and I don't pretend to have an obvious answer.  I
>>> understand the desire to improve performance.  On the other hand, PCI
>>> has been successful over the long term because devices adhere to
>>> standardized ways of doing things, which makes generic software
>>> possible.  Quirks degrade that story, of course, especially when there
>>> is an existing standardized solution that isn't being used.  I'm not
>>> at all happy about vendors that decide against the standard solution
>>> and then ask OS folks to do extra work to compensate.
>>
>> Should someone want to implement readiness time reporting down the road,
>> they'll need to do the same work as patch 1 in this series (making the
>> flr delay a configurable parameter).
> 
> Sure.  That's a trivial change.  The problem is the quirk itself.
> 
> The Readiness Time Reporting Extended Capability is read-only with no
> control bits in it so it requires no actual logic in the device.
> Maybe you can just implement that capability with a firmware change on
> the device and add the corresponding Linux support for it.

Hi Bjorn,

We checked with our HW folks, it's not possible for us to update the pci
register components with this particular card, they are read only. What
are your thoughts on the sysfs approach mentioned in the previous email?

Thanks,
Graham

Re: [PATCH v3 0/2] PCI: Reduce FLR delay to 10ms for MSFT devices

[Graham Whyte]

On 6/18/2025 9:42 AM, Graham Whyte wrote:
> 
> 
> On 6/16/2025 2:05 PM, Bjorn Helgaas wrote:
>> On Mon, Jun 16, 2025 at 12:02:41PM -0700, Graham Whyte wrote:
>>> On 6/13/2025 8:33 AM, Bjorn Helgaas wrote:
>>>> On Thu, Jun 12, 2025 at 09:41:45AM -0700, Graham Whyte wrote:
>>>>> On 6/11/2025 11:31 PM, Christoph Hellwig wrote:
>>>>>> On Wed, Jun 11, 2025 at 01:08:21PM -0700, Graham Whyte wrote:
>>>>>>> We can ask our HW engineers to implement function readiness but we need
>>>>>>> to be able to support exiting products, hence why posting it as a quirk.
>>>>>>
>>>>>> Your report sounds like it works perfectly fine, it's just that you
>>>>>> want to reduce the delay.  For that you'll need to stick to the standard
>>>>>> methods instead of adding quirks, which are for buggy hardware that does
>>>>>> not otherwise work.
>>>>>
>>>>> Bjorn, what would you recommend as next steps here?
>>>>
>>>> This is a tough call and I don't pretend to have an obvious answer.  I
>>>> understand the desire to improve performance.  On the other hand, PCI
>>>> has been successful over the long term because devices adhere to
>>>> standardized ways of doing things, which makes generic software
>>>> possible.  Quirks degrade that story, of course, especially when there
>>>> is an existing standardized solution that isn't being used.  I'm not
>>>> at all happy about vendors that decide against the standard solution
>>>> and then ask OS folks to do extra work to compensate.
>>>
>>> Should someone want to implement readiness time reporting down the road,
>>> they'll need to do the same work as patch 1 in this series (making the
>>> flr delay a configurable parameter).
>>
>> Sure.  That's a trivial change.  The problem is the quirk itself.
>>
>> The Readiness Time Reporting Extended Capability is read-only with no
>> control bits in it so it requires no actual logic in the device.
>> Maybe you can just implement that capability with a firmware change on
>> the device and add the corresponding Linux support for it.
> 
> Hi Bjorn,
> 
> We checked with our HW folks, it's not possible for us to update the pci
> register components with this particular card, they are read only. What
> are your thoughts on the sysfs approach mentioned in the previous email?
> 
> Thanks,
> Graham

Hi Bjorn, just wanted to follow up on this here.

Re: [PATCH v3 1/2] PCI: Add flr_delay parameter to pci_dev struct

[Bjorn Helgaas]

On Wed, Jun 11, 2025 at 12:05:51AM +0000, grwhyte@linux.microsoft.com wrote:
> From: Graham Whyte <grwhyte@linux.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@linux.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
>