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]>
---
changes:
v2：align ICS code name to public doc.
v3: no change.
v4: response to Christoph's (Christoph Hellwig <[email protected]>)
tip, move pci_wait_port_outdpc() to DPC driver and its declaration
to pci.h.
v5: fix building issue reported by [email protected] with some config.
v6: move from [1/5] to [2/5].
v7: no change.
v8: no change.
drivers/pci/pci.h | 2 ++
drivers/pci/pcie/dpc.c | 27 +++++++++++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..455b32187abd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -455,10 +455,12 @@ void pci_restore_dpc_state(struct pci_dev *dev);
void pci_dpc_init(struct pci_dev *pdev);
void dpc_process_error(struct pci_dev *pdev);
pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
+bool pci_wait_port_outdpc(struct pci_dev *pdev);
#else
static inline void pci_save_dpc_state(struct pci_dev *dev) {}
static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
static inline void pci_dpc_init(struct pci_dev *pdev) {}
+static inline bool pci_wait_port_outdpc(struct pci_dev *pdev) { return false; }
#endif
#ifdef CONFIG_PCI_ATS
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index daa9a4153776..2e0e091ce923 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -71,6 +71,33 @@ void pci_restore_dpc_state(struct pci_dev *dev)
pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
}
+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;
+}
+
static int dpc_wait_rp_inactive(struct pci_dev *pdev)
{
unsigned long timeout = jiffies + HZ;
--
2.18.4
On 10/7/20 4:31 AM, Ethan Zhao wrote:
> 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
This data is based on one particular architecture. So using this
to create a timed loop in pci_wait_port_outdpc() looks incorrect.
I still recommend looking for some locking model to fix this
issue (may be atomic state flag or lock).
> 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]>
> ---
> changes:
> v2：align ICS code name to public doc.
> v3: no change.
> v4: response to Christoph's (Christoph Hellwig <[email protected]>)
> tip, move pci_wait_port_outdpc() to DPC driver and its declaration
> to pci.h.
> v5: fix building issue reported by [email protected] with some config.
> v6: move from [1/5] to [2/5].
> v7: no change.
> v8: no change.
>
> drivers/pci/pci.h | 2 ++
> drivers/pci/pcie/dpc.c | 27 +++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fa12f7cbc1a0..455b32187abd 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -455,10 +455,12 @@ void pci_restore_dpc_state(struct pci_dev *dev);
> void pci_dpc_init(struct pci_dev *pdev);
> void dpc_process_error(struct pci_dev *pdev);
> pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
> +bool pci_wait_port_outdpc(struct pci_dev *pdev);
> #else
> static inline void pci_save_dpc_state(struct pci_dev *dev) {}
> static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
> static inline void pci_dpc_init(struct pci_dev *pdev) {}
> +static inline bool pci_wait_port_outdpc(struct pci_dev *pdev) { return false; }
> #endif
>
> #ifdef CONFIG_PCI_ATS
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index daa9a4153776..2e0e091ce923 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -71,6 +71,33 @@ void pci_restore_dpc_state(struct pci_dev *dev)
> pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
> }
>
> +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;
> +}
> +
> static int dpc_wait_rp_inactive(struct pci_dev *pdev)
> {
> unsigned long timeout = jiffies + HZ;
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Thu, Oct 8, 2020 at 2:16 AM Kuppuswamy, Sathyanarayanan
<[email protected]> wrote:
>
>
> On 10/7/20 4:31 AM, Ethan Zhao wrote:
> > 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
> This data is based on one particular architecture. So using this
> to create a timed loop in pci_wait_port_outdpc() looks incorrect.
>
> I still recommend looking for some locking model to fix this
> issue (may be atomic state flag or lock).
It is actually a device semaphore. DLLSC/PDC handler needs to wait
for the critical
area is clear to enter by monitoring the DPC triggered status is
cleaned or not.
Another problem is, DPC reset/interrupt is initiated by hardware,
you couldn't place
a software lock between interrupt handler and device resetting.
While device semaphore--- DPC triggered status is the right one to wait for.
Better idea ?
Thanks,
Ethan
> > 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]>
> > ---
> > changes:
> > v2：align ICS code name to public doc.
> > v3: no change.
> > v4: response to Christoph's (Christoph Hellwig <[email protected]>)
> > tip, move pci_wait_port_outdpc() to DPC driver and its declaration
> > to pci.h.
> > v5: fix building issue reported by [email protected] with some config.
> > v6: move from [1/5] to [2/5].
> > v7: no change.
> > v8: no change.
> >
> > drivers/pci/pci.h | 2 ++
> > drivers/pci/pcie/dpc.c | 27 +++++++++++++++++++++++++++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index fa12f7cbc1a0..455b32187abd 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -455,10 +455,12 @@ void pci_restore_dpc_state(struct pci_dev *dev);
> > void pci_dpc_init(struct pci_dev *pdev);
> > void dpc_process_error(struct pci_dev *pdev);
> > pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
> > +bool pci_wait_port_outdpc(struct pci_dev *pdev);
> > #else
> > static inline void pci_save_dpc_state(struct pci_dev *dev) {}
> > static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
> > static inline void pci_dpc_init(struct pci_dev *pdev) {}
> > +static inline bool pci_wait_port_outdpc(struct pci_dev *pdev) { return false; }
> > #endif
> >
> > #ifdef CONFIG_PCI_ATS
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index daa9a4153776..2e0e091ce923 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -71,6 +71,33 @@ void pci_restore_dpc_state(struct pci_dev *dev)
> > pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
> > }
> >
> > +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;
> > +}
> > +
> > static int dpc_wait_rp_inactive(struct pci_dev *pdev)
> > {
> > unsigned long timeout = jiffies + HZ;
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>
On Thu, Oct 8, 2020 at 2:16 AM Kuppuswamy, Sathyanarayanan
<[email protected]> wrote:
>
>
> On 10/7/20 4:31 AM, Ethan Zhao wrote:
> > 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
> This data is based on one particular architecture. So using this
> to create a timed loop in pci_wait_port_outdpc() looks incorrect.
To clarify there, it is not random to wait for specific 1000ms for
specific architecture.
Though there is no specification to say how many ms totally cost by
hardware DPC containment,
plus its interrupt handling. but you could read the whole PCIe
specification to know how
many ms cost at most by power state transition, link training etc, you
may know the max single delay
in hardware state transition is 200ms. Other small delay in hardware
state transition is 100ms,
48ms, 32ms etc.
If the DPC containment hardware procedure is pure resetting (or cold
power on) without
software access configuration as the worst case. we wait its handling
process from 10ms
(actually 20ms is the minimum delay we could do with msleep() ) till
1000ms timeout is a
reasonable value.
Thanks,
Ethan
>
> I still recommend looking for some locking model to fix this
> issue (may be atomic state flag or lock).
> > 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]>
> > ---
> > changes:
> > v2：align ICS code name to public doc.
> > v3: no change.
> > v4: response to Christoph's (Christoph Hellwig <[email protected]>)
> > tip, move pci_wait_port_outdpc() to DPC driver and its declaration
> > to pci.h.
> > v5: fix building issue reported by [email protected] with some config.
> > v6: move from [1/5] to [2/5].
> > v7: no change.
> > v8: no change.
> >
> > drivers/pci/pci.h | 2 ++
> > drivers/pci/pcie/dpc.c | 27 +++++++++++++++++++++++++++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index fa12f7cbc1a0..455b32187abd 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -455,10 +455,12 @@ void pci_restore_dpc_state(struct pci_dev *dev);
> > void pci_dpc_init(struct pci_dev *pdev);
> > void dpc_process_error(struct pci_dev *pdev);
> > pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
> > +bool pci_wait_port_outdpc(struct pci_dev *pdev);
> > #else
> > static inline void pci_save_dpc_state(struct pci_dev *dev) {}
> > static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
> > static inline void pci_dpc_init(struct pci_dev *pdev) {}
> > +static inline bool pci_wait_port_outdpc(struct pci_dev *pdev) { return false; }
> > #endif
> >
> > #ifdef CONFIG_PCI_ATS
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index daa9a4153776..2e0e091ce923 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -71,6 +71,33 @@ void pci_restore_dpc_state(struct pci_dev *dev)
> > pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
> > }
> >
> > +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;
> > +}
> > +
> > static int dpc_wait_rp_inactive(struct pci_dev *pdev)
> > {
> > unsigned long timeout = jiffies + HZ;
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>