Linux PCI subsystem development
 help / color / mirror / Atom feed
* [PATCH v4 0/4] PCI: Add delay macros for better code readability and maintainability
@ 2025-11-01 16:05 Hans Zhang
  2025-11-01 16:05 ` [PATCH v4 1/4] PCI: Add macro for secondary bus reset delay Hans Zhang
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Hans Zhang @ 2025-11-01 16:05 UTC (permalink / raw)
  To: bhelgaas, helgaas, linux-pci, linux-kernel; +Cc: Hans Zhang

This series improves code readability and maintainability in the PCI
subsystem by replacing hard-coded delay values with descriptive macros.

---
Hi Bjorn,

I wonder if there is still a need to advance this series? If it's not necessary,
please drop it. Thank you very much.

Best regards
Hans


Changes for v4:
https://patchwork.kernel.org/project/linux-pci/patch/20250826170315.721551-1-18255117159@163.com/

- According to Bjorn's feedback, the benefits of using fsleep are not significant.
  drop the 0002 patch in v3. (Bjorn)
- For the controller drivers, the added macros do no good and provide no value.
  So if you ever respin this series, you can drop them. (Mani)

Changes for v3:
https://patchwork.kernel.org/project/linux-pci/cover/20250822155908.625553-1-18255117159@163.com/

- According to Bjorn's suggestion, split the first patch of v2 and add
  macro definitions to the remaining patches.

Changes for v2:
https://patchwork.kernel.org/project/linux-pci/patch/20250820160944.489061-1-18255117159@163.com/

- According to the Maintainer's suggestion, it was modified to fsleep,
  usleep_range, and macro definitions were used instead of hard code. (Bjorn)
---

Hans Zhang (4):
  PCI: Add macro for secondary bus reset delay
  PCI: Add macro for link status check delay
  PCI: pciehp: Add macros for hotplug operation delays
  PCI/DPC: Add macro for RP busy check delay

 drivers/pci/hotplug/pciehp_hpc.c |  7 +++++--
 drivers/pci/pci.c                | 11 +++++------
 drivers/pci/pci.h                |  3 +++
 drivers/pci/pcie/dpc.c           |  4 +++-
 4 files changed, 16 insertions(+), 9 deletions(-)


base-commit: ba36dd5ee6fd4643ebbf6ee6eefcecf0b07e35c7
-- 
2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 1/4] PCI: Add macro for secondary bus reset delay
  2025-11-01 16:05 [PATCH v4 0/4] PCI: Add delay macros for better code readability and maintainability Hans Zhang
@ 2025-11-01 16:05 ` Hans Zhang
  2025-11-05 15:56   ` Hans Zhang
  2025-11-01 16:05 ` [PATCH v4 2/4] PCI: Add macro for link status check delay Hans Zhang
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Hans Zhang @ 2025-11-01 16:05 UTC (permalink / raw)
  To: bhelgaas, helgaas, linux-pci, linux-kernel; +Cc: Hans Zhang

Add PCI_T_RST_SEC_BUS_DELAY_MS macro for the secondary bus reset
delay value according to PCIe r7.0 spec, section 7.5.1.3.13.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/pci.c | 7 ++-----
 drivers/pci/pci.h | 3 +++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b14dd064006c..86449f2d627b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4912,11 +4912,8 @@ void pci_reset_secondary_bus(struct pci_dev *dev)
 	ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
 
-	/*
-	 * PCI spec v3.0 7.6.4.2 requires minimum Trst of 1ms.  Double
-	 * this to 2ms to ensure that we meet the minimum requirement.
-	 */
-	msleep(2);
+	/* Double this to 2ms to ensure that we meet the minimum requirement */
+	msleep(2 * PCI_T_RST_SEC_BUS_DELAY_MS);
 
 	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4492b809094b..31f975619774 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -63,6 +63,9 @@ struct pcie_tlp_log;
 #define PCIE_LINK_WAIT_MAX_RETRIES	10
 #define PCIE_LINK_WAIT_SLEEP_MS		90
 
+/* PCIe r7.0, sec 7.5.1.3.13, requires minimum Trst of 1ms */
+#define PCI_T_RST_SEC_BUS_DELAY_MS	1
+
 /* Message Routing (r[2:0]); PCIe r6.0, sec 2.2.8 */
 #define PCIE_MSG_TYPE_R_RC	0
 #define PCIE_MSG_TYPE_R_ADDR	1
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 2/4] PCI: Add macro for link status check delay
  2025-11-01 16:05 [PATCH v4 0/4] PCI: Add delay macros for better code readability and maintainability Hans Zhang
  2025-11-01 16:05 ` [PATCH v4 1/4] PCI: Add macro for secondary bus reset delay Hans Zhang
@ 2025-11-01 16:05 ` Hans Zhang
  2025-11-03 15:39   ` Bjorn Helgaas
  2025-11-01 16:05 ` [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays Hans Zhang
  2025-11-01 16:05 ` [PATCH v4 4/4] PCI/DPC: Add macro for RP busy check delay Hans Zhang
  3 siblings, 1 reply; 13+ messages in thread
From: Hans Zhang @ 2025-11-01 16:05 UTC (permalink / raw)
  To: bhelgaas, helgaas, linux-pci, linux-kernel; +Cc: Hans Zhang

Add PCIE_LINK_STATUS_CHECK_MS macro for link status check delay.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 86449f2d627b..b57d8e4c3a48 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -77,6 +77,8 @@ struct pci_pme_device {
  */
 #define PCIE_RESET_READY_POLL_MS 60000 /* msec */
 
+#define PCIE_LINK_STATUS_CHECK_MS 1
+
 static void pci_dev_d3_sleep(struct pci_dev *dev)
 {
 	unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay);
@@ -4632,7 +4634,7 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
 		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
 		if ((lnksta & lnksta_mask) == lnksta_match)
 			return 0;
-		msleep(1);
+		msleep(PCIE_LINK_STATUS_CHECK_MS);
 	} while (time_before(jiffies, end_jiffies));
 
 	return -ETIMEDOUT;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays
  2025-11-01 16:05 [PATCH v4 0/4] PCI: Add delay macros for better code readability and maintainability Hans Zhang
  2025-11-01 16:05 ` [PATCH v4 1/4] PCI: Add macro for secondary bus reset delay Hans Zhang
  2025-11-01 16:05 ` [PATCH v4 2/4] PCI: Add macro for link status check delay Hans Zhang
@ 2025-11-01 16:05 ` Hans Zhang
  2025-11-01 16:13   ` Christophe JAILLET
  2025-11-03 15:37   ` Bjorn Helgaas
  2025-11-01 16:05 ` [PATCH v4 4/4] PCI/DPC: Add macro for RP busy check delay Hans Zhang
  3 siblings, 2 replies; 13+ messages in thread
From: Hans Zhang @ 2025-11-01 16:05 UTC (permalink / raw)
  To: bhelgaas, helgaas, linux-pci, linux-kernel; +Cc: Hans Zhang

Add WAIT_PDS_TIMEOUT_MS and POLL_CMD_TIMEOUT_MS macros for hotplug
operation delays to improve code readability.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bcc51b26d03d..15b09c6a8d6b 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -28,6 +28,9 @@
 #include "../pci.h"
 #include "pciehp.h"
 
+#define WAIT_PDS_TIMEOUT_MS	10
+#define POLL_CMD_TIMEOUT_MS	10
+
 static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
 	/*
 	 * Match all Dell systems, as some Dell systems have inband
@@ -103,7 +106,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 			smp_mb();
 			return 1;
 		}
-		msleep(10);
+		msleep(POLL_CMD_TIMEOUT_MS);
 		timeout -= 10;
 	} while (timeout >= 0);
 	return 0;	/* timeout */
@@ -283,7 +286,7 @@ static void pcie_wait_for_presence(struct pci_dev *pdev)
 		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
 		if (slot_status & PCI_EXP_SLTSTA_PDS)
 			return;
-		msleep(10);
+		msleep(WAIT_PDS_TIMEOUT_MS);
 		timeout -= 10;
 	} while (timeout > 0);
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 4/4] PCI/DPC: Add macro for RP busy check delay
  2025-11-01 16:05 [PATCH v4 0/4] PCI: Add delay macros for better code readability and maintainability Hans Zhang
                   ` (2 preceding siblings ...)
  2025-11-01 16:05 ` [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays Hans Zhang
@ 2025-11-01 16:05 ` Hans Zhang
  3 siblings, 0 replies; 13+ messages in thread
From: Hans Zhang @ 2025-11-01 16:05 UTC (permalink / raw)
  To: bhelgaas, helgaas, linux-pci, linux-kernel; +Cc: Hans Zhang

Add PCIE_EXP_DPC_BUSY_CHECK_MS macro for RP busy check delay.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/pcie/dpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index fc18349614d7..bd5e8cd9e43e 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -21,6 +21,8 @@
 #define PCI_EXP_DPC_CTL_EN_MASK	(PCI_EXP_DPC_CTL_EN_FATAL | \
 				 PCI_EXP_DPC_CTL_EN_NONFATAL)
 
+#define PCIE_EXP_DPC_BUSY_CHECK_MS	10
+
 static const char * const rp_pio_error_string[] = {
 	"Configuration Request received UR Completion",	 /* Bit Position 0  */
 	"Configuration Request received CA Completion",	 /* Bit Position 1  */
@@ -135,7 +137,7 @@ static int dpc_wait_rp_inactive(struct pci_dev *pdev)
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	while (status & PCI_EXP_DPC_RP_BUSY &&
 					!time_after(jiffies, timeout)) {
-		msleep(10);
+		msleep(PCIE_EXP_DPC_BUSY_CHECK_MS);
 		pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	}
 	if (status & PCI_EXP_DPC_RP_BUSY) {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays
  2025-11-01 16:05 ` [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays Hans Zhang
@ 2025-11-01 16:13   ` Christophe JAILLET
  2025-11-01 16:16     ` Hans Zhang
  2025-11-03 15:37   ` Bjorn Helgaas
  1 sibling, 1 reply; 13+ messages in thread
From: Christophe JAILLET @ 2025-11-01 16:13 UTC (permalink / raw)
  To: Hans Zhang, bhelgaas, helgaas, linux-pci, linux-kernel

Le 01/11/2025 à 17:05, Hans Zhang a écrit :
> Add WAIT_PDS_TIMEOUT_MS and POLL_CMD_TIMEOUT_MS macros for hotplug
> operation delays to improve code readability.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>   drivers/pci/hotplug/pciehp_hpc.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bcc51b26d03d..15b09c6a8d6b 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -28,6 +28,9 @@
>   #include "../pci.h"
>   #include "pciehp.h"
>   
> +#define WAIT_PDS_TIMEOUT_MS	10
> +#define POLL_CMD_TIMEOUT_MS	10
> +
>   static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
>   	/*
>   	 * Match all Dell systems, as some Dell systems have inband
> @@ -103,7 +106,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>   			smp_mb();
>   			return 1;
>   		}
> -		msleep(10);
> +		msleep(POLL_CMD_TIMEOUT_MS);
>   		timeout -= 10;

Should we have: timeout -= POLL_CMD_TIMEOUT_MS;

?

>   	} while (timeout >= 0);
>   	return 0;	/* timeout */
> @@ -283,7 +286,7 @@ static void pcie_wait_for_presence(struct pci_dev *pdev)
>   		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
>   		if (slot_status & PCI_EXP_SLTSTA_PDS)
>   			return;
> -		msleep(10);
> +		msleep(WAIT_PDS_TIMEOUT_MS);

Same with WAIT_PDS_TIMEOUT_MS.

CJ

>   		timeout -= 10;
>   	} while (timeout > 0);
>   }


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays
  2025-11-01 16:13   ` Christophe JAILLET
@ 2025-11-01 16:16     ` Hans Zhang
  0 siblings, 0 replies; 13+ messages in thread
From: Hans Zhang @ 2025-11-01 16:16 UTC (permalink / raw)
  To: Christophe JAILLET, bhelgaas, helgaas, linux-pci, linux-kernel



