powerpc/eeh: Release EEH device state synchronously

If a device is hot unplgged during EEH recovery, it's possible for the RTAS call to ibm,configure-pe in pseries_eeh_configure() to return parameter error (-3), however negative return values are not checked for and this leads to an infinite loop.

Fix this by correctly bailing out on negative values.

Signed-off-by Sam Bobroff <[email protected]>

---
 arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/platforms/pseries/eeh_pseries.c +1 -1

index 893ba3f562c4..c4ef03bec0de 100644
@@ -605,7 +605,7 @@ static int pseries_eeh_configure_bridge(struct eeh_pe *pe)
 				config_addr, BUID_HI(pe->phb->buid),
 				BUID_LO(pe->phb->buid));
 
-		if (!ret)
+		if (ret <= 0)
 			return ret;
 
 		/*

EEH device state is currently removed (by eeh_remove_device()) during the device release handler, which is invoked as the device's reference count drops to zero. This may take some time, or forever, as other threads may hold references.

However, the PCI device state is released synchronously by pci_stop_and_remove_bus_device(). This mismatch causes problems, for example the device may be re-discovered as a new device before the release handler has been called, leaving the PCI and EEH state mismatched.

So instead, call eeh_remove_device() from the bus device removal handlers, which are called synchronously in the removal path.

Signed-off-by Sam Bobroff <[email protected]>

---
 arch/powerpc/kernel/eeh.c         | 26 ++++++++++++++++++++++++++
 arch/powerpc/kernel/pci-hotplug.c |  2 --
 2 files changed, 26 insertions(+), 2 deletions(-)

arch/powerpc/kernel/eeh.c +2 -0

index 17cb3e9b5697..c36c5a7db5ca 100644
@@ -1106,6 +1106,32 @@ static int eeh_init(void)
 
 core_initcall_sync(eeh_init);
 
+static int eeh_device_notifier(struct notifier_block *nb,
+			       unsigned long action, void *data)

arch/powerpc/kernel/eeh.c +14 -0

+	return NOTIFY_DONE;
+}
+
+static struct notifier_block eeh_device_nb = {
+	.notifier_call = eeh_device_notifier,
+};
+
+static __init int eeh_set_bus_notifier(void)
+{
+	bus_register_notifier(&pci_bus_type, &eeh_device_nb);
+	return 0;
+}
+arch_initcall(eeh_set_bus_notifier);
+
 /**
  * eeh_add_device_early - Enable EEH for the indicated device node
  * @pdn: PCI device node for which to set up EEH

arch/powerpc/kernel/pci-hotplug.c +0 -2

index d6a67f814983..28e9aa274f64 100644
@@ -57,8 +57,6 @@ void pcibios_release_device(struct pci_dev *dev)
 	struct pci_controller *phb = pci_bus_to_host(dev->bus);
 	struct pci_dn *pdn = pci_get_pdn(dev);
 
-	eeh_remove_device(dev);
-
 	if (phb->controller_ops.release_device)
 		phb->controller_ops.release_device(dev);
 

Because the bus notifier calls eeh_rmv_from_parent_pe() (via eeh_remove_device()) when a VF is removed, the call in remove_sriov_vf_pdns() is redundant.

When EEH device state was released asynchronously by the device release handler, it was possible for an outstanding reference to prevent it's release and it was necessary to work around that if a device was re-discovered at the same PCI location.

Now that the state is released synchronously that is no longer possible and the workaround is no longer necessary.

Signed-off-by Sam Bobroff <[email protected]>

---
 arch/powerpc/kernel/eeh.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

arch/powerpc/kernel/eeh.c +1 -22

index c36c5a7db5ca..12c248a16527 100644
@@ -1206,28 +1206,7 @@ void eeh_add_device_late(struct pci_dev *dev)
 		eeh_edev_dbg(edev, "Device already referenced!\n");
 		return;
 	}
-
-	/*
-	 * The EEH cache might not be removed correctly because of
-	 * unbalanced kref to the device during unplug time, which
-	 * relies on pcibios_release_device(). So we have to remove
-	 * that here explicitly.
-	 */
-	if (edev->pdev) {
-		eeh_rmv_from_parent_pe(edev);
-		eeh_addr_cache_rmv_dev(edev->pdev);
-		eeh_sysfs_remove_device(edev->pdev);
-
-		/*
-		 * We definitely should have the PCI device removed
-		 * though it wasn't correctly. So we needn't call
-		 * into error handler afterwards.
-		 */
-		edev->mode |= EEH_DEV_NO_HANDLER;
-
-		edev->pdev = NULL;
-		dev->dev.archdata.edev = NULL;
-	}
+	WARN_ON_ONCE(edev->pdev);
 
 	if (eeh_has_flag(EEH_PROBE_MODE_DEV))
 		eeh_ops->probe(pdn, NULL);

