* [PATCH 1/5 V4] PCI: define a function to check and wait till port finish DPC handling
2020-09-27 8:27 [PATCH 0/5 V4] Fix DPC hotplug race and enhance error handling Ethan Zhao
@ 2020-09-27 8:27 ` Ethan Zhao
2020-09-27 9:05 ` Andy Shevchenko
2020-09-27 11:08 ` kernel test robot
2020-09-27 8:27 ` [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC Ethan Zhao
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Ethan Zhao @ 2020-09-27 8:27 UTC (permalink / raw)
To: bhelgaas, oohall, ruscur, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, pei.p.jia, ashok.raj,
sathyanarayanan.kuppuswamy, hch, joe, Ethan Zhao
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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Christoph Hellwig <hch@infradead.org>
---
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.
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..8fdb0d823d5a 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) {}
+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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5 V4] PCI: define a function to check and wait till port finish DPC handling
2020-09-27 8:27 ` [PATCH 1/5 V4] PCI: define a function to check and wait till port finish DPC handling Ethan Zhao
@ 2020-09-27 9:05 ` Andy Shevchenko
2020-09-27 11:08 ` kernel test robot
1 sibling, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2020-09-27 9:05 UTC (permalink / raw)
To: Ethan Zhao, Dave Hansen
Cc: Bjorn Helgaas, Oliver, ruscur, Lukas Wunner, Andy Shevchenko,
Stuart Hayes, Alexandru Gagniuc, Mika Westerberg, linux-pci,
Linux Kernel Mailing List, pei.p.jia, ashok.raj,
Sathyanarayanan Kuppuswamy, Christoph Hellwig, Joe Perches
On Sun, Sep 27, 2020 at 11:33 AM Ethan Zhao <haifeng.zhao@intel.com> 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
> 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>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
I haven't given you this tag. Where did you get it from?
(Dave, that's the case where we need to push the [internal review] process)
> Reviewed-by: Christoph Hellwig <hch@infradead.org>
> ---
> 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.
>
> 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..8fdb0d823d5a 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) {}
> +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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5 V4] PCI: define a function to check and wait till port finish DPC handling
2020-09-27 8:27 ` [PATCH 1/5 V4] PCI: define a function to check and wait till port finish DPC handling Ethan Zhao
2020-09-27 9:05 ` Andy Shevchenko
@ 2020-09-27 11:08 ` kernel test robot
1 sibling, 0 replies; 16+ messages in thread
From: kernel test robot @ 2020-09-27 11:08 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 8401 bytes --]
Hi Ethan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on next-20200925]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Ethan-Zhao/Fix-DPC-hotplug-race-and-enhance-error-handling/20200927-163058
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-a011-20200927 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/afffc9849677c90368ff1edc2de779db5511aeb0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ethan-Zhao/Fix-DPC-hotplug-race-and-enhance-error-handling/20200927-163058
git checkout afffc9849677c90368ff1edc2de779db5511aeb0
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: drivers/pci/bus.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/probe.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/host-bridge.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/remove.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pci.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pci-driver.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/search.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pci-sysfs.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/rom.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/setup-res.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/vpd.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/setup-bus.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/vc.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/setup-irq.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pcie/portdrv_core.o: in function `pci_wait_port_outdpc':
drivers/pci/pcie/../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pcie/portdrv_pci.o: in function `pci_wait_port_outdpc':
drivers/pci/pcie/../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pcie/err.o: in function `pci_wait_port_outdpc':
drivers/pci/pcie/../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pcie/aspm.o: in function `pci_wait_port_outdpc':
drivers/pci/pcie/../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pcie/pme.o: in function `pci_wait_port_outdpc':
drivers/pci/pcie/../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pcie/ptm.o: in function `pci_wait_port_outdpc':
drivers/pci/pcie/../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pcie/bw_notification.o: in function `pci_wait_port_outdpc':
drivers/pci/pcie/../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/proc.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/slot.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pci-acpi.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/quirks.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/msi.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/ats.o: in function `pci_wait_port_outdpc':
>> drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/pci-label.o: in function `pci_wait_port_outdpc':
drivers/pci/pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/controller/dwc/pcie-designware.o: in function `pci_wait_port_outdpc':
drivers/pci/controller/dwc/../../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
ld: drivers/pci/controller/dwc/pcie-designware-host.o: in function `pci_wait_port_outdpc':
drivers/pci/controller/dwc/../../pci.h:463: multiple definition of `pci_wait_port_outdpc'; drivers/pci/access.o:drivers/pci/pci.h:463: first defined here
vim +463 drivers/pci/pci.h
451
452 #ifdef CONFIG_PCIE_DPC
453 void pci_save_dpc_state(struct pci_dev *dev);
454 void pci_restore_dpc_state(struct pci_dev *dev);
455 void pci_dpc_init(struct pci_dev *pdev);
456 void dpc_process_error(struct pci_dev *pdev);
457 pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
458 bool pci_wait_port_outdpc(struct pci_dev *pdev);
459 #else
460 static inline void pci_save_dpc_state(struct pci_dev *dev) {}
461 static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
462 static inline void pci_dpc_init(struct pci_dev *pdev) {}
> 463 inline bool pci_wait_port_outdpc(struct pci_dev *pdev) { return false; }
464 #endif
465
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31004 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC
2020-09-27 8:27 [PATCH 0/5 V4] Fix DPC hotplug race and enhance error handling Ethan Zhao
2020-09-27 8:27 ` [PATCH 1/5 V4] PCI: define a function to check and wait till port finish DPC handling Ethan Zhao
@ 2020-09-27 8:27 ` Ethan Zhao
2020-09-27 9:10 ` Andy Shevchenko
[not found] ` <35e7a493-a6f0-ac16-eb27-8ee5e77312ab@intel.com>
2020-09-27 8:27 ` [PATCH 3/5 V4] PCI/ERR: get device before call device driver to avoid NULL pointer reference Ethan Zhao
` (2 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Ethan Zhao @ 2020-09-27 8:27 UTC (permalink / raw)
To: bhelgaas, oohall, ruscur, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, pei.p.jia, ashok.raj,
sathyanarayanan.kuppuswamy, hch, joe, Ethan Zhao
When root port has DPC capability and it is enabled, then triggered by
errors, DPC DLLSC and PDC interrupts will be sent to DPC driver, pciehp
driver at the same time.
That will cause following result:
1. Link and device are recovered by hardware DPC and software DPC driver,
device
isn't removed, but the pciehp might treat it as device was hot removed.
2. Race condition happens bettween pciehp_unconfigure_device() called by
pciehp_ist() in pciehp driver and pci_do_recovery() called by
dpc_handler in DPC driver. no luck, there is no lock to protect
pci_stop_and_remove_bus_device()
against pci_walk_bus(), they hold different samphore and mutex,
pci_stop_and_remove_bus_device holds pci_rescan_remove_lock, and
pci_walk_bus() holds pci_bus_sem.
This race condition is not purely code analysis, it could be triggered by
following command series:
# setpci -s 64:02.0 0x196.w=000a // 64:02.0 rootport has DPC capability
# setpci -s 65:00.0 0x04.w=0544 // 65:00.0 NVMe SSD populated in port
# mount /dev/nvme0n1p1 nvme
One shot will cause system panic and NULL pointer reference happened.
(tested on stable 5.8 & ICS(Ice Lake SP platform, see
https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))
Buffer I/O error on dev nvme0n1p1, logical block 3328, async page read
BUG: kernel NULL pointer dereference, address: 0000000000000050
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0
Oops: 0000 [#1] SMP NOPTI
CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0 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) knl
GS: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
irq_thread+0xea/0x170
? irq_forced_thread_fn+0x80/0x80
? irq_thread_check_affinity+0xf0/0xf0
kthread+0x124/0x140
? kthread_park+0x90/0x90
ret_from_fork+0x1f/0x30
Modules linked in: nft_fib_inet.........
CR2: 0000000000000050
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.
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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Changes:
V2: revise doc according to Andy's suggestion.
V3: no change.
V4: 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC
2020-09-27 8:27 ` [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC Ethan Zhao
@ 2020-09-27 9:10 ` Andy Shevchenko
2020-09-28 2:54 ` Zhao, Haifeng
[not found] ` <MWHPR11MB1696B1DF37BE8F323E67FEE497350@MWHPR11MB1696.namprd11.prod.outlook.com>
[not found] ` <35e7a493-a6f0-ac16-eb27-8ee5e77312ab@intel.com>
1 sibling, 2 replies; 16+ messages in thread
From: Andy Shevchenko @ 2020-09-27 9:10 UTC (permalink / raw)
To: Ethan Zhao, Dave Hansen
Cc: Bjorn Helgaas, Oliver, ruscur, Lukas Wunner, Andy Shevchenko,
Stuart Hayes, Alexandru Gagniuc, Mika Westerberg, linux-pci,
Linux Kernel Mailing List, pei.p.jia, ashok.raj,
Sathyanarayanan Kuppuswamy, Christoph Hellwig, Joe Perches
On Sun, Sep 27, 2020 at 11:31 AM Ethan Zhao <haifeng.zhao@intel.com> wrote:
>
> When root port has DPC capability and it is enabled, then triggered by
> errors, DPC DLLSC and PDC interrupts will be sent to DPC driver, pciehp
> driver at the same time.
> That will cause following result:
>
> 1. Link and device are recovered by hardware DPC and software DPC driver,
> device
> isn't removed, but the pciehp might treat it as device was hot removed.
>
> 2. Race condition happens bettween pciehp_unconfigure_device() called by
> pciehp_ist() in pciehp driver and pci_do_recovery() called by
> dpc_handler in DPC driver. no luck, there is no lock to protect
> pci_stop_and_remove_bus_device()
> against pci_walk_bus(), they hold different samphore and mutex,
> pci_stop_and_remove_bus_device holds pci_rescan_remove_lock, and
> pci_walk_bus() holds pci_bus_sem.
>
> This race condition is not purely code analysis, it could be triggered by
> following command series:
>
> # setpci -s 64:02.0 0x196.w=000a // 64:02.0 rootport has DPC capability
> # setpci -s 65:00.0 0x04.w=0544 // 65:00.0 NVMe SSD populated in port
> # mount /dev/nvme0n1p1 nvme
>
> One shot will cause system panic and NULL pointer reference happened.
> (tested on stable 5.8 & ICS(Ice Lake SP platform, see
> https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server))
>
> Buffer I/O error on dev nvme0n1p1, logical block 3328, async page read
> BUG: kernel NULL pointer dereference, address: 0000000000000050
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0
Seems like you randomly did something about the series and would like
it to be applied?! It's no go!
Please, read my comments again v1 one more time and carefully comment
or address.
Why do you still have these (some above, some below this comment)
non-relevant lines of oops?
> Oops: 0000 [#1] SMP NOPTI
> CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0 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) knl
> GS: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
> irq_thread+0xea/0x170
> ? irq_forced_thread_fn+0x80/0x80
> ? irq_thread_check_affinity+0xf0/0xf0
> kthread+0x124/0x140
> ? kthread_park+0x90/0x90
> ret_from_fork+0x1f/0x30
> Modules linked in: nft_fib_inet.........
> CR2: 0000000000000050
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
And no, this is not how the tags are being applied.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC
2020-09-27 9:10 ` Andy Shevchenko
@ 2020-09-28 2:54 ` Zhao, Haifeng
[not found] ` <MWHPR11MB1696B1DF37BE8F323E67FEE497350@MWHPR11MB1696.namprd11.prod.outlook.com>
1 sibling, 0 replies; 16+ messages in thread
From: Zhao, Haifeng @ 2020-09-28 2:54 UTC (permalink / raw)
To: Andy Shevchenko, Hansen, Dave
Cc: Bjorn Helgaas, Oliver, ruscur, Lukas Wunner, Andy Shevchenko,
Stuart Hayes, Alexandru Gagniuc, Mika Westerberg, linux-pci,
Linux Kernel Mailing List, Jia, Pei P, ashok.raj, Kuppuswamy,
Sathyanarayanan, Christoph Hellwig, Joe Perches
Andy,
May I ask which line of the Oops is " you randomly did something " ? and should be removed ?
Thanks,
Ethan
-----Original Message-----
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Sent: Sunday, September 27, 2020 5:10 PM
To: Zhao, Haifeng <haifeng.zhao@intel.com>; Hansen, Dave <dave.hansen@intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>; Oliver <oohall@gmail.com>; ruscur@russell.cc; Lukas Wunner <lukas@wunner.de>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Stuart Hayes <stuart.w.hayes@gmail.com>; Alexandru Gagniuc <mr.nuke.me@gmail.com>; Mika Westerberg <mika.westerberg@linux.intel.com>; linux-pci <linux-pci@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Jia, Pei P <pei.p.jia@intel.com>; ashok.raj@linux.intel.com; Kuppuswamy, Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>; Christoph Hellwig <hch@infradead.org>; Joe Perches <joe@perches.com>
Subject: Re: [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC
On Sun, Sep 27, 2020 at 11:31 AM Ethan Zhao <haifeng.zhao@intel.com> wrote:
>
> When root port has DPC capability and it is enabled, then triggered by
> errors, DPC DLLSC and PDC interrupts will be sent to DPC driver,
> pciehp driver at the same time.
> That will cause following result:
>
> 1. Link and device are recovered by hardware DPC and software DPC driver,
> device
> isn't removed, but the pciehp might treat it as device was hot removed.
>
> 2. Race condition happens bettween pciehp_unconfigure_device() called by
> pciehp_ist() in pciehp driver and pci_do_recovery() called by
> dpc_handler in DPC driver. no luck, there is no lock to protect
> pci_stop_and_remove_bus_device()
> against pci_walk_bus(), they hold different samphore and mutex,
> pci_stop_and_remove_bus_device holds pci_rescan_remove_lock, and
> pci_walk_bus() holds pci_bus_sem.
>
> This race condition is not purely code analysis, it could be triggered
> by following command series:
>
> # setpci -s 64:02.0 0x196.w=000a // 64:02.0 rootport has DPC capability
> # setpci -s 65:00.0 0x04.w=0544 // 65:00.0 NVMe SSD populated in port
> # mount /dev/nvme0n1p1 nvme
>
> One shot will cause system panic and NULL pointer reference happened.
> (tested on stable 5.8 & ICS(Ice Lake SP platform, see
> https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server
> ))
>
> Buffer I/O error on dev nvme0n1p1, logical block 3328, async page read
> BUG: kernel NULL pointer dereference, address: 0000000000000050
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0
Seems like you randomly did something about the series and would like it to be applied?! It's no go!
Please, read my comments again v1 one more time and carefully comment or address.
Why do you still have these (some above, some below this comment) non-relevant lines of oops?
> Oops: 0000 [#1] SMP NOPTI
> CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0 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) knl
> GS: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
> irq_thread+0xea/0x170
> ? irq_forced_thread_fn+0x80/0x80
> ? irq_thread_check_affinity+0xf0/0xf0
> kthread+0x124/0x140
> ? kthread_park+0x90/0x90
> ret_from_fork+0x1f/0x30
> Modules linked in: nft_fib_inet.........
> CR2: 0000000000000050
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
And no, this is not how the tags are being applied.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <MWHPR11MB1696B1DF37BE8F323E67FEE497350@MWHPR11MB1696.namprd11.prod.outlook.com>]
* Re: [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC
[not found] ` <MWHPR11MB1696B1DF37BE8F323E67FEE497350@MWHPR11MB1696.namprd11.prod.outlook.com>
@ 2020-09-28 8:42 ` Andy Shevchenko
0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2020-09-28 8:42 UTC (permalink / raw)
To: Zhao, Haifeng, Bjorn Helgaas; +Cc: andriy.shevchenko, linux-pci
On Mon, Sep 28, 2020 at 5:24 AM Zhao, Haifeng <haifeng.zhao@intel.com> wrote:
>
> Andy,
> May I ask what you think that's not relevant in the oops ? any rules ?
First of all, please don't send private messages on OSS development
matters (partially returned Cc list).
Second, stop top postings.
Now to the question, the rule is simple — it's called "common sense".
See below the examples, but you may cut even more. So, don't pollute
commit messages with unrelated information.
> On Sun, Sep 27, 2020 at 11:31 AM Ethan Zhao <haifeng.zhao@intel.com> wrote:
...
> > Buffer I/O error on dev nvme0n1p1, logical block 3328, async page read
> > BUG: kernel NULL pointer dereference, address: 0000000000000050
The below...
> > #PF: supervisor read access in kernel mode
> > #PF: error_code(0x0000) - not-present page
> > PGD 0
>
> Seems like you randomly did something about the series and would like it to be applied?! It's no go!
> Please, read my comments again v1 one more time and carefully comment or address.
>
> Why do you still have these (some above, some below this comment) non-relevant lines of oops?
..
> > Oops: 0000 [#1] SMP NOPTI
...is anybody getting new information from the above? I don't think so.
> > CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0 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) knl
> > GS: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
The below, for example, does it affect anyhow the meaning of the crash
log? I don't think so, can be cut.
> > irq_thread+0xea/0x170
> > ? irq_forced_thread_fn+0x80/0x80
> > ? irq_thread_check_affinity+0xf0/0xf0
> > kthread+0x124/0x140
> > ? kthread_park+0x90/0x90
> > ret_from_fork+0x1f/0x30
> > Modules linked in: nft_fib_inet.........
> > CR2: 0000000000000050
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <35e7a493-a6f0-ac16-eb27-8ee5e77312ab@intel.com>]
* RE: [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC
[not found] ` <35e7a493-a6f0-ac16-eb27-8ee5e77312ab@intel.com>
@ 2020-09-28 4:58 ` Zhao, Haifeng
0 siblings, 0 replies; 16+ messages in thread
From: Zhao, Haifeng @ 2020-09-28 4:58 UTC (permalink / raw)
To: Kuppuswamy, Sathyanarayanan, bhelgaas, oohall, ruscur, lukas,
andriy.shevchenko, stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, Jia, Pei P, ashok.raj, hch, joe
Sathyanarayanan,
-----Original Message-----
From: Kuppuswamy, Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
Sent: Monday, September 28, 2020 2:59 AM
To: Zhao, Haifeng <haifeng.zhao@intel.com>; bhelgaas@google.com; oohall@gmail.com; ruscur@russell.cc; lukas@wunner.de; andriy.shevchenko@linux.intel.com; stuart.w.hayes@gmail.com; mr.nuke.me@gmail.com; mika.westerberg@linux.intel.com
Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Jia, Pei P <pei.p.jia@intel.com>; ashok.raj@linux.intel.com; hch@infradead.org; joe@perches.com
Subject: Re: [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC
On 9/27/20 1:27 AM, Ethan Zhao wrote:
> When root port has DPC capability and it is enabled, then triggered by
> errors, DPC DLLSC and PDC interrupts will be sent to DPC driver,
> pciehp driver at the same time.
> That will cause following result:
>
> 1. Link and device are recovered by hardware DPC and software DPC driver,
> device
> isn't removed, but the pciehp might treat it as device was hot removed.
>
> 2. Race condition happens bettween pciehp_unconfigure_device() called by
> pciehp_ist() in pciehp driver and pci_do_recovery() called by
> dpc_handler in DPC driver. no luck, there is no lock to protect
> pci_stop_and_remove_bus_device()
> against pci_walk_bus(), they hold different samphore and mutex,
> pci_stop_and_remove_bus_device holds pci_rescan_remove_lock, and
> pci_walk_bus() holds pci_bus_sem.
Why not address the locking issue? May be a common lock?
>
> This race condition is not purely code analysis, it could be triggered
> by following command series:
>
> # setpci -s 64:02.0 0x196.w=000a // 64:02.0 rootport has DPC capability
> # setpci -s 65:00.0 0x04.w=0544 // 65:00.0 NVMe SSD populated in port
> # mount /dev/nvme0n1p1 nvme
>
> One shot will cause system panic and NULL pointer reference happened.
> (tested on stable 5.8 & ICS(Ice Lake SP platform, see
> https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server
> ))
>
> Buffer I/O error on dev nvme0n1p1, logical block 3328, async page read
> BUG: kernel NULL pointer dereference, address: 0000000000000050
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0
> Oops: 0000 [#1] SMP NOPTI
> CPU: 12 PID: 513 Comm: irq/124-pcie-dp Not tainted 5.8.0 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) knl
> GS: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
> irq_thread+0xea/0x170
> ? irq_forced_thread_fn+0x80/0x80
> ? irq_thread_check_affinity+0xf0/0xf0
> kthread+0x124/0x140
> ? kthread_park+0x90/0x90
> ret_from_fork+0x1f/0x30
> Modules linked in: nft_fib_inet.........
> CR2: 0000000000000050
>
> With this patch, the handling flow of DPC containment and hotplug is
> partly ordered and serialized,
If its a partial fix, what scenario is not covered?
:see the 1/5 patch.
> 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.
>
> 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>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> Changes:
> V2: revise doc according to Andy's suggestion.
> V3: no change.
> V4: 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);
This would add worst case 1s delay in handling the DLLSC events. This does not distinguish between DLLSC event triggered by DPC or hotplug. Also additional delay may violate the timing requirements.
: It will wait only when DPC is enabled and triggered. Or it will skip the waiting.
Test with different time interval between hot-remove and hot-plugin, also no spec
Says it will violate timing requirement. It works.
Thanks,
Ethan
> pciehp_handle_presence_or_link_change(ctrl, events);
> + }
> up_read(&ctrl->reset_lock);
>
> ret = IRQ_HANDLED;
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/5 V4] PCI/ERR: get device before call device driver to avoid NULL pointer reference
2020-09-27 8:27 [PATCH 0/5 V4] Fix DPC hotplug race and enhance error handling Ethan Zhao
2020-09-27 8:27 ` [PATCH 1/5 V4] PCI: define a function to check and wait till port finish DPC handling Ethan Zhao
2020-09-27 8:27 ` [PATCH 2/5 V4] PCI: pciehp: check and wait port status out of DPC before handling DLLSC and PDC Ethan Zhao
@ 2020-09-27 8:27 ` Ethan Zhao
2020-09-27 8:27 ` [PATCH 4/5 V4] PCI: only return true when dev io state is really changed Ethan Zhao
2020-09-27 8:27 ` [PATCH 5/5 V4] PCI/ERR: don't mix io state not changed and no driver together Ethan Zhao
4 siblings, 0 replies; 16+ messages in thread
From: Ethan Zhao @ 2020-09-27 8:27 UTC (permalink / raw)
To: bhelgaas, oohall, ruscur, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, pei.p.jia, ashok.raj,
sathyanarayanan.kuppuswamy, hch, joe, Ethan Zhao
During DPC error injection test we found there is race condition between
pciehp and DPC driver, NULL pointer reference 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))
Buffer I/O error on dev nvme0n1p1, logical block 468843328,
async page read
BUG: kernel NULL pointer dereference, address: 0000000000000050
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0
Oops: 0000 [#1] SMP NOPTI
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
irq_thread+0xea/0x170
? irq_forced_thread_fn+0x80/0x80
? irq_thread_check_affinity+0xf0/0xf0
kthread+0x124/0x140
? kthread_park+0x90/0x90
ret_from_fork+0x1f/0x30
Modules linked in: nft_fib_inet.........
CR2: 0000000000000050
Though we partly close the race condition with patch 'PCI: pciehp: check
and wait port status out of DPC before handling DLLSC and PDC', but there
is no hardware spec or software sequence to guarantee the pcie_ist() run
into pci_wait_port_outdpc() first or DPC triggered status bits being set
first when errors triggered DPC containment procedure, so device still
could be removed by function pci_stop_and_removed_bus_device() then freed
by pci_dev_put() in pciehp driver first during pcie_do_recover()/
pci_walk_bus() is called by dpc_handler() in DPC driver.
Maybe unify pci_bus_sem and pci_rescan_remove_lock to serialize the
removal and walking operation is the right way, but here we use
pci_dev_get() to increase the reference count of device before using the
device to avoid it is freed in use.
With this patch and patch 'PCI: pciehp: check and wait port status out of
DPC before handling DLLSC and PDC', stable 5.9-rc6 could pass the error
injection test and no panic happened.
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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Changes:
V2: revise doc according to Andy's suggestion.
V3: no change.
V4: 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/5 V4] PCI: only return true when dev io state is really changed
2020-09-27 8:27 [PATCH 0/5 V4] Fix DPC hotplug race and enhance error handling Ethan Zhao
` (2 preceding siblings ...)
2020-09-27 8:27 ` [PATCH 3/5 V4] PCI/ERR: get device before call device driver to avoid NULL pointer reference Ethan Zhao
@ 2020-09-27 8:27 ` Ethan Zhao
2020-09-27 9:14 ` Joe Perches
2020-09-27 8:27 ` [PATCH 5/5 V4] PCI/ERR: don't mix io state not changed and no driver together Ethan Zhao
4 siblings, 1 reply; 16+ messages in thread
From: Ethan Zhao @ 2020-09-27 8:27 UTC (permalink / raw)
To: bhelgaas, oohall, ruscur, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, pei.p.jia, ashok.raj,
sathyanarayanan.kuppuswamy, hch, joe, 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 simplify the pci_dev_set_io_state() function to only return true
when dev->error_state is changed.
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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Joe Perches <joe@perches.com>
---
Changnes:
V2: revise description and code according to suggestion from Andy.
V3: change code to simpler.
V4: no change.
drivers/pci/pci.h | 37 +++++--------------------------------
1 file changed, 5 insertions(+), 32 deletions(-)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7cbc1a0..a2c1c7d5f494 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -359,39 +359,12 @@ 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;
- }
- if (changed)
- dev->error_state = new;
- return changed;
+ if (dev->error_state == new)
+ return false;
+
+ dev->error_state = new;
+ return true;
}
static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 4/5 V4] PCI: only return true when dev io state is really changed
2020-09-27 8:27 ` [PATCH 4/5 V4] PCI: only return true when dev io state is really changed Ethan Zhao
@ 2020-09-27 9:14 ` Joe Perches
2020-09-28 1:47 ` Zhao, Haifeng
0 siblings, 1 reply; 16+ messages in thread
From: Joe Perches @ 2020-09-27 9:14 UTC (permalink / raw)
To: Ethan Zhao, bhelgaas, oohall, ruscur, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, pei.p.jia, ashok.raj,
sathyanarayanan.kuppuswamy, hch
On Sun, 2020-09-27 at 04:27 -0400, Ethan Zhao wrote:
> 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 simplify the pci_dev_set_io_state() function to only return true
> when dev->error_state is changed.
>
> 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>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Reviewed-by: Joe Perches <joe@perches.com>
Hi Ethan/Haifeng.
Like Andy, I did not "review" this patch and sign it.
I merely suggested another simplification.
Please do not add -by: lines unless actually received by you.
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 4/5 V4] PCI: only return true when dev io state is really changed
2020-09-27 9:14 ` Joe Perches
@ 2020-09-28 1:47 ` Zhao, Haifeng
0 siblings, 0 replies; 16+ messages in thread
From: Zhao, Haifeng @ 2020-09-28 1:47 UTC (permalink / raw)
To: Joe Perches, bhelgaas, oohall, ruscur, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, Jia, Pei P, ashok.raj, Kuppuswamy,
Sathyanarayanan, hch
Sorry for that offence, I should ask for your permission.
-----Original Message-----
From: Joe Perches <joe@perches.com>
Sent: Sunday, September 27, 2020 5:14 PM
To: Zhao, Haifeng <haifeng.zhao@intel.com>; bhelgaas@google.com; oohall@gmail.com; ruscur@russell.cc; lukas@wunner.de; andriy.shevchenko@linux.intel.com; stuart.w.hayes@gmail.com; mr.nuke.me@gmail.com; mika.westerberg@linux.intel.com
Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Jia, Pei P <pei.p.jia@intel.com>; ashok.raj@linux.intel.com; Kuppuswamy, Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>; hch@infradead.org
Subject: Re: [PATCH 4/5 V4] PCI: only return true when dev io state is really changed
On Sun, 2020-09-27 at 04:27 -0400, Ethan Zhao wrote:
> 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 simplify the pci_dev_set_io_state() function to only return true
> when dev->error_state is changed.
>
> 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>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> Reviewed-by: Joe Perches <joe@perches.com>
Hi Ethan/Haifeng.
Like Andy, I did not "review" this patch and sign it.
I merely suggested another simplification.
Please do not add -by: lines unless actually received by you.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/5 V4] PCI/ERR: don't mix io state not changed and no driver together
2020-09-27 8:27 [PATCH 0/5 V4] Fix DPC hotplug race and enhance error handling Ethan Zhao
` (3 preceding siblings ...)
2020-09-27 8:27 ` [PATCH 4/5 V4] PCI: only return true when dev io state is really changed Ethan Zhao
@ 2020-09-27 8:27 ` Ethan Zhao
4 siblings, 0 replies; 16+ messages in thread
From: Ethan Zhao @ 2020-09-27 8:27 UTC (permalink / raw)
To: bhelgaas, oohall, ruscur, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me, mika.westerberg
Cc: linux-pci, linux-kernel, pei.p.jia, ashok.raj,
sathyanarayanan.kuppuswamy, hch, joe, 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.
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..d85f27c90c26 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 might already being in error handling ...\n");
+ vote = PCI_ERS_RESULT_NONE;
+ } else if (!dev->driver ||
!dev->driver->err_handler ||
!dev->driver->err_handler->error_detected) {
/*
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread