[PATCH v8 0/6] Fix DPC hotplug race and enhance error handling

[PATCH v8 0/6] Fix DPC hotplug race and enhance error handling

[Ethan Zhao]

Hi,folks,

This simple patch set fixed some serious security issues found when DPC
error injection and NVMe SSD hotplug brute force test were doing -- race
condition between DPC handler and pciehp, AER interrupt handlers, caused
system hang and system with DPC feature couldn't recover to normal
working state as expected (NVMe instance lost, mount operation hang,
race PCIe access caused uncorrectable errors reported alternatively etc).

The fundamental premise is that when due to error conditions (NON-FATAL/
FATAL) when events are processed by both DPC handler and hotplug handling
of DLLSC/PDC both operating on the same device object ends up with crashes 
(from  Ashok).

Debug shows when port DPC feature was enabled and triggered by errors,
DLLSC/PDC/DPC interrupts will be sent to pciehp and DPC driver almost
at the same time, and no delay between them is required by specification.
so DPC driver and pciehp drivers may handle these interrupts cocurrently,
thus introduces the possibility of race condition, other details see every
commit description part.

With this patch set applied, stable 5.9-rc6 on ICS (Ice Lake SP platform,
see
https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))

could pass the PCIe Gen4 NVMe SSD brute force hotplug test with any time
interval between hot-remove and plug-in operation tens of times without
any errors occur and system works normal.

With this patch set applied, system with DPC feature could recover from
NON-FATAL and FATAL errors injection test and works as expected.

System works smoothly when errors happen while hotplug is doing, no
uncorrectable errors found.

Brute DPC error injection script:

for i in {0..100}
do
        setpci -s 64:02.0 0x196.w=000a
        setpci -s 65:00.0 0x04.w=0544
        mount /dev/nvme0n1p1 /root/nvme
        sleep 1
done

This patch set could be applied to stable 5.9-rc6/rc7/rc8 directly.

Help to review and test.

v2: changed according to review by Andy Shevchenko.
v3: changed patch 4/5 to simpler coding.
v4: move function pci_wait_port_outdpc() to DPC driver and its
   declaration to pci.h. (tip from Christoph Hellwig <hch@infradead.org>).
v5: fix building issue reported by lkp@intel.com with some config.
v6: move patch[3/5] as the first patch according to Lukas's suggestion.
    and rewrite the comment part of patch[3/5].
v7: change the patch[4/5], based on Bjorn's code and truth table.
    change the patch[5/5] about the debug output information.