Quoted 28 lines · Sam Bobroff

EEH device state is currently removed (by eeh_remove_device()) during the device release handler, which is invoked as the device's reference count drops to zero. This may take some time, or forever, as other threads may hold references.

However, the PCI device state is released synchronously by pci_stop_and_remove_bus_device(). This mismatch causes problems, for example the device may be re-discovered as a new device before the release handler has been called, leaving the PCI and EEH state mismatched.

So instead, call eeh_remove_device() from the bus device removal handlers, which are called synchronously in the removal path.

Signed-off-by Sam Bobroff <[email protected]>

---
 arch/powerpc/kernel/eeh.c         | 26 ++++++++++++++++++++++++++
 arch/powerpc/kernel/pci-hotplug.c |  2 --
 2 files changed, 26 insertions(+), 2 deletions(-)

arch/powerpc/kernel/eeh.c +0 -0

index 17cb3e9b5697..c36c5a7db5ca 100644
@@ -1106,6 +1106,32 @@ static int eeh_init(void)
 
 core_initcall_sync(eeh_init);
 

arch/powerpc/kernel/eeh.c static int eeh_init(void) · line 1120

Quoted 12 lines

arch/powerpc/kernel/eeh.c +12 -0

+static int eeh_device_notifier(struct notifier_block *nb,
+			       unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	switch (action) {
+	case BUS_NOTIFY_DEL_DEVICE:
+		eeh_remove_device(to_pci_dev(dev));
+		break;
+	default:
+		break;
+	}

A comment briefly explaining why we're not doing anything in the add case might be nice.

Reviewed-by Oliver O'Halloran <[email protected]>

Quoted 30 lines

arch/powerpc/kernel/eeh.c +14 -0

+	return NOTIFY_DONE;
+}
+
+static struct notifier_block eeh_device_nb = {
+	.notifier_call = eeh_device_notifier,
+};
+
+static __init int eeh_set_bus_notifier(void)
+{
+	bus_register_notifier(&pci_bus_type, &eeh_device_nb);
+	return 0;
+}
+arch_initcall(eeh_set_bus_notifier);
+
 /**
  * eeh_add_device_early - Enable EEH for the indicated device node
  * @pdn: PCI device node for which to set up EEH

arch/powerpc/kernel/pci-hotplug.c +0 -2

index d6a67f814983..28e9aa274f64 100644
@@ -57,8 +57,6 @@ void pcibios_release_device(struct pci_dev *dev)
 	struct pci_controller *phb = pci_bus_to_host(dev->bus);
 	struct pci_dn *pdn = pci_get_pdn(dev);
 
-	eeh_remove_device(dev);
-
 	if (phb->controller_ops.release_device)
 		phb->controller_ops.release_device(dev);
 

Quoted 4 lines · Sam Bobroff

When EEH device state was released asynchronously by the device release handler, it was possible for an outstanding reference to prevent it's release and it was necessary to work around that if a device was re-discovered at the same PCI location.

I think this is a bit misleading. The main situation where you'll hit this hack is when recovering a device with a driver that doesn't implement the error handling callbacks. In that case the device is removed, reset, then re-probed by the PCI core, but we assume it's the same physical device so the eeh_device state remains active.

If you actually changed the underlying device I suspect something bad would happen.

Quoted 3 lines

Now that the state is released synchronously that is no longer possible and the workaround is no longer necessary.

You could probably fold this into the previous patch, but eh. You could probably fold this into the previous patch, but eh.

Quoted 39 lines

Signed-off-by Sam Bobroff <[email protected]>

---
 arch/powerpc/kernel/eeh.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

arch/powerpc/kernel/eeh.c +1 -22

index c36c5a7db5ca..12c248a16527 100644
@@ -1206,28 +1206,7 @@ void eeh_add_device_late(struct pci_dev *dev)
 		eeh_edev_dbg(edev, "Device already referenced!\n");
 		return;
 	}
-
-	/*
-	 * The EEH cache might not be removed correctly because of
-	 * unbalanced kref to the device during unplug time, which
-	 * relies on pcibios_release_device(). So we have to remove
-	 * that here explicitly.
-	 */
-	if (edev->pdev) {
-		eeh_rmv_from_parent_pe(edev);
-		eeh_addr_cache_rmv_dev(edev->pdev);
-		eeh_sysfs_remove_device(edev->pdev);
-
-		/*
-		 * We definitely should have the PCI device removed
-		 * though it wasn't correctly. So we needn't call
-		 * into error handler afterwards.
-		 */
-		edev->mode |= EEH_DEV_NO_HANDLER;
-
-		edev->pdev = NULL;
-		dev->dev.archdata.edev = NULL;
-	}
+	WARN_ON_ONCE(edev->pdev);
 
 	if (eeh_has_flag(EEH_PROBE_MODE_DEV))
 		eeh_ops->probe(pdn, NULL);

Quoted 3 lines · Sam Bobroff

Because the bus notifier calls eeh_rmv_from_parent_pe() (via eeh_remove_device()) when a VF is removed, the call in remove_sriov_vf_pdns() is redundant.

eeh_rmv_from_parent_pe() won't actually remove the device if the recovering flag is set on the PE. Are you sure we're not introducing a race here?

Quoted 6 lines · Oliver O'Halloran

Quoted 4 lines · Sam Bobroff

When EEH device state was released asynchronously by the device release handler, it was possible for an outstanding reference to prevent it's release and it was necessary to work around that if a device was re-discovered at the same PCI location.

Quoted 8 lines

I think this is a bit misleading. The main situation where you'll hit this hack is when recovering a device with a driver that doesn't implement the error handling callbacks. In that case the device is removed, reset, then re-probed by the PCI core, but we assume it's the same physical device so the eeh_device state remains active.

If you actually changed the underlying device I suspect something bad would happen.

I'm not sure I understand. Isn't the case you're talking about caught by the earlier check (just above the patch)?

        if (edev->pdev == dev) {
                eeh_edev_dbg(edev, "Device already referenced!\n");
                return;
        }

Quoted 17 lines · Sam Bobroff

Quoted 6 lines · Oliver O'Halloran

Quoted 4 lines · Sam Bobroff

When EEH device state was released asynchronously by the device release handler, it was possible for an outstanding reference to prevent it's release and it was necessary to work around that if a device was re-discovered at the same PCI location.

Quoted 8 lines

I think this is a bit misleading. The main situation where you'll hit this hack is when recovering a device with a driver that doesn't implement the error handling callbacks. In that case the device is removed, reset, then re-probed by the PCI core, but we assume it's the same physical device so the eeh_device state remains active.

If you actually changed the underlying device I suspect something bad would happen.

Quoted 7 lines

I'm not sure I understand. Isn't the case you're talking about caught by the earlier check (just above the patch)?

        if (edev->pdev == dev) {
                eeh_edev_dbg(edev, "Device already referenced!\n");
                return;
        }

No, in the case I'm talking about the pci_dev is torn down and freed(). After the PE is reset we re-probe the device and create a new pci_dev. If the release of the old pci_dev is delayed we need the hack this patch is removing.

The check above should probably be a WARN_ON() since we should never be re-running the EEH probe on the same device. I think there is a case where that can happen, but I don't remember the details.

Oliver