On 2025/11/2 00:13, Christophe JAILLET wrote:
> Le 01/11/2025 à 17:05, Hans Zhang a écrit :
>> Add WAIT_PDS_TIMEOUT_MS and POLL_CMD_TIMEOUT_MS macros for hotplug
>> operation delays to improve code readability.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   drivers/pci/hotplug/pciehp_hpc.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/ 
>> pciehp_hpc.c
>> index bcc51b26d03d..15b09c6a8d6b 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -28,6 +28,9 @@
>>   #include "../pci.h"
>>   #include "pciehp.h"
>> +#define WAIT_PDS_TIMEOUT_MS    10
>> +#define POLL_CMD_TIMEOUT_MS    10
>> +
>>   static const struct dmi_system_id 
>> inband_presence_disabled_dmi_table[] = {
>>       /*
>>        * Match all Dell systems, as some Dell systems have inband
>> @@ -103,7 +106,7 @@ static int pcie_poll_cmd(struct controller *ctrl, 
>> int timeout)
>>               smp_mb();
>>               return 1;
>>           }
>> -        msleep(10);
>> +        msleep(POLL_CMD_TIMEOUT_MS);
>>           timeout -= 10;
> 
> Should we have: timeout -= POLL_CMD_TIMEOUT_MS;
> 
> ?

Hi Christophe,

Thank you very much for your reply and reminder.

If Bjorn finds it meaningful, I will fix it in the next version.


> 
>>       } while (timeout >= 0);
>>       return 0;    /* timeout */
>> @@ -283,7 +286,7 @@ static void pcie_wait_for_presence(struct pci_dev 
>> *pdev)
>>           pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
>>           if (slot_status & PCI_EXP_SLTSTA_PDS)
>>               return;
>> -        msleep(10);
>> +        msleep(WAIT_PDS_TIMEOUT_MS);
> 
> Same with WAIT_PDS_TIMEOUT_MS.
> 

Will change.

Best regards,
Hans

> CJ
> 
>>           timeout -= 10;
>>       } while (timeout > 0);
>>   }


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays
  2025-11-01 16:05 ` [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays Hans Zhang
  2025-11-01 16:13   ` Christophe JAILLET
@ 2025-11-03 15:37   ` Bjorn Helgaas
  2025-11-03 17:24     ` Lukas Wunner
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2025-11-03 15:37 UTC (permalink / raw)
  To: Hans Zhang; +Cc: bhelgaas, linux-pci, linux-kernel, Lukas Wunner

[+cc Lukas]

On Sun, Nov 02, 2025 at 12:05:37AM +0800, Hans Zhang wrote:
> Add WAIT_PDS_TIMEOUT_MS and POLL_CMD_TIMEOUT_MS macros for hotplug
> operation delays to improve code readability.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bcc51b26d03d..15b09c6a8d6b 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -28,6 +28,9 @@
>  #include "../pci.h"
>  #include "pciehp.h"
>  
> +#define WAIT_PDS_TIMEOUT_MS	10
> +#define POLL_CMD_TIMEOUT_MS	10
> +
>  static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
>  	/*
>  	 * Match all Dell systems, as some Dell systems have inband
> @@ -103,7 +106,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>  			smp_mb();
>  			return 1;
>  		}
> -		msleep(10);
> +		msleep(POLL_CMD_TIMEOUT_MS);

Lukas might have different opinions and I would defer to him here.

But IMO (a) these aren't timeouts, they are poll intervals, (b) the
values are arbitrary with no connection to a spec, so less reason for
a #define, and (c) the #defines don't improve readability because now
I have to look at two places to understand the poll loops.

>  		timeout -= 10;
>  	} while (timeout >= 0);
>  	return 0;	/* timeout */
> @@ -283,7 +286,7 @@ static void pcie_wait_for_presence(struct pci_dev *pdev)
>  		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
>  		if (slot_status & PCI_EXP_SLTSTA_PDS)
>  			return;
> -		msleep(10);
> +		msleep(WAIT_PDS_TIMEOUT_MS);
>  		timeout -= 10;
>  	} while (timeout > 0);
>  }
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/4] PCI: Add macro for link status check delay
  2025-11-01 16:05 ` [PATCH v4 2/4] PCI: Add macro for link status check delay Hans Zhang
@ 2025-11-03 15:39   ` Bjorn Helgaas
  2025-11-05 15:54     ` Hans Zhang
  0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2025-11-03 15:39 UTC (permalink / raw)
  To: Hans Zhang; +Cc: bhelgaas, linux-pci, linux-kernel

On Sun, Nov 02, 2025 at 12:05:36AM +0800, Hans Zhang wrote:
> Add PCIE_LINK_STATUS_CHECK_MS macro for link status check delay.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/pci/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 86449f2d627b..b57d8e4c3a48 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -77,6 +77,8 @@ struct pci_pme_device {
>   */
>  #define PCIE_RESET_READY_POLL_MS 60000 /* msec */
>  
> +#define PCIE_LINK_STATUS_CHECK_MS 1
> +
>  static void pci_dev_d3_sleep(struct pci_dev *dev)
>  {
>  	unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay);
> @@ -4632,7 +4634,7 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
>  		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
>  		if ((lnksta & lnksta_mask) == lnksta_match)
>  			return 0;
> -		msleep(1);
> +		msleep(PCIE_LINK_STATUS_CHECK_MS);

Doesn't seem worth it to me.

>  	} while (time_before(jiffies, end_jiffies));
>  
>  	return -ETIMEDOUT;
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays
  2025-11-03 15:37   ` Bjorn Helgaas
