powerpc/eeh: Release EEH device state synchronously
Subsystems (2)
v2 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.
Notes for v2:
I've dropped both cleanup patches (3/4, 4/4) because that type of cleanup (replacing a call to eeh_rmv_from_parent_pe() with one to eeh_remove_device()) is incorrect: if called during recovery, it will cause edev->pe to remain set when it would have been cleared previously. This would lead to stale information in the edev. I think there should be a way to simplify the code around EEH_PE_KEEP but I'll look at that separately.
Patch set changelog follows:
Patch set v2:
Patch 1/2: powerpc/eeh: fix pseries_eeh_configure_bridge()
Patch 2/2: powerpc/eeh: Release EEH device state synchronously
- Added comment explaining why the add case can't be handled similarly to the remove case. Dropped (was 3/4) powerpc/eeh: Remove workaround from eeh_add_device_late() Dropped (was 4/4) powerpc/eeh: Clean up edev cleanup for VFs
Patch set v1:
Patch 1/4: powerpc/eeh: fix pseries_eeh_configure_bridge() Patch 2/4: powerpc/eeh: Release EEH device state synchronously Patch 3/4: powerpc/eeh: Remove workaround from eeh_add_device_late() Patch 4/4: powerpc/eeh: Clean up edev cleanup for VFs
Sam Bobroff (2):
powerpc/eeh: fix pseries_eeh_configure_bridge() powerpc/eeh: Release EEH device state synchronously
arch/powerpc/kernel/eeh.c | 31 ++++++++++++++++++++ arch/powerpc/kernel/pci-hotplug.c | 2 -- arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +- 3 files changed, 32 insertions(+), 3 deletions(-)
Signature · 1 line
2.22.0.216.g00a2a96fc9
PATCH v2 1/2 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 v2 1/2] powerpc/eeh: fix pseries_eeh_configure_bridge()
Quoted 17 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.
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 +0
-0
arch/powerpc/platforms/pseries/eeh_pseries.c static int pseries_eeh_configure_bridge(struct eeh_pe *pe) · line 609
Quoted 6 lines
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;
Note that this returns the firmware error value (e.g. -3 parameter error) without converting it to a Linux errno. Nothing checks the error value of this function as best I can tell, but -EINVAL would be better than an implicit -ESRCH here.
And while this will behave correctly, the pr_warn() at the end of pseries_eeh_configure_bridge() hints that someone had the intention that this code should log a message on such an error:
static int pseries_eeh_configure_bridge(struct eeh_pe *pe)
{
int config_addr;
int ret;
/* Waiting 0.2s maximum before skipping configuration */
int max_wait = 200;
/* Figure out the PE address */
config_addr = pe->config_addr;
if (pe->addr)
config_addr = pe->addr;
while (max_wait > 0) {
ret = rtas_call(ibm_configure_pe, 3, 1, NULL,
config_addr, BUID_HI(pe->phb->buid),
BUID_LO(pe->phb->buid));
if (!ret)
return ret;
/*
* If RTAS returns a delay value that's above 100ms, cut it
* down to 100ms in case firmware made a mistake. For more
* on how these delay values work see rtas_busy_delay_time
*/
if (ret > RTAS_EXTENDED_DELAY_MIN+2 &&
ret <= RTAS_EXTENDED_DELAY_MAX)
ret = RTAS_EXTENDED_DELAY_MIN+2;
max_wait -= rtas_busy_delay_time(ret);
if (max_wait < 0)
break;
rtas_busy_delay(ret);
}
pr_warn("%s: Unable to configure bridge PHB#%x-PE#%x (%d)\n",
__func__, pe->phb->global_number, pe->addr, ret);
return ret;
}So perhaps the error path should be made to break out of the loop instead of returning. Or is the parameter error result simply uninteresting in this scenario?
Re: [PATCH v2 1/2] powerpc/eeh: fix pseries_eeh_configure_bridge()
Quoted 21 lines · Nathan Lynch
Quoted 20 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.
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 +0
-0@@ -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));
Quoted 8 lines
arch/powerpc/platforms/pseries/eeh_pseries.c static int pseries_eeh_configure_bridge(struct eeh_pe *pe) · line 609
Quoted 3 lines
arch/powerpc/platforms/pseries/eeh_pseries.c +1
-1-if (!ret) +if (ret <=0 )return ret;Note that this returns the firmware error value (e.g. -3 parameter error) without converting it to a Linux errno. Nothing checks the error value of this function as best I can tell, but
-EINVALwould be better than an implicit-ESRCHhere.
Right, it's never used but I agree. I'll change it for v3.
Quoted 46 lines
And while this will behave correctly, the
pr_warn()at the end ofpseries_eeh_configure_bridge()hints that someone had the intention that this code should log a message on such an error:static int
pseries_eeh_configure_bridge(struct eeh_pe *pe){ int config_addr; int ret; /* Waiting 0.2s maximum before skipping configuration */ int max_wait = 200; /* Figure out the PE address */ config_addr = pe->config_addr; if (pe->addr) config_addr = pe->addr; while (max_wait > 0) { ret = rtas_call(ibm_configure_pe, 3, 1, NULL, config_addr, BUID_HI(pe->phb->buid), BUID_LO(pe->phb->buid)); if (!ret) return ret; /* * If RTAS returns a delay value that's above 100ms, cut it * down to 100ms in case firmware made a mistake. For more * on how these delay values work see rtas_busy_delay_time */ if (ret > RTAS_EXTENDED_DELAY_MIN+2 && ret <= RTAS_EXTENDED_DELAY_MAX) ret = RTAS_EXTENDED_DELAY_MIN+2; max_wait -= rtas_busy_delay_time(ret); if (max_wait < 0) break; rtas_busy_delay(ret); } pr_warn("%s: Unable to configure bridge PHB#%x-PE#%x (%d)\n", __func__, pe->phb->global_number, pe->addr, ret); return ret; }
Quoted 3 lines
So perhaps the error path should be made to break out of the loop instead of returning. Or is the parameter error result simply uninteresting in this scenario?
Sounds reasonable to me, and given that the only way I know to trigger the error path (see the commit message) is not going to be common, I think a message is a good idea. (And, as one of the people likely to debug a future issue here, I'll probably appreciate it.)
Cheers,
Sam.
PATCH v2 2/2 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.
---
v2 - Added comment explaining why the add case can't be handled similarly to the remove case.
arch/powerpc/kernel/eeh.c | 31 +++++++++++++++++++++++++++++++ arch/powerpc/kernel/pci-hotplug.c | 2 -- 2 files changed, 31 insertions(+), 2 deletions(-)
arch/powerpc/kernel/eeh.c +31 -0
@@ -1106,6 +1106,37 @@ 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) { +/* +* Note: It's not possible to perform EEH device addition (i.e. +* {pseries,pnv}_pcibios_bus_add_device()) here because it depends on +* the device's resources, which have not yet been set up. +*/ +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