All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] powerpc/eeh: Release EEH device state synchronously
@ 2020-03-30  4:56 Sam Bobroff
  2020-03-30  4:56 ` [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge() Sam Bobroff
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Sam Bobroff @ 2020-03-30  4:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Oliver O'Halloran

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(-)

-- 
2.22.0.216.g00a2a96fc9


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge()
  2020-03-30  4:56 [PATCH 0/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
@ 2020-03-30  4:56 ` Sam Bobroff
  2020-04-03  4:19   ` Oliver O'Halloran
  2020-03-30  4:56 ` [PATCH 2/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Sam Bobroff @ 2020-03-30  4:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Oliver O'Halloran

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 <sbobroff@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 893ba3f562c4..c4ef03bec0de 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -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;
 
 		/*
-- 
2.22.0.216.g00a2a96fc9


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] powerpc/eeh: Release EEH device state synchronously
  2020-03-30  4:56 [PATCH 0/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
  2020-03-30  4:56 ` [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge() Sam Bobroff
@ 2020-03-30  4:56 ` Sam Bobroff
  2020-04-03  4:51   ` Oliver O'Halloran
  2020-03-30  4:56 ` [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late() Sam Bobroff
  2020-03-30  4:56 ` [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs Sam Bobroff
  3 siblings, 1 reply; 14+ messages in thread
From: Sam Bobroff @ 2020-03-30  4:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Oliver O'Halloran

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 <sbobroff@linux.ibm.com>
---
 arch/powerpc/kernel/eeh.c         | 26 ++++++++++++++++++++++++++
 arch/powerpc/kernel/pci-hotplug.c |  2 --
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 17cb3e9b5697..c36c5a7db5ca 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -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
diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
index d6a67f814983..28e9aa274f64 100644
--- a/arch/powerpc/kernel/pci-hotplug.c
+++ b/arch/powerpc/kernel/pci-hotplug.c
@@ -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);
 
-- 
2.22.0.216.g00a2a96fc9


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
  2020-03-30  4:56 [PATCH 0/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
  2020-03-30  4:56 ` [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge() Sam Bobroff
  2020-03-30  4:56 ` [PATCH 2/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
@ 2020-03-30  4:56 ` Sam Bobroff
  2020-04-03  6:08   ` Oliver O'Halloran
  2020-03-30  4:56 ` [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs Sam Bobroff
  3 siblings, 1 reply; 14+ messages in thread
From: Sam Bobroff @ 2020-03-30  4:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Oliver O'Halloran

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 <sbobroff@linux.ibm.com>
---
 arch/powerpc/kernel/eeh.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index c36c5a7db5ca..12c248a16527 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -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);
-- 
2.22.0.216.g00a2a96fc9


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs
  2020-03-30  4:56 [PATCH 0/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
                   ` (2 preceding siblings ...)
  2020-03-30  4:56 ` [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late() Sam Bobroff
@ 2020-03-30  4:56 ` Sam Bobroff
  2020-04-03  5:45   ` Oliver O'Halloran
  3 siblings, 1 reply; 14+ messages in thread
From: Sam Bobroff @ 2020-03-30  4:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Oliver O'Halloran

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.

So remove the call.

Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
---
 arch/powerpc/kernel/pci_dn.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 4e654df55969..f6ac25f7af63 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -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);
 			}
-- 
2.22.0.216.g00a2a96fc9


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge()
  2020-03-30  4:56 ` [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge() Sam Bobroff
@ 2020-04-03  4:19   ` Oliver O'Halloran
  0 siblings, 0 replies; 14+ messages in thread
From: Oliver O'Halloran @ 2020-04-03  4:19 UTC (permalink / raw)
  To: Sam Bobroff, linuxppc-dev

On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> 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.

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>


> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index 893ba3f562c4..c4ef03bec0de 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
> @@ -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;
>  
>  		/*


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] powerpc/eeh: Release EEH device state synchronously
  2020-03-30  4:56 ` [PATCH 2/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
@ 2020-04-03  4:51   ` Oliver O'Halloran
  2020-04-08  6:15     ` Sam Bobroff
  0 siblings, 1 reply; 14+ messages in thread
From: Oliver O'Halloran @ 2020-04-03  4:51 UTC (permalink / raw)
  To: Sam Bobroff, linuxppc-dev

On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> 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 <sbobroff@linux.ibm.com>
> ---
>  arch/powerpc/kernel/eeh.c         | 26 ++++++++++++++++++++++++++
>  arch/powerpc/kernel/pci-hotplug.c |  2 --
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 17cb3e9b5697..c36c5a7db5ca 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -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;
> +	}

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

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>

> +	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
> diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
> index d6a67f814983..28e9aa274f64 100644
> --- a/arch/powerpc/kernel/pci-hotplug.c
> +++ b/arch/powerpc/kernel/pci-hotplug.c
> @@ -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);
>  


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs
  2020-03-30  4:56 ` [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs Sam Bobroff
@ 2020-04-03  5:45   ` Oliver O'Halloran
  2020-04-08  6:33     ` Sam Bobroff
  0 siblings, 1 reply; 14+ messages in thread
From: Oliver O'Halloran @ 2020-04-03  5:45 UTC (permalink / raw)
  To: Sam Bobroff, linuxppc-dev

On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> 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?


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
  2020-03-30  4:56 ` [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late() Sam Bobroff
@ 2020-04-03  6:08   ` Oliver O'Halloran
  2020-04-08  6:21     ` Sam Bobroff
  0 siblings, 1 reply; 14+ messages in thread
From: Oliver O'Halloran @ 2020-04-03  6:08 UTC (permalink / raw)
  To: Sam Bobroff, linuxppc-dev

On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> 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.

> 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.

> Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
>  arch/powerpc/kernel/eeh.c | 23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index c36c5a7db5ca..12c248a16527 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -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);


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] powerpc/eeh: Release EEH device state synchronously
  2020-04-03  4:51   ` Oliver O'Halloran
@ 2020-04-08  6:15     ` Sam Bobroff
  0 siblings, 0 replies; 14+ messages in thread
From: Sam Bobroff @ 2020-04-08  6:15 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

On Fri, Apr 03, 2020 at 03:51:18PM +1100, Oliver O'Halloran wrote:
> On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> > 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 <sbobroff@linux.ibm.com>
> > ---
> >  arch/powerpc/kernel/eeh.c         | 26 ++++++++++++++++++++++++++
> >  arch/powerpc/kernel/pci-hotplug.c |  2 --
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> > index 17cb3e9b5697..c36c5a7db5ca 100644
> > --- a/arch/powerpc/kernel/eeh.c
> > +++ b/arch/powerpc/kernel/eeh.c
> > @@ -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;
> > +	}
> 
> 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.
> 
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
> 
> > +	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
> > diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
> > index d6a67f814983..28e9aa274f64 100644
> > --- a/arch/powerpc/kernel/pci-hotplug.c
> > +++ b/arch/powerpc/kernel/pci-hotplug.c
> > @@ -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);
> >  
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
  2020-04-03  6:08   ` Oliver O'Halloran
@ 2020-04-08  6:21     ` Sam Bobroff
  2020-04-08  6:53       ` Oliver O'Halloran
  0 siblings, 1 reply; 14+ messages in thread
From: Sam Bobroff @ 2020-04-08  6:21 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

On Fri, Apr 03, 2020 at 05:08:32PM +1100, Oliver O'Halloran wrote:
> On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> > 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.

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;
	}
> 
> > 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.

> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > ---
> >  arch/powerpc/kernel/eeh.c | 23 +----------------------
> >  1 file changed, 1 insertion(+), 22 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> > index c36c5a7db5ca..12c248a16527 100644
> > --- a/arch/powerpc/kernel/eeh.c
> > +++ b/arch/powerpc/kernel/eeh.c
> > @@ -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);
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs
  2020-04-03  5:45   ` Oliver O'Halloran
@ 2020-04-08  6:33     ` Sam Bobroff
  0 siblings, 0 replies; 14+ messages in thread
From: Sam Bobroff @ 2020-04-08  6:33 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Fri, Apr 03, 2020 at 04:45:47PM +1100, Oliver O'Halloran wrote:
> On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> > 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?
> 

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
  2020-04-08  6:21     ` Sam Bobroff
@ 2020-04-08  6:53       ` Oliver O'Halloran
  2020-04-15  6:44         ` Sam Bobroff
  0 siblings, 1 reply; 14+ messages in thread
From: Oliver O'Halloran @ 2020-04-08  6:53 UTC (permalink / raw)
  To: Sam Bobroff; +Cc: linuxppc-dev

On Wed, Apr 8, 2020 at 4:22 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
>
> On Fri, Apr 03, 2020 at 05:08:32PM +1100, Oliver O'Halloran wrote:
> > On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> > > 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.
>
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
  2020-04-08  6:53       ` Oliver O'Halloran
@ 2020-04-15  6:44         ` Sam Bobroff
  0 siblings, 0 replies; 14+ messages in thread
From: Sam Bobroff @ 2020-04-15  6:44 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

On Wed, Apr 08, 2020 at 04:53:36PM +1000, Oliver O'Halloran wrote:
> On Wed, Apr 8, 2020 at 4:22 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
> >
> > On Fri, Apr 03, 2020 at 05:08:32PM +1100, Oliver O'Halloran wrote:
> > > On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> > > > 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.
> >
> > 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.

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().)

> 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.

> Oliver

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-04-15  6:47 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30  4:56 [PATCH 0/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
2020-03-30  4:56 ` [PATCH 1/4] powerpc/eeh: fix pseries_eeh_configure_bridge() Sam Bobroff
2020-04-03  4:19   ` Oliver O'Halloran
2020-03-30  4:56 ` [PATCH 2/4] powerpc/eeh: Release EEH device state synchronously Sam Bobroff
2020-04-03  4:51   ` Oliver O'Halloran
2020-04-08  6:15     ` Sam Bobroff
2020-03-30  4:56 ` [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late() Sam Bobroff
2020-04-03  6:08   ` Oliver O'Halloran
2020-04-08  6:21     ` Sam Bobroff
2020-04-08  6:53       ` Oliver O'Halloran
2020-04-15  6:44         ` Sam Bobroff
2020-03-30  4:56 ` [PATCH 4/4] powerpc/eeh: Clean up edev cleanup for VFs Sam Bobroff
2020-04-03  5:45   ` Oliver O'Halloran
2020-04-08  6:33     ` Sam Bobroff

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.