@ 2025-11-03 17:24     ` Lukas Wunner
  2025-11-05 15:54       ` Hans Zhang
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Wunner @ 2025-11-03 17:24 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Hans Zhang, bhelgaas, linux-pci, linux-kernel

On Mon, Nov 03, 2025 at 09:37:34AM -0600, Bjorn Helgaas wrote:
> On Sun, Nov 02, 2025 at 12:05:37AM +0800, Hans Zhang wrote:
> > Add WAIT_PDS_TIMEOUT_MS and POLL_CMD_TIMEOUT_MS macros for hotplug
> > operation delays to improve code readability.
[...]
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -28,6 +28,9 @@
> >  #include "../pci.h"
> >  #include "pciehp.h"
> >  
> > +#define WAIT_PDS_TIMEOUT_MS	10
> > +#define POLL_CMD_TIMEOUT_MS	10
> > +
> >  static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
> >  	/*
> >  	 * Match all Dell systems, as some Dell systems have inband
> > @@ -103,7 +106,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
> >  			smp_mb();
> >  			return 1;
> >  		}
> > -		msleep(10);
> > +		msleep(POLL_CMD_TIMEOUT_MS);
> 
> Lukas might have different opinions and I would defer to him here.
> 
> But IMO (a) these aren't timeouts, they are poll intervals, (b) the
> values are arbitrary with no connection to a spec, so less reason for
> a #define, and (c) the #defines don't improve readability because now
> I have to look at two places to understand the poll loops.

I agree on all counts.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays
  2025-11-03 17:24     ` Lukas Wunner
@ 2025-11-05 15:54       ` Hans Zhang
  0 siblings, 0 replies; 13+ messages in thread
From: Hans Zhang @ 2025-11-05 15:54 UTC (permalink / raw)
  To: Lukas Wunner, Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel



On 2025/11/4 01:24, Lukas Wunner wrote:
> On Mon, Nov 03, 2025 at 09:37:34AM -0600, Bjorn Helgaas wrote:
>> On Sun, Nov 02, 2025 at 12:05:37AM +0800, Hans Zhang wrote:
>>> Add WAIT_PDS_TIMEOUT_MS and POLL_CMD_TIMEOUT_MS macros for hotplug
>>> operation delays to improve code readability.
> [...]
>>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>>> @@ -28,6 +28,9 @@
>>>   #include "../pci.h"
>>>   #include "pciehp.h"
>>>   
>>> +#define WAIT_PDS_TIMEOUT_MS	10
>>> +#define POLL_CMD_TIMEOUT_MS	10
>>> +
>>>   static const struct dmi_system_id inband_presence_disabled_dmi_table[] = {
>>>   	/*
>>>   	 * Match all Dell systems, as some Dell systems have inband
>>> @@ -103,7 +106,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>>>   			smp_mb();
>>>   			return 1;
>>>   		}
>>> -		msleep(10);
>>> +		msleep(POLL_CMD_TIMEOUT_MS);
>>
>> Lukas might have different opinions and I would defer to him here.
>>
>> But IMO (a) these aren't timeouts, they are poll intervals, (b) the
>> values are arbitrary with no connection to a spec, so less reason for
>> a #define, and (c) the #defines don't improve readability because now
>> I have to look at two places to understand the poll loops.
> 
> I agree on all counts.
> 

Hi Bjorn,

Please drop this patch.

Best regards,
Hans


> Thanks,
> 
> Lukas


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/4] PCI: Add macro for link status check delay
  2025-11-03 15:39   ` Bjorn Helgaas
@ 2025-11-05 15:54     ` Hans Zhang
  0 siblings, 0 replies; 13+ messages in thread
From: Hans Zhang @ 2025-11-05 15:54 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel



On 2025/11/3 23:39, Bjorn Helgaas wrote:
> On Sun, Nov 02, 2025 at 12:05:36AM +0800, Hans Zhang wrote:
>> Add PCIE_LINK_STATUS_CHECK_MS macro for link status check delay.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   drivers/pci/pci.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 86449f2d627b..b57d8e4c3a48 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -77,6 +77,8 @@ struct pci_pme_device {
>>    */
>>   #define PCIE_RESET_READY_POLL_MS 60000 /* msec */
>>   
>> +#define PCIE_LINK_STATUS_CHECK_MS 1
>> +
>>   static void pci_dev_d3_sleep(struct pci_dev *dev)
>>   {
>>   	unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay);
>> @@ -4632,7 +4634,7 @@ static int pcie_wait_for_link_status(struct pci_dev *pdev,
>>   		pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
>>   		if ((lnksta & lnksta_mask) == lnksta_match)
>>   			return 0;
>> -		msleep(1);
>> +		msleep(PCIE_LINK_STATUS_CHECK_MS);
> 
> Doesn't seem worth it to me.

Hi Bjorn,

Please drop this patch.

Best regards,
Hans

> 
>>   	} while (time_before(jiffies, end_jiffies));
>>   
>>   	return -ETIMEDOUT;
>> -- 
>> 2.34.1
>>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/4] PCI: Add macro for secondary bus reset delay
  2025-11-01 16:05 ` [PATCH v4 1/4] PCI: Add macro for secondary bus reset delay Hans Zhang
@ 2025-11-05 15:56   ` Hans Zhang
  0 siblings, 0 replies; 13+ messages in thread
From: Hans Zhang @ 2025-11-05 15:56 UTC (permalink / raw)
  To: bhelgaas, helgaas, linux-pci, linux-kernel

Hi Bjorn,

I wonder if this is still necessary? If not, please drop it as well. Thanks.

Best regards,
Hans

On 2025/11/2 00:05, Hans Zhang wrote:
> Add PCI_T_RST_SEC_BUS_DELAY_MS macro for the secondary bus reset
> delay value according to PCIe r7.0 spec, section 7.5.1.3.13.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>   drivers/pci/pci.c | 7 ++-----
>   drivers/pci/pci.h | 3 +++
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b14dd064006c..86449f2d627b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4912,11 +4912,8 @@ void pci_reset_secondary_bus(struct pci_dev *dev)
>   	ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
>   	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
>   
> -	/*
> -	 * PCI spec v3.0 7.6.4.2 requires minimum Trst of 1ms.  Double
> -	 * this to 2ms to ensure that we meet the minimum requirement.
> -	 */
> -	msleep(2);
> +	/* Double this to 2ms to ensure that we meet the minimum requirement */
> +	msleep(2 * PCI_T_RST_SEC_BUS_DELAY_MS);
>   
>   	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
>   	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4492b809094b..31f975619774 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -63,6 +63,9 @@ struct pcie_tlp_log;
>   #define PCIE_LINK_WAIT_MAX_RETRIES	10
>   #define PCIE_LINK_WAIT_SLEEP_MS		90
>   
> +/* PCIe r7.0, sec 7.5.1.3.13, requires minimum Trst of 1ms */
> +#define PCI_T_RST_SEC_BUS_DELAY_MS	1
> +
>   /* Message Routing (r[2:0]); PCIe r6.0, sec 2.2.8 */
>   #define PCIE_MSG_TYPE_R_RC	0
>   #define PCIE_MSG_TYPE_R_ADDR	1


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-11-05 15:56 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-01 16:05 [PATCH v4 0/4] PCI: Add delay macros for better code readability and maintainability Hans Zhang
2025-11-01 16:05 ` [PATCH v4 1/4] PCI: Add macro for secondary bus reset delay Hans Zhang
2025-11-05 15:56   ` Hans Zhang
2025-11-01 16:05 ` [PATCH v4 2/4] PCI: Add macro for link status check delay Hans Zhang
2025-11-03 15:39   ` Bjorn Helgaas
2025-11-05 15:54     ` Hans Zhang
2025-11-01 16:05 ` [PATCH v4 3/4] PCI: pciehp: Add macros for hotplug operation delays Hans Zhang
2025-11-01 16:13   ` Christophe JAILLET
2025-11-01 16:16     ` Hans Zhang
2025-11-03 15:37   ` Bjorn Helgaas
2025-11-03 17:24     ` Lukas Wunner
2025-11-05 15:54       ` Hans Zhang
2025-11-01 16:05 ` [PATCH v4 4/4] PCI/DPC: Add macro for RP busy check delay Hans Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox