2020-09-27 03:31:21

by Ethan Zhao

[permalink] [raw]
Subject: [PATCH 1/5 V2] PCI: define a function to check and wait till port finish DPC handling

Once root port DPC capability is enabled and triggered, at the beginning
of DPC is triggered, the DPC status bits are set by hardware and then
sends DPC/DLLSC/PDC interrupts to OS DPC and pciehp drivers, it will
take the port and software DPC interrupt handler 10ms to 50ms (test data
on ICS(Ice Lake SP platform, see
https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server)
& stable 5.9-rc6) to complete the DPC containment procedure
till the DPC status is cleared at the end of the DPC interrupt handler.

We use this function to check if the root port is in DPC handling status
and wait till the hardware and software completed the procedure.

Signed-off-by: Ethan Zhao <[email protected]>
Tested-by: Wen Jin <[email protected]>
Tested-by: Shanshan Zhang <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
changes:
V2:align ICS code name to public doc.
include/linux/pci.h | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 835530605c0d..5beb76c6ae26 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -38,6 +38,7 @@
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/resource_ext.h>
+#include <linux/delay.h>
#include <uapi/linux/pci.h>

#include <linux/pci_ids.h>
@@ -2427,4 +2428,34 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type);
WARN_ONCE(condition, "%s %s: " fmt, \
dev_driver_string(&(pdev)->dev), pci_name(pdev), ##arg)

+#ifdef CONFIG_PCIE_DPC
+static inline bool pci_wait_port_outdpc(struct pci_dev *pdev)
+{
+ u16 cap = pdev->dpc_cap, status;
+ u16 loop = 0;
+
+ if (!cap) {
+ pci_WARN_ONCE(pdev, !cap, "No DPC capability initiated\n");
+ return false;
+ }
+ pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
+ pci_dbg(pdev, "DPC status %x, cap %x\n", status, cap);
+ while (status & PCI_EXP_DPC_STATUS_TRIGGER && loop < 100) {
+ msleep(10);
+ loop++;
+ pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
+ }
+ if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
+ pci_dbg(pdev, "Out of DPC %x, cost %d ms\n", status, loop*10);
+ return true;
+ }
+ pci_dbg(pdev, "Timeout to wait port out of DPC status\n");
+ return false;
+}
+#else
+static inline bool pci_wait_port_outdpc(struct pci_dev *pdev)
+{
+ return true;
+}
+#endif
#endif /* LINUX_PCI_H */
--
2.18.4


2020-09-27 06:26:31

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/5 V2] PCI: define a function to check and wait till port finish DPC handling

> +#ifdef CONFIG_PCIE_DPC
> +static inline bool pci_wait_port_outdpc(struct pci_dev *pdev)
> +{
> + u16 cap = pdev->dpc_cap, status;
> + u16 loop = 0;
> +
> + if (!cap) {
> + pci_WARN_ONCE(pdev, !cap, "No DPC capability initiated\n");
> + return false;
> + }
> + pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> + pci_dbg(pdev, "DPC status %x, cap %x\n", status, cap);
> + while (status & PCI_EXP_DPC_STATUS_TRIGGER && loop < 100) {
> + msleep(10);
> + loop++;
> + pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> + }
> + if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
> + pci_dbg(pdev, "Out of DPC %x, cost %d ms\n", status, loop*10);
> + return true;
> + }
> + pci_dbg(pdev, "Timeout to wait port out of DPC status\n");
> + return false;
> +}

I don't think that there is any good reason to have this as an
inline function.

2020-09-27 06:52:05

by Ethan Zhao

[permalink] [raw]
Subject: RE: [PATCH 1/5 V2] PCI: define a function to check and wait till port finish DPC handling

Yep, I am think the same question, is there any other files better to put this function ?
How about pci.c ?

Thanks,
Ethan

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Sunday, September 27, 2020 2:24 PM
To: Zhao, Haifeng <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Jia, Pei P <[email protected]>; [email protected]; Kuppuswamy, Sathyanarayanan <[email protected]>
Subject: Re: [PATCH 1/5 V2] PCI: define a function to check and wait till port finish DPC handling

> +#ifdef CONFIG_PCIE_DPC
> +static inline bool pci_wait_port_outdpc(struct pci_dev *pdev) {
> + u16 cap = pdev->dpc_cap, status;
> + u16 loop = 0;
> +
> + if (!cap) {
> + pci_WARN_ONCE(pdev, !cap, "No DPC capability initiated\n");
> + return false;
> + }
> + pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> + pci_dbg(pdev, "DPC status %x, cap %x\n", status, cap);
> + while (status & PCI_EXP_DPC_STATUS_TRIGGER && loop < 100) {
> + msleep(10);
> + loop++;
> + pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> + }
> + if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
> + pci_dbg(pdev, "Out of DPC %x, cost %d ms\n", status, loop*10);
> + return true;
> + }
> + pci_dbg(pdev, "Timeout to wait port out of DPC status\n");
> + return false;
> +}

I don't think that there is any good reason to have this as an inline function.

2020-09-29 02:37:39

by Ethan Zhao

[permalink] [raw]
Subject: Re: [PATCH 1/5 V2] PCI: define a function to check and wait till port finish DPC handling

Fixed this concern by moving the function to DPC driver and its
declaration to pci.h. see v5

Thanks,
Ethan

On Sun, Sep 27, 2020 at 2:27 PM Christoph Hellwig <[email protected]> wrote:
>
> > +#ifdef CONFIG_PCIE_DPC
> > +static inline bool pci_wait_port_outdpc(struct pci_dev *pdev)
> > +{
> > + u16 cap = pdev->dpc_cap, status;
> > + u16 loop = 0;
> > +
> > + if (!cap) {
> > + pci_WARN_ONCE(pdev, !cap, "No DPC capability initiated\n");
> > + return false;
> > + }
> > + pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> > + pci_dbg(pdev, "DPC status %x, cap %x\n", status, cap);
> > + while (status & PCI_EXP_DPC_STATUS_TRIGGER && loop < 100) {
> > + msleep(10);
> > + loop++;
> > + pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> > + }
> > + if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
> > + pci_dbg(pdev, "Out of DPC %x, cost %d ms\n", status, loop*10);
> > + return true;
> > + }
> > + pci_dbg(pdev, "Timeout to wait port out of DPC status\n");
> > + return false;
> > +}
>
> I don't think that there is any good reason to have this as an
> inline function.