powerpc/eeh: Release EEH device state synchronously
Subsystems (2)
cover powerpc/eeh: Release EEH device state synchronously
Hi everyone,
Here are some fixes and cleanups that have come from other work but that I think stand on their own.
Only one patch ("Release EEH device state synchronously", suggested by Oliver O'Halloran) is a significant change: it moves the cleanup of some EEH device data out of the (possibly asynchronous) device release handler and into the (synchronously called) bus notifier. This is useful for future work as it makes it easier to reason about the lifetimes of EEH structures.
Note that I've left a few WARN_ON_ONCEs in the code because I'm paranoid, but I have not been able to hit them during testing.
Cheers,
Sam.
Sam Bobroff (4):
powerpc/eeh: fix pseries_eeh_configure_bridge() powerpc/eeh: Release EEH device state synchronously powerpc/eeh: Remove workaround from eeh_add_device_late() powerpc/eeh: Clean up edev cleanup for VFs
arch/powerpc/kernel/eeh.c | 49 +++++++++++--------- arch/powerpc/kernel/pci-hotplug.c | 2 - arch/powerpc/kernel/pci_dn.c | 9 +--- arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +- 4 files changed, 29 insertions(+), 33 deletions(-)
Signature · 1 line
2.22.0.216.g00a2a96fc9
PATCH 1/4 powerpc/eeh: fix pseries_eeh_configure_bridge()
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.
--- arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/platforms/pseries/eeh_pseries.c +1 -1
@@ -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;/*
Signature · 1 line
2.22.0.216.g00a2a96fc9
Re: [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge()
Quoted 7 lines · Sam Bobroff
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.
This should probably be a standalone patch. Looks fine otherwise.
Quoted 18 lines
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@@ -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;/*
PATCH 2/4 powerpc/eeh: Release EEH device state synchronously
In reply to Sam BobroffEEH 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.
--- 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 +26 -0
@@ -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) +{ +struct device *dev = data; + +switch (action) { +case BUS_NOTIFY_DEL_DEVICE : +eeh_remove_device (to_pci_dev (dev)); +break ; +default : +break ; + } +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
@@ -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);
Signature · 1 line
2.22.0.216.g00a2a96fc9
Quoted 30 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 +2
-0@@ -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 static int eeh_init(void) · line 1120
Quoted 10 lines
arch/powerpc/kernel/eeh.c +10
-0+{ +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.
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@@ -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 29 lines · Oliver O'Halloran
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@@ -1106,6 +1106,32 @@ static int eeh_init(void)core_initcall_sync (eeh_init);
Quoted 15 lines
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.
Good point, I'll add something for v2.
Quoted 34 lines
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@@ -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);
PATCH 3/4 powerpc/eeh: Remove workaround from eeh_add_device_late()
In reply to Sam BobroffWhen 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.
--- arch/powerpc/kernel/eeh.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)
arch/powerpc/kernel/eeh.c +1 -22
@@ -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 );
Signature · 1 line
2.22.0.216.g00a2a96fc9
Oliver O'Halloran
reviewerRe: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
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 2 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@@ -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 );
Re: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
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_devicestate 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 6 lines
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.
True.
Quoted 40 lines
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@@ -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 );
Oliver O'Halloran
reviewerRe: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
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_devicestate 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
Re: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
Quoted 26 lines · Oliver O'Halloran
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_devicestate 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; }
Quoted 4 lines
No, in the case I'm talking about the
pci_devis torn down andfreed(). After the PE is reset we re-probe the device and create a newpci_dev. If the release of the oldpci_devis delayed we need the hack this patch is removing.
Oh, yes, that is the case I was intending to change here. But I must be missing something, isn't it also the case that's changed by patch 2/4?
What I intended was, after patch 2, eeh_remove_device() is called from the bus notifier so it happens imediately when recovery calls pci_stop_and_remove_bus_device(). Once it returns, edev->pdev has already been set to NULL by eeh_remove_device() so this case can't be hit anymore, and we should clean it up (this patch).
(There is a slight difference in the way EEH_PE_KEEP is handled between the code removed here and the body of eeh_remove_device(), but checking and explaining that is already on my list for v2.)
(I did test recovery on an unaware device and didn't hit the WARN_ON_ONCE().)
Quoted 3 lines
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.
Yeah, I also certainly see the "Device already referenced!" message while debugging, and it would be good to track down.
Quoted 1 line
Oliver
PATCH 4/4 powerpc/eeh: Clean up edev cleanup for VFs
In reply to Sam BobroffBecause 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.
So remove the call.
--- arch/powerpc/kernel/pci_dn.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
arch/powerpc/kernel/pci_dn.c +1 -8
@@ -236,14 +236,7 @@ void remove_sriov_vf_pdns(struct pci_dev *pdev)*/ edev =pdn_to_eeh_dev (pdn);if (edev) { -/* -* We allocate pci_dn's for the totalvfs count, -* but only only the vfs that were activated -* have a configured PE. -*/ -if (edev->pe) -eeh_rmv_from_parent_pe (edev); - +WARN_ON_ONCE (edev->pe); pdn->edev =NULL ;kfree (edev); }
Signature · 1 line
2.22.0.216.g00a2a96fc9
Oliver O'Halloran
reviewerRe: [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs
Quoted 3 lines · Sam Bobroff
Because the bus notifier calls
eeh_rmv_from_parent_pe()(viaeeh_remove_device()) when a VF is removed, the call inremove_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?
Re: [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs
Quoted 9 lines · Oliver O'Halloran
Quoted 3 lines · Sam Bobroff
Because the bus notifier calls
eeh_rmv_from_parent_pe()(viaeeh_remove_device()) when a VF is removed, the call inremove_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?
Ah, I assume you're referring to the difference between calling eeh_remove_device() and directly calling eeh_rmv_from_parent_pe(), where the behaviour for PE's with EEH_PE_KEEP set is subtly different.
I'll take a closer look at it and make sure to explain it better in v2.