3.16.60-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Michael Neuling mikey@neuling.org
commit f0295e047fcf52ccb42561fb7de6942f5201b676 upstream.
The current EEH callbacks can race with a driver unbind. This can result in a backtraces like this:
EEH: Frozen PHB#0-PE#1fc detected EEH: PE location: S000009, PHB location: N/A CPU: 2 PID: 2312 Comm: kworker/u258:3 Not tainted 4.15.6-openpower1 #2 Workqueue: nvme-wq nvme_reset_work [nvme] Call Trace: dump_stack+0x9c/0xd0 (unreliable) eeh_dev_check_failure+0x420/0x470 eeh_check_failure+0xa0/0xa4 nvme_reset_work+0x138/0x1414 [nvme] process_one_work+0x1ec/0x328 worker_thread+0x2e4/0x3a8 kthread+0x14c/0x154 ret_from_kernel_thread+0x5c/0xc8 nvme nvme1: Removing after probe failure status: -19 <snip> cpu 0x23: Vector: 300 (Data Access) at [c000000ff50f3800] pc: c0080000089a0eb0: nvme_error_detected+0x4c/0x90 [nvme] lr: c000000000026564: eeh_report_error+0xe0/0x110 sp: c000000ff50f3a80 msr: 9000000000009033 dar: 400 dsisr: 40000000 current = 0xc000000ff507c000 paca = 0xc00000000fdc9d80 softe: 0 irq_happened: 0x01 pid = 782, comm = eehd Linux version 4.15.6-openpower1 (smc@smc-desktop) (gcc version 6.4.0 (Buildroot 2017.11.2-00008-g4b6188e)) #2 SM P Tue Feb 27 12:33:27 PST 2018 enter ? for help eeh_report_error+0xe0/0x110 eeh_pe_dev_traverse+0xc0/0xdc eeh_handle_normal_event+0x184/0x4c4 eeh_handle_event+0x30/0x288 eeh_event_handler+0x124/0x170 kthread+0x14c/0x154 ret_from_kernel_thread+0x5c/0xc8
The first part is an EEH (on boot), the second half is the resulting crash. nvme probe starts the nvme_reset_work() worker thread. This worker thread starts touching the device which see a device error (EEH) and hence queues up an event in the powerpc EEH worker thread. nvme_reset_work() then continues and runs nvme_remove_dead_ctrl_work() which results in unbinding the driver from the device and hence releases all resources. At the same time, the EEH worker thread starts doing the EEH .error_detected() driver callback, which no longer works since the resources have been freed.
This fixes the problem in the same way the generic PCIe AER code (in drivers/pci/pcie/aer/aerdrv_core.c) does. It makes the EEH code hold the device_lock() while performing the driver EEH callbacks and associated code. This ensures either the callbacks are no longer register, or if they are registered the driver will not be removed from underneath us.
This has been broken forever. The EEH call backs were first introduced in 2005 (in 77bd7415610) but it's not clear if a lock was needed back then.
Fixes: 77bd74156101 ("[PATCH] powerpc: PCI Error Recovery: PPC64 core recovery routines") Signed-off-by: Michael Neuling mikey@neuling.org Reviewed-by: Benjamin Herrenschmidt benh@kernel.crashing.org Signed-off-by: Michael Ellerman mpe@ellerman.id.au [bwh: Backported to 3.16: adjust context] Signed-off-by: Ben Hutchings ben@decadent.org.uk --- --- a/arch/powerpc/kernel/eeh_driver.c +++ b/arch/powerpc/kernel/eeh_driver.c @@ -198,18 +198,18 @@ static void *eeh_report_error(void *data
if (!dev || eeh_dev_removed(edev)) return NULL; + + device_lock(&dev->dev); dev->error_state = pci_channel_io_frozen;
driver = eeh_pcid_get(dev); - if (!driver) return NULL; + if (!driver) goto out_no_dev;
eeh_disable_irq(dev);
if (!driver->err_handler || - !driver->err_handler->error_detected) { - eeh_pcid_put(dev); - return NULL; - } + !driver->err_handler->error_detected) + goto out;
rc = driver->err_handler->error_detected(dev, pci_channel_io_frozen);
@@ -217,7 +217,10 @@ static void *eeh_report_error(void *data if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc; if (*res == PCI_ERS_RESULT_NONE) *res = rc;
+out: eeh_pcid_put(dev); +out_no_dev: + device_unlock(&dev->dev); return NULL; }
@@ -240,15 +243,14 @@ static void *eeh_report_mmio_enabled(voi if (!dev || eeh_dev_removed(edev)) return NULL;
+ device_lock(&dev->dev); driver = eeh_pcid_get(dev); - if (!driver) return NULL; + if (!driver) goto out_no_dev;
if (!driver->err_handler || !driver->err_handler->mmio_enabled || - (edev->mode & EEH_DEV_NO_HANDLER)) { - eeh_pcid_put(dev); - return NULL; - } + (edev->mode & EEH_DEV_NO_HANDLER)) + goto out;
rc = driver->err_handler->mmio_enabled(dev);
@@ -256,7 +258,10 @@ static void *eeh_report_mmio_enabled(voi if (rc == PCI_ERS_RESULT_NEED_RESET) *res = rc; if (*res == PCI_ERS_RESULT_NONE) *res = rc;
+out: eeh_pcid_put(dev); +out_no_dev: + device_unlock(&dev->dev); return NULL; }
@@ -279,19 +284,19 @@ static void *eeh_report_reset(void *data
if (!dev || eeh_dev_removed(edev)) return NULL; + + device_lock(&dev->dev); dev->error_state = pci_channel_io_normal;
driver = eeh_pcid_get(dev); - if (!driver) return NULL; + if (!driver) goto out_no_dev;
eeh_enable_irq(dev);
if (!driver->err_handler || !driver->err_handler->slot_reset || - (edev->mode & EEH_DEV_NO_HANDLER)) { - eeh_pcid_put(dev); - return NULL; - } + (edev->mode & EEH_DEV_NO_HANDLER)) + goto out;
rc = driver->err_handler->slot_reset(dev); if ((*res == PCI_ERS_RESULT_NONE) || @@ -299,7 +304,10 @@ static void *eeh_report_reset(void *data if (*res == PCI_ERS_RESULT_DISCONNECT && rc == PCI_ERS_RESULT_NEED_RESET) *res = rc;
+out: eeh_pcid_put(dev); +out_no_dev: + device_unlock(&dev->dev); return NULL; }
@@ -320,10 +328,12 @@ static void *eeh_report_resume(void *dat
if (!dev || eeh_dev_removed(edev)) return NULL; + + device_lock(&dev->dev); dev->error_state = pci_channel_io_normal;
driver = eeh_pcid_get(dev); - if (!driver) return NULL; + if (!driver) goto out_no_dev;
eeh_enable_irq(dev);
@@ -331,13 +341,15 @@ static void *eeh_report_resume(void *dat !driver->err_handler->resume || (edev->mode & EEH_DEV_NO_HANDLER)) { edev->mode &= ~EEH_DEV_NO_HANDLER; - eeh_pcid_put(dev); - return NULL; + goto out; }
driver->err_handler->resume(dev);
+out: eeh_pcid_put(dev); +out_no_dev: + device_unlock(&dev->dev); return NULL; }
@@ -357,22 +369,25 @@ static void *eeh_report_failure(void *da
if (!dev || eeh_dev_removed(edev)) return NULL; + + device_lock(&dev->dev); dev->error_state = pci_channel_io_perm_failure;
driver = eeh_pcid_get(dev); - if (!driver) return NULL; + if (!driver) goto out_no_dev;
eeh_disable_irq(dev);
if (!driver->err_handler || - !driver->err_handler->error_detected) { - eeh_pcid_put(dev); - return NULL; - } + !driver->err_handler->error_detected) + goto out;
driver->err_handler->error_detected(dev, pci_channel_io_perm_failure);
+out: eeh_pcid_put(dev); +out_no_dev: + device_unlock(&dev->dev); return NULL; }