v8: according Bjorn's suggestion, put the pci_dev_set_io_state()
    simplification but no function code in one patch.(almost copy of
    Bjorn's code and truth table, understood).
    patch 5/6 re-based the function change code of pci_dev_set_io_state().
    per Ashok's request, add more description to this cover-letter part.


Thanks,
Ethan


Ethan Zhao (6):
  PCI/ERR: get device before call device driver to avoid NULL pointer
    dereference
  PCI/DPC: define a function to check and wait till port finish DPC
    handling
  PCI: pciehp: check and wait port status out of DPC before handling
    DLLSC and PDC
  PCI/ERR: simplify function pci_dev_set_io_state() with if
  PCI/ERR: only return true when dev io state is really changed
  PCI/ERR: don't mix io state not changed and no driver together

 drivers/pci/hotplug/pciehp_hpc.c |  4 ++-
 drivers/pci/pci.h                | 55 +++++++++++++-------------------
 drivers/pci/pcie/dpc.c           | 27 ++++++++++++++++
 drivers/pci/pcie/err.c           | 18 +++++++++--
 4 files changed, 69 insertions(+), 35 deletions(-)


base-commit: 549738f15da0e5a00275977623be199fbbf7df50
-- 
2.18.4

[PATCH v8 1/6] PCI/ERR: get device before call device driver to avoid NULL pointer dereference

[Ethan Zhao]

During DPC error injection test we found there is race condition between
pciehp and DPC driver, NULL pointer dereference caused panic as following

 # setpci -s 64:02.0 0x196.w=000a
  // 64:02.0 is rootport has DPC capability
 # setpci -s 65:00.0 0x04.w=0544
  // 65:00.0 is NVMe SSD populated in above port
 # mount /dev/nvme0n1p1 nvme

 (tested on stable 5.8 & ICS(Ice Lake SP platform, see
 https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))

 BUG: kernel NULL pointer dereference, address: 0000000000000050
 ...
 CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0-0.0.7.el8.x86_64+ #1
 RIP: 0010:report_error_detected.cold.4+0x7d/0xe6
 Code: b6 d0 e8 e8 fe 11 00 e8 16 c5 fb ff be 06 00 00 00 48 89 df e8 d3 65 ff
 ff b8 06 00 00 00 e9 75 fc ff ff 48 8b 43 68 45 31 c9 <48> 8b 50 50 48 83 3a 00
 41 0f 94 c1 45 31 c0 48 85 d2 41 0f 94 c0
 RSP: 0018:ff8e06cf8762fda8 EFLAGS: 00010246
 RAX: 0000000000000000 RBX: ff4e3eaacf42a000 RCX: ff4e3eb31f223c01
 RDX: ff4e3eaacf42a140 RSI: ff4e3eb31f223c00 RDI: ff4e3eaacf42a138
 RBP: ff8e06cf8762fdd0 R08: 00000000000000bf R09: 0000000000000000
 R10: 000000eb8ebeab53 R11: ffffffff93453258 R12: 0000000000000002
 R13: ff4e3eaacf42a130 R14: ff8e06cf8762fe2c R15: ff4e3eab44733828
 FS:  0000000000000000(0000) GS:ff4e3eab1fd00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000050 CR3: 0000000f8f80a004 CR4: 0000000000761ee0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 PKRU: 55555554
 Call Trace:
 ? report_normal_detected+0x20/0x20
 report_frozen_detected+0x16/0x20
 pci_walk_bus+0x75/0x90
 ? dpc_irq+0x90/0x90
 pcie_do_recovery+0x157/0x201
 ? irq_finalize_oneshot.part.47+0xe0/0xe0
 dpc_handler+0x29/0x40
 irq_thread_fn+0x24/0x60
 ...

Debug shows when port DPC feature was enabled and triggered by errors,
DLLSC/PDC/DPC interrupts will be sent to pciehp and DPC driver almost
at the same time, and no delay between them is required by specification.
so DPC driver and pciehp drivers may handle these interrupts cocurrently.

While DPC driver is doing pci_walk_bus() and calling device driver's 
callback without pci_dev_get() to increase device reference count, the
device and its driver instance are likely being freed by
pci_stop_and_removed_bus_device()
-> pci_dev_put().

So does pci_dev_get() before using the device instance to avoid NULL
pointer dereference.

Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com>
Tested-by: Wen Jin <wen.jin@intel.com>
Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
---
Changes:
 v2: revise doc according to Andy's suggestion.
 v3: no change.
 v4: no change.
 v5: no change.
 v6: moved to [1/5] from [3/5] and revised comment according to Lukas'
     suggestion.
 v7: no change.
 v8: no change.

 drivers/pci/pcie/err.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index c543f419d8f9..e35c4480c86b 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -52,6 +52,8 @@ static int report_error_detected(struct pci_dev *dev,
 	pci_ers_result_t vote;
 	const struct pci_error_handlers *err_handler;
 
+	if (!pci_dev_get(dev))
+		return 0;
 	device_lock(&dev->dev);
 	if (!pci_dev_set_io_state(dev, state) ||
 		!dev->driver ||
@@ -76,6 +78,7 @@ static int report_error_detected(struct pci_dev *dev,
 	pci_uevent_ers(dev, vote);
 	*result = merge_result(*result, vote);
 	device_unlock(&dev->dev);
+	pci_dev_put(dev);
 	return 0;
 }
 
@@ -94,6 +97,8 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data)
 	pci_ers_result_t vote, *result = data;
 	const struct pci_error_handlers *err_handler;
 
+	if (!pci_dev_get(dev))
+		return 0;
 	device_lock(&dev->dev);
 	if (!dev->driver ||
 		!dev->driver->err_handler ||
@@ -105,6 +110,7 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data)
 	*result = merge_result(*result, vote);
 out:
 	device_unlock(&dev->dev);
+	pci_dev_put(dev);
 	return 0;
 }
 
@@ -113,6 +119,8 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
 	pci_ers_result_t vote, *result = data;
 	const struct pci_error_handlers *err_handler;
 
+	if (!pci_dev_get(dev))
+		return 0;
 	device_lock(&dev->dev);
 	if (!dev->driver ||
 		!dev->driver->err_handler ||
@@ -124,6 +132,7 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
 	*result = merge_result(*result, vote);
 out:
 	device_unlock(&dev->dev);
+	pci_dev_put(dev);
 	return 0;
 }
 
@@ -131,6 +140,8 @@ static int report_resume(struct pci_dev *dev, void *data)
 {
 	const struct pci_error_handlers *err_handler;
 
+	if (!pci_dev_get(dev))
+		return 0;
 	device_lock(&dev->dev);
 	if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
 		!dev->driver ||
@@ -143,6 +154,7 @@ static int report_resume(struct pci_dev *dev, void *data)
 out:
 	pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
 	device_unlock(&dev->dev);
+	pci_dev_put(dev);
 	return 0;
 }
 
-- 
2.18.4

[PATCH v8 2/6] PCI/DPC: define a function to check and wait till port finish DPC handling

[Ethan Zhao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=a, Size: 3116 bytes --]

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 <haifeng.zhao@intel.com>
Tested-by: Wen Jin <wen.jin@intel.com>
Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
---
changes:
 v2：align ICS code name to public doc.
 v3: no change.
 v4: response to Christoph's (Christoph Hellwig <hch@infradead.org>)
     tip, move pci_wait_port_outdpc() to DPC driver and its declaration
     to pci.h.
 v5: fix building issue reported by lkp@intel.com 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

[PATCH v8 3/6] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC

[Ethan Zhao]

When root port has DPC capability and it is enabled, then triggered by
errors, DPC DLLSC and PDC etc interrupts will be sent to DPC driver, 
pciehp drivers almost at the same time.
Thus will cause following messed and confused errors handling/recovery/
removal/plugin procedure.

1. Port and device are in error recovery resetting initiated by DPC
   hardware, pciehp driver treats them as device is doing hot-remove or 
   hot-plugin the same time.

2. While DPC handler calling device driver->err_handler callback(
   error_detected/resume etc), but the slot may be powered off by

   pciehp
   -> remove_board()
      -> pciehp_power_off_slot().

3. While DPC handler -> pci_do_recovery is doing different action to
   detect error and recover based on device->error_state, pciehp driver
   could change it on the fly by:

   pciehp_unconfigure_device()
   ->pci_walk_bus()
     -> pci_dev_set_disconnected()

4. While DPC handler is calling device driver err_handler callback to 
   detect error and recover, pciehp driver could is doing device unbind
   and release its driver.

   ...

While NON-FATAL/FATAL errors happen while hotplug is(is not)doing, result
is not determinate.

So we need some kind of synchronization between pciehp DLLSC/PDC handling
and DPC driver error recover handling.  we need a determinate result
of DPC error containment, link is recovered, link isn't recovered, device
is still there, device is removed, then do pciehp hot-remove and
hot-plugin procudure, don't mix them together.

Per our test on ICS platform, DPC error containment and software handler
will take 10ms up to 50ms till clean the DPC triggered status. it is quick
enough for pciehp compared with its 1000ms waiting to ignore DLLSC/PDC
after doing power off.

With this patch, the handling flow of DPC containment and hotplug is
partly ordered and serialized, let hardware DPC do the controller reset
etc recovery action first, then DPC driver handling the call-back from
device drivers, clear the DPC status, at the end, pciehp handle the DLLSC
and PDC etc.

After tens of PCIe Gen4 NVMe SSD brute force hot-remove and hot-plugin with
any time internval between the two actions, also stressed with the DPC
injection test. system recovered to normal working state from
NON-FATAL/FATAL errors as expected. hotplug works well without any random
undeterminate errors or malfunction.

Brute DPC error injection script:

for i in {0..100}
do
        setpci -s 64:02.0 0x196.w=000a
        setpci -s 65:00.0 0x04.w=0544
        mount /dev/nvme0n1p1 /root/nvme
        sleep 1
done

Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com>
Tested-by: Wen Jin <wen.jin@intel.com>
Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
---
Changes:
 v2: revise doc according to Andy's suggestion.
 v3: no change.
 v4: no change.
 v5: no change.
 v6: moved to [3/5] from [2/5] and re-wrote description.
 v7: no change.
 v8: no change.

 drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 53433b37e181..6f271160f18d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -710,8 +710,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 	down_read(&ctrl->reset_lock);
 	if (events & DISABLE_SLOT)
 		pciehp_handle_disable_request(ctrl);
-	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
+	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) {
+		pci_wait_port_outdpc(pdev);
 		pciehp_handle_presence_or_link_change(ctrl, events);
+	}
 	up_read(&ctrl->reset_lock);
 
 	ret = IRQ_HANDLED;
-- 
2.18.4

[PATCH v8 4/6] PCI/ERR: simplify function pci_dev_set_io_state() with if

[Ethan Zhao]

No function change.

Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com>
---
Changes:
 v8: based on Bjorn's code and truth table, simplify the logic of
function pci_dev_set_io_state(), no function change.

 drivers/pci/pci.h | 54 ++++++++++++++++++++---------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 455b32187abd..bceb3f108744 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -359,39 +359,31 @@ struct pci_sriov {
 static inline bool pci_dev_set_io_state(struct pci_dev *dev,
 					pci_channel_state_t new)
 {
-	bool changed = false;
-
 	device_lock_assert(&dev->dev);
-	switch (new) {
-	case pci_channel_io_perm_failure:
-		switch (dev->error_state) {
-		case pci_channel_io_frozen:
-		case pci_channel_io_normal:
-		case pci_channel_io_perm_failure:
-			changed = true;
-			break;
-		}
-		break;
-	case pci_channel_io_frozen:
-		switch (dev->error_state) {
-		case pci_channel_io_frozen:
-		case pci_channel_io_normal:
-			changed = true;
-			break;
-		}
-		break;
-	case pci_channel_io_normal:
-		switch (dev->error_state) {
-		case pci_channel_io_frozen:
-		case pci_channel_io_normal:
-			changed = true;
-			break;
-		}
-		break;
+/*
+ *                     Truth table:
+ *                     requested new state
+ *     current          ------------------------------------------
+ *     state            normal         frozen         perm_failure
+ *     ------------  +  -------------  -------------  ------------
+ *     normal        |  normal         frozen         perm_failure
+ *     frozen        |  normal         frozen         perm_failure
+ *     perm_failure  |  perm_failure*  perm_failure*  perm_failure
+ */
+
+	/* Can always put a device in perm_failure state */
+	if (new == pci_channel_io_perm_failure) {
+		dev->error_state = pci_channel_io_perm_failure;
+		return true;
 	}
-	if (changed)
-		dev->error_state = new;
-	return changed;
+
+	/* If already in perm_failure, can't set to normal or frozen */
+	if (dev->error_state == pci_channel_io_perm_failure)
+		return false;
+
+	 /* Can always change normal to frozen or vice versa */
+	dev->error_state = new;
+	return true;
 }
 
 static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
-- 
2.18.4

[PATCH v8 5/6] PCI/ERR: only return true when dev io state is really changed

[Ethan Zhao]

When uncorrectable error happens, AER driver and DPC driver interrupt
handlers likely call

   pcie_do_recovery()
   ->pci_walk_bus()
     ->report_frozen_detected()

with pci_channel_io_frozen the same time.
   If pci_dev_set_io_state() return true even if the original state is
pci_channel_io_frozen, that will cause AER or DPC handler re-enter
the error detecting and recovery procedure one after another.
   The result is the recovery flow mixed between AER and DPC.
So change the pci_dev_set_io_state() function to only return true
when dev->error_state is really changed.

Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com>
---
Changnes:
 v2: revise description and code according to suggestion from Andy.
 v3: change code to simpler.
 v4: no change.
 v5: no change.
 v6: no change.
 v7: changed based on Bjorn's code and truth table.
 v8: according to Bjorn's suggestion, rebase on another simplification
     patch.   

 drivers/pci/pci.h | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index bceb3f108744..a11e0f9d9bdf 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -371,17 +371,14 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev,
  *     perm_failure  |  perm_failure*  perm_failure*  perm_failure
  */
 
-	/* Can always put a device in perm_failure state */
-	if (new == pci_channel_io_perm_failure) {
-		dev->error_state = pci_channel_io_perm_failure;
-		return true;
-	}
-
-	/* If already in perm_failure, can't set to normal or frozen */
+	/* If already in perm_failure, can't change it's state */
 	if (dev->error_state == pci_channel_io_perm_failure)
 		return false;
+	/* not change at all */
+	else if (dev->error_state == new)
+		return false;
 
-	 /* Can always change normal to frozen or vice versa */
+	 /* Can always change from normal/frozen to other different state */
 	dev->error_state = new;
 	return true;
 }
-- 
2.18.4

[PATCH v8 6/6] PCI/ERR: don't mix io state not changed and no driver together

[Ethan Zhao]

When we see 'can't recover (no error_detected callback)' on console,
Maybe the reason is io state is not changed by calling
pci_dev_set_io_state(), that is confused. fix it.

Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com>
Tested-by: Wen Jin <wen.jin@intel.com>
Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
---
Chagnes:
 v2: no change.
 v3: no change.
 v4: no change.
 v5: no change.
 v6: no change.
 v7: change debug output information.
 v8: no change.

 drivers/pci/pcie/err.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index e35c4480c86b..2ca2723f3b34 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -55,8 +55,10 @@ static int report_error_detected(struct pci_dev *dev,
 	if (!pci_dev_get(dev))
 		return 0;
 	device_lock(&dev->dev);
-	if (!pci_dev_set_io_state(dev, state) ||
-		!dev->driver ||
+	if (!pci_dev_set_io_state(dev, state)) {
+		pci_dbg(dev, "Device was in that state or not allowed setting.\n");
+		vote = PCI_ERS_RESULT_NONE;
+	} else if (!dev->driver ||
 		!dev->driver->err_handler ||
 		!dev->driver->err_handler->error_detected) {
 		/*
-- 
2.18.4

Re: [PATCH v8 1/6] PCI/ERR: get device before call device driver to avoid NULL pointer dereference

[Kuppuswamy, Sathyanarayanan]

On 10/7/20 4:31 AM, Ethan Zhao wrote:
> During DPC error injection test we found there is race condition between
> pciehp and DPC driver, NULL pointer dereference caused panic as following
>
>   # setpci -s 64:02.0 0x196.w=000a
>    // 64:02.0 is rootport has DPC capability
>   # setpci -s 65:00.0 0x04.w=0544
>    // 65:00.0 is NVMe SSD populated in above port
>   # mount /dev/nvme0n1p1 nvme
>
>   (tested on stable 5.8 & ICS(Ice Lake SP platform, see
>   https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))
>
>   BUG: kernel NULL pointer dereference, address: 0000000000000050
>   ...
>   CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0-0.0.7.el8.x86_64+ #1
>   RIP: 0010:report_error_detected.cold.4+0x7d/0xe6
>   Code: b6 d0 e8 e8 fe 11 00 e8 16 c5 fb ff be 06 00 00 00 48 89 df e8 d3 65 ff
>   ff b8 06 00 00 00 e9 75 fc ff ff 48 8b 43 68 45 31 c9 <48> 8b 50 50 48 83 3a 00
>   41 0f 94 c1 45 31 c0 48 85 d2 41 0f 94 c0
>   RSP: 0018:ff8e06cf8762fda8 EFLAGS: 00010246
>   RAX: 0000000000000000 RBX: ff4e3eaacf42a000 RCX: ff4e3eb31f223c01
>   RDX: ff4e3eaacf42a140 RSI: ff4e3eb31f223c00 RDI: ff4e3eaacf42a138
>   RBP: ff8e06cf8762fdd0 R08: 00000000000000bf R09: 0000000000000000
>   R10: 000000eb8ebeab53 R11: ffffffff93453258 R12: 0000000000000002
>   R13: ff4e3eaacf42a130 R14: ff8e06cf8762fe2c R15: ff4e3eab44733828
>   FS:  0000000000000000(0000) GS:ff4e3eab1fd00000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 0000000000000050 CR3: 0000000f8f80a004 CR4: 0000000000761ee0
>   DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>   DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>   PKRU: 55555554
>   Call Trace:
>   ? report_normal_detected+0x20/0x20
>   report_frozen_detected+0x16/0x20
>   pci_walk_bus+0x75/0x90
>   ? dpc_irq+0x90/0x90
>   pcie_do_recovery+0x157/0x201
>   ? irq_finalize_oneshot.part.47+0xe0/0xe0
>   dpc_handler+0x29/0x40
>   irq_thread_fn+0x24/0x60
>   ...
>
> Debug shows when port DPC feature was enabled and triggered by errors,
> DLLSC/PDC/DPC interrupts will be sent to pciehp and DPC driver almost
> at the same time, and no delay between them is required by specification.
> so DPC driver and pciehp drivers may handle these interrupts cocurrently.
>
> While DPC driver is doing pci_walk_bus() and calling device driver's
> callback without pci_dev_get() to increase device reference count, the
> device and its driver instance are likely being freed by
> pci_stop_and_removed_bus_device()
> -> pci_dev_put().
>
> So does pci_dev_get() before using the device instance to avoid NULL
> pointer dereference.
Won't it be better if you get this in pcie_do_recovery()?
>
> Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com>
> Tested-by: Wen Jin <wen.jin@intel.com>
> Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
> ---
> Changes:
>   v2: revise doc according to Andy's suggestion.
>   v3: no change.
>   v4: no change.
>   v5: no change.
>   v6: moved to [1/5] from [3/5] and revised comment according to Lukas'
>       suggestion.
>   v7: no change.
>   v8: no change.
>
>   drivers/pci/pcie/err.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index c543f419d8f9..e35c4480c86b 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -52,6 +52,8 @@ static int report_error_detected(struct pci_dev *dev,
>   	pci_ers_result_t vote;
>   	const struct pci_error_handlers *err_handler;
>   
> +	if (!pci_dev_get(dev))
> +		return 0;
>   	device_lock(&dev->dev);
>   	if (!pci_dev_set_io_state(dev, state) ||
>   		!dev->driver ||
> @@ -76,6 +78,7 @@ static int report_error_detected(struct pci_dev *dev,
>   	pci_uevent_ers(dev, vote);
>   	*result = merge_result(*result, vote);
>   	device_unlock(&dev->dev);
> +	pci_dev_put(dev);
>   	return 0;
>   }
>   
> @@ -94,6 +97,8 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data)
>   	pci_ers_result_t vote, *result = data;
>   	const struct pci_error_handlers *err_handler;
>   
> +	if (!pci_dev_get(dev))
> +		return 0;
>   	device_lock(&dev->dev);
>   	if (!dev->driver ||
>   		!dev->driver->err_handler ||
> @@ -105,6 +110,7 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data)
>   	*result = merge_result(*result, vote);
>   out:
>   	device_unlock(&dev->dev);
> +	pci_dev_put(dev);
>   	return 0;
>   }
>   
> @@ -113,6 +119,8 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
>   	pci_ers_result_t vote, *result = data;
>   	const struct pci_error_handlers *err_handler;
>   
> +	if (!pci_dev_get(dev))
> +		return 0;
>   	device_lock(&dev->dev);
>   	if (!dev->driver ||
>   		!dev->driver->err_handler ||
> @@ -124,6 +132,7 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
>   	*result = merge_result(*result, vote);
>   out:
>   	device_unlock(&dev->dev);
> +	pci_dev_put(dev);
>   	return 0;
>   }
>   
> @@ -131,6 +140,8 @@ static int report_resume(struct pci_dev *dev, void *data)
>   {
>   	const struct pci_error_handlers *err_handler;
>   
> +	if (!pci_dev_get(dev))
> +		return 0;
>   	device_lock(&dev->dev);
>   	if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
>   		!dev->driver ||
> @@ -143,6 +154,7 @@ static int report_resume(struct pci_dev *dev, void *data)
>   out:
>   	pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
>   	device_unlock(&dev->dev);
> +	pci_dev_put(dev);
>   	return 0;
>   }
>   

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v8 2/6] PCI/DPC: define a function to check and wait till port finish DPC handling

[Kuppuswamy, Sathyanarayanan]

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 <haifeng.zhao@intel.com>
> Tested-by: Wen Jin <wen.jin@intel.com>
> Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
> ---
> changes:
>   v2：align ICS code name to public doc.
>   v3: no change.
>   v4: response to Christoph's (Christoph Hellwig <hch@infradead.org>)
>       tip, move pci_wait_port_outdpc() to DPC driver and its declaration
>       to pci.h.
>   v5: fix building issue reported by lkp@intel.com 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

Re: [PATCH v8 1/6] PCI/ERR: get device before call device driver to avoid NULL pointer dereference

[Ethan Zhao]

On Thu, Oct 8, 2020 at 1:24 AM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@intel.com> wrote:
>
>
> On 10/7/20 4:31 AM, Ethan Zhao wrote:
> > During DPC error injection test we found there is race condition between
> > pciehp and DPC driver, NULL pointer dereference caused panic as following
> >
> >   # setpci -s 64:02.0 0x196.w=000a
> >    // 64:02.0 is rootport has DPC capability
> >   # setpci -s 65:00.0 0x04.w=0544
> >    // 65:00.0 is NVMe SSD populated in above port
> >   # mount /dev/nvme0n1p1 nvme
> >
> >   (tested on stable 5.8 & ICS(Ice Lake SP platform, see
> >   https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))
> >
> >   BUG: kernel NULL pointer dereference, address: 0000000000000050
> >   ...
> >   CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0-0.0.7.el8.x86_64+ #1
> >   RIP: 0010:report_error_detected.cold.4+0x7d/0xe6
> >   Code: b6 d0 e8 e8 fe 11 00 e8 16 c5 fb ff be 06 00 00 00 48 89 df e8 d3 65 ff
> >   ff b8 06 00 00 00 e9 75 fc ff ff 48 8b 43 68 45 31 c9 <48> 8b 50 50 48 83 3a 00
> >   41 0f 94 c1 45 31 c0 48 85 d2 41 0f 94 c0
> >   RSP: 0018:ff8e06cf8762fda8 EFLAGS: 00010246
> >   RAX: 0000000000000000 RBX: ff4e3eaacf42a000 RCX: ff4e3eb31f223c01
> >   RDX: ff4e3eaacf42a140 RSI: ff4e3eb31f223c00 RDI: ff4e3eaacf42a138
> >   RBP: ff8e06cf8762fdd0 R08: 00000000000000bf R09: 0000000000000000
> >   R10: 000000eb8ebeab53 R11: ffffffff93453258 R12: 0000000000000002
> >   R13: ff4e3eaacf42a130 R14: ff8e06cf8762fe2c R15: ff4e3eab44733828
> >   FS:  0000000000000000(0000) GS:ff4e3eab1fd00000(0000) knlGS:0000000000000000
> >   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >   CR2: 0000000000000050 CR3: 0000000f8f80a004 CR4: 0000000000761ee0
> >   DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >   DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >   PKRU: 55555554
> >   Call Trace:
> >   ? report_normal_detected+0x20/0x20
> >   report_frozen_detected+0x16/0x20
> >   pci_walk_bus+0x75/0x90
> >   ? dpc_irq+0x90/0x90
> >   pcie_do_recovery+0x157/0x201
> >   ? irq_finalize_oneshot.part.47+0xe0/0xe0
> >   dpc_handler+0x29/0x40
> >   irq_thread_fn+0x24/0x60
> >   ...
> >
> > Debug shows when port DPC feature was enabled and triggered by errors,
> > DLLSC/PDC/DPC interrupts will be sent to pciehp and DPC driver almost
> > at the same time, and no delay between them is required by specification.
> > so DPC driver and pciehp drivers may handle these interrupts cocurrently.
> >
> > While DPC driver is doing pci_walk_bus() and calling device driver's
> > callback without pci_dev_get() to increase device reference count, the
> > device and its driver instance are likely being freed by
> > pci_stop_and_removed_bus_device()
> > -> pci_dev_put().
> >
> > So does pci_dev_get() before using the device instance to avoid NULL
> > pointer dereference.
> Won't it be better if you get this in pcie_do_recovery()?

Don't think so, just like lock, we should keep the scope with lock
protected as small as possible.
Locking a big area unnecessarily isn't acceptable.

Thanks,
Ethan
> >
> > Signed-off-by: Ethan Zhao <haifeng.zhao@intel.com>
> > Tested-by: Wen Jin <wen.jin@intel.com>
> > Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
> > ---
> > Changes:
> >   v2: revise doc according to Andy's suggestion.
> >   v3: no change.
> >   v4: no change.
> >   v5: no change.
> >   v6: moved to [1/5] from [3/5] and revised comment according to Lukas'
> >       suggestion.
> >   v7: no change.
> >   v8: no change.
> >
> >   drivers/pci/pcie/err.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index c543f419d8f9..e35c4480c86b 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -52,6 +52,8 @@ static int report_error_detected(struct pci_dev *dev,
> >       pci_ers_result_t vote;
> >       const struct pci_error_handlers *err_handler;
> >
> > +     if (!pci_dev_get(dev))
> > +             return 0;
> >       device_lock(&dev->dev);
> >       if (!pci_dev_set_io_state(dev, state) ||
> >               !dev->driver ||
> > @@ -76,6 +78,7 @@ static int report_error_detected(struct pci_dev *dev,
> >       pci_uevent_ers(dev, vote);
> >       *result = merge_result(*result, vote);
> >       device_unlock(&dev->dev);
> > +     pci_dev_put(dev);
> >       return 0;
> >   }
> >
> > @@ -94,6 +97,8 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data)
> >       pci_ers_result_t vote, *result = data;
> >       const struct pci_error_handlers *err_handler;
> >
> > +     if (!pci_dev_get(dev))
> > +             return 0;
> >       device_lock(&dev->dev);
> >       if (!dev->driver ||
> >               !dev->driver->err_handler ||
> > @@ -105,6 +110,7 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data)
> >       *result = merge_result(*result, vote);
> >   out:
> >       device_unlock(&dev->dev);
> > +     pci_dev_put(dev);
> >       return 0;
> >   }
> >
> > @@ -113,6 +119,8 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
> >       pci_ers_result_t vote, *result = data;
> >       const struct pci_error_handlers *err_handler;
> >
> > +     if (!pci_dev_get(dev))
> > +             return 0;
> >       device_lock(&dev->dev);
> >       if (!dev->driver ||
> >               !dev->driver->err_handler ||
> > @@ -124,6 +132,7 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
> >       *result = merge_result(*result, vote);
> >   out:
> >       device_unlock(&dev->dev);
> > +     pci_dev_put(dev);
> >       return 0;
> >   }
> >
> > @@ -131,6 +140,8 @@ static int report_resume(struct pci_dev *dev, void *data)
> >   {
> >       const struct pci_error_handlers *err_handler;
> >
> > +     if (!pci_dev_get(dev))
> > +             return 0;
> >       device_lock(&dev->dev);
> >       if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
> >               !dev->driver ||
> > @@ -143,6 +154,7 @@ static int report_resume(struct pci_dev *dev, void *data)
> >   out:
> >       pci_uevent_ers(dev, PCI_ERS_RESULT_RECOVERED);
> >       device_unlock(&dev->dev);
> > +     pci_dev_put(dev);
> >       return 0;
> >   }
> >
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>

Re: [PATCH v8 2/6] PCI/DPC: define a function to check and wait till port finish DPC handling

[Ethan Zhao]

On Thu, Oct 8, 2020 at 2:16 AM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@intel.com> 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 <haifeng.zhao@intel.com>
> > Tested-by: Wen Jin <wen.jin@intel.com>
> > Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
> > ---
> > changes:
> >   v2：align ICS code name to public doc.
> >   v3: no change.
> >   v4: response to Christoph's (Christoph Hellwig <hch@infradead.org>)
> >       tip, move pci_wait_port_outdpc() to DPC driver and its declaration
> >       to pci.h.
> >   v5: fix building issue reported by lkp@intel.com 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
>

Re: [PATCH v8 2/6] PCI/DPC: define a function to check and wait till port finish DPC handling

[Ethan Zhao]

On Thu, Oct 8, 2020 at 2:16 AM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@intel.com> 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 <haifeng.zhao@intel.com>
> > Tested-by: Wen Jin <wen.jin@intel.com>
> > Tested-by: Shanshan Zhang <ShanshanX.Zhang@intel.com>
> > ---
> > changes:
> >   v2：align ICS code name to public doc.
> >   v3: no change.
> >   v4: response to Christoph's (Christoph Hellwig <hch@infradead.org>)
> >       tip, move pci_wait_port_outdpc() to DPC driver and its declaration
> >       to pci.h.
> >   v5: fix building issue reported by lkp@intel.com 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
>