linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* Misc EEH fixes
@ 2019-07-15  8:56 Oliver O'Halloran
  2019-07-15  8:56 ` [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges Oliver O'Halloran
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Oliver O'Halloran @ 2019-07-15  8:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: sbobroff

Fixes of no particular importance. I just wanted to cut down the pile of
patches I've got hanging around.



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

* [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges
  2019-07-15  8:56 Misc EEH fixes Oliver O'Halloran
@ 2019-07-15  8:56 ` Oliver O'Halloran
  2019-07-16  3:53   ` Sam Bobroff
  2020-01-29  5:17   ` Michael Ellerman
  2019-07-15  8:56 ` [PATCH 2/5] powerpc/eeh_sysfs: Fix incorrect comment Oliver O'Halloran
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 12+ messages in thread
From: Oliver O'Halloran @ 2019-07-15  8:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: sbobroff, Oliver O'Halloran

At the point where we start inserting ranges into the EEH address cache the
binding between pci_dev and eeh_dev has already been set up. Instead of
consulting the pci_dn tree we can retrieve the eeh_dev directly using
pci_dev_to_eeh_dev().

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/kernel/eeh_cache.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
index 3204723..908ba69 100644
--- a/arch/powerpc/kernel/eeh_cache.c
+++ b/arch/powerpc/kernel/eeh_cache.c
@@ -156,18 +156,10 @@ eeh_addr_cache_insert(struct pci_dev *dev, resource_size_t alo,
 
 static void __eeh_addr_cache_insert_dev(struct pci_dev *dev)
 {
-	struct pci_dn *pdn;
 	struct eeh_dev *edev;
 	int i;
 
-	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
-	if (!pdn) {
-		pr_warn("PCI: no pci dn found for dev=%s\n",
-			pci_name(dev));
-		return;
-	}
-
-	edev = pdn_to_eeh_dev(pdn);
+	edev = pci_dev_to_eeh_dev(dev);
 	if (!edev) {
 		pr_warn("PCI: no EEH dev found for %s\n",
 			pci_name(dev));
-- 
2.9.5


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

* [PATCH 2/5] powerpc/eeh_sysfs: Fix incorrect comment
  2019-07-15  8:56 Misc EEH fixes Oliver O'Halloran
  2019-07-15  8:56 ` [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges Oliver O'Halloran
@ 2019-07-15  8:56 ` Oliver O'Halloran
  2019-07-16  3:54   ` Sam Bobroff
  2019-07-15  8:56 ` [PATCH 3/5] powerpc/eeh_sysfs: ifdef pseries sr-iov sysfs properties Oliver O'Halloran
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Oliver O'Halloran @ 2019-07-15  8:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: sbobroff, Oliver O'Halloran

The EEH_ATTR_SHOW() helper is used to display fields from struct eeh_dev
not struct pci_dn.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/kernel/eeh_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
index 3fa04dd..6a2c2886f 100644
--- a/arch/powerpc/kernel/eeh_sysfs.c
+++ b/arch/powerpc/kernel/eeh_sysfs.c
@@ -30,7 +30,7 @@
 /**
  * EEH_SHOW_ATTR -- Create sysfs entry for eeh statistic
  * @_name: name of file in sysfs directory
- * @_memb: name of member in struct pci_dn to access
+ * @_memb: name of member in struct eeh_dev to access
  * @_format: printf format for display
  *
  * All of the attributes look very similar, so just
-- 
2.9.5


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

* [PATCH 3/5] powerpc/eeh_sysfs: ifdef pseries sr-iov sysfs properties
  2019-07-15  8:56 Misc EEH fixes Oliver O'Halloran
  2019-07-15  8:56 ` [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges Oliver O'Halloran
  2019-07-15  8:56 ` [PATCH 2/5] powerpc/eeh_sysfs: Fix incorrect comment Oliver O'Halloran
@ 2019-07-15  8:56 ` Oliver O'Halloran
  2019-07-16  3:54   ` Sam Bobroff
  2019-07-15  8:56 ` [PATCH 4/5] powerpc/eeh_sysfs: Remove double pci_dn lookup Oliver O'Halloran
  2019-07-15  8:56 ` [PATCH 5/5] powerpc/eeh_sysfs: Make clearing EEH_DEV_SYSFS saner Oliver O'Halloran
  4 siblings, 1 reply; 12+ messages in thread
From: Oliver O'Halloran @ 2019-07-15  8:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: sbobroff, Oliver O'Halloran

There are several EEH sysfs properties that only exists when the
"ibm,is-open-sriov-pf" property appears in the device tree node of the PCI
device. This used on pseries to indicate to the guest that the hypervisor
allows the guest to configure the SR-IOV capability. Doing this requires
some handshaking between the guest, hypervisor and userspace when a VF is
EEH frozen which is why these properties exist.

This is all dead code on non-pseries platforms so wrap it in an #ifdef
CONFIG_PPC_PSERIES to make the dependency clearer.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/kernel/eeh_sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
index 6a2c2886f..3adf8cd 100644
--- a/arch/powerpc/kernel/eeh_sysfs.c
+++ b/arch/powerpc/kernel/eeh_sysfs.c
@@ -91,7 +91,7 @@ static ssize_t eeh_pe_state_store(struct device *dev,
 
 static DEVICE_ATTR_RW(eeh_pe_state);
 
-#ifdef CONFIG_PCI_IOV
+#if defined(CONFIG_PCI_IOV) && defined(CONFIG_PPC_PSERIES)
 static ssize_t eeh_notify_resume_show(struct device *dev,
 				      struct device_attribute *attr, char *buf)
 {
@@ -148,7 +148,7 @@ static void eeh_notify_resume_remove(struct pci_dev *pdev)
 #else
 static inline int eeh_notify_resume_add(struct pci_dev *pdev) { return 0; }
 static inline void eeh_notify_resume_remove(struct pci_dev *pdev) { }
-#endif /* CONFIG_PCI_IOV */
+#endif /* CONFIG_PCI_IOV && CONFIG PPC_PSERIES*/
 
 void eeh_sysfs_add_device(struct pci_dev *pdev)
 {
-- 
2.9.5


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

* [PATCH 4/5] powerpc/eeh_sysfs: Remove double pci_dn lookup.
  2019-07-15  8:56 Misc EEH fixes Oliver O'Halloran
                   ` (2 preceding siblings ...)
  2019-07-15  8:56 ` [PATCH 3/5] powerpc/eeh_sysfs: ifdef pseries sr-iov sysfs properties Oliver O'Halloran
@ 2019-07-15  8:56 ` Oliver O'Halloran
  2019-07-16  3:55   ` Sam Bobroff
  2019-07-15  8:56 ` [PATCH 5/5] powerpc/eeh_sysfs: Make clearing EEH_DEV_SYSFS saner Oliver O'Halloran
  4 siblings, 1 reply; 12+ messages in thread
From: Oliver O'Halloran @ 2019-07-15  8:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: sbobroff, Oliver O'Halloran

In eeh_notify_resume_show() the pci_dn for the device is looked up once in
the declaration block and then once after checking for a NULL eeh_dev.
Remove the second lookup since it's pointless.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/kernel/eeh_sysfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
index 3adf8cd..c4cc8fc 100644
--- a/arch/powerpc/kernel/eeh_sysfs.c
+++ b/arch/powerpc/kernel/eeh_sysfs.c
@@ -102,7 +102,6 @@ static ssize_t eeh_notify_resume_show(struct device *dev,
 	if (!edev || !edev->pe)
 		return -ENODEV;
 
-	pdn = pci_get_pdn(pdev);
 	return sprintf(buf, "%d\n", pdn->last_allow_rc);
 }
 
-- 
2.9.5


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

* [PATCH 5/5] powerpc/eeh_sysfs: Make clearing EEH_DEV_SYSFS saner
  2019-07-15  8:56 Misc EEH fixes Oliver O'Halloran
                   ` (3 preceding siblings ...)
  2019-07-15  8:56 ` [PATCH 4/5] powerpc/eeh_sysfs: Remove double pci_dn lookup Oliver O'Halloran
@ 2019-07-15  8:56 ` Oliver O'Halloran
  2019-07-16  4:00   ` Sam Bobroff
  4 siblings, 1 reply; 12+ messages in thread
From: Oliver O'Halloran @ 2019-07-15  8:56 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: sbobroff, Oliver O'Halloran

The eeh_sysfs_remove_device() function is supposed to clear the
EEH_DEV_SYSFS flag since it indicates the EEH sysfs entries have been added
for a pci_dev.

When the sysfs files are removed eeh_remove_device() the eeh_dev and the
pci_dev have already been de-associated. This then causes the
pci_dev_to_eeh_dev() call in eeh_sysfs_remove_device() to return NULL so
the flag can't be cleared from the still-live eeh_dev. This problem is
worked around in the caller by clearing the flag manually. However, this
behaviour doesn't make a whole lot of sense, so this patch fixes it by:

a) Re-ordering eeh_remove_device() so that eeh_sysfs_remove_device() is
   called before de-associating the pci_dev and eeh_dev.

b) Making eeh_sysfs_remove_device() emit a warning if there's no
   corresponding eeh_dev for a pci_dev. The paths where the sysfs
   files are only reachable if EEH was setup for the device
   for the device in the first place so hitting this warning
   indicates a programming error.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/kernel/eeh.c       | 30 +++++++++++++++++-------------
 arch/powerpc/kernel/eeh_sysfs.c | 15 ++++++++-------
 2 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index f192d57..6e24896 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1203,7 +1203,6 @@ void eeh_add_device_late(struct pci_dev *dev)
 		eeh_rmv_from_parent_pe(edev);
 		eeh_addr_cache_rmv_dev(edev->pdev);
 		eeh_sysfs_remove_device(edev->pdev);
-		edev->mode &= ~EEH_DEV_SYSFS;
 
 		/*
 		 * We definitely should have the PCI device removed
@@ -1306,17 +1305,11 @@ void eeh_remove_device(struct pci_dev *dev)
 	edev->pdev = NULL;
 
 	/*
-	 * The flag "in_error" is used to trace EEH devices for VFs
-	 * in error state or not. It's set in eeh_report_error(). If
-	 * it's not set, eeh_report_{reset,resume}() won't be called
-	 * for the VF EEH device.
+	 * eeh_sysfs_remove_device() uses pci_dev_to_eeh_dev() so we need to
+	 * remove the sysfs files before clearing dev.archdata.edev
 	 */
-	edev->in_error = false;
-	dev->dev.archdata.edev = NULL;
-	if (!(edev->pe->state & EEH_PE_KEEP))
-		eeh_rmv_from_parent_pe(edev);
-	else
-		edev->mode |= EEH_DEV_DISCONNECTED;
+	if (edev->mode & EEH_DEV_SYSFS)
+		eeh_sysfs_remove_device(dev);
 
 	/*
 	 * We're removing from the PCI subsystem, that means
@@ -1327,8 +1320,19 @@ void eeh_remove_device(struct pci_dev *dev)
 	edev->mode |= EEH_DEV_NO_HANDLER;
 
 	eeh_addr_cache_rmv_dev(dev);
-	eeh_sysfs_remove_device(dev);
-	edev->mode &= ~EEH_DEV_SYSFS;
+
+	/*
+	 * The flag "in_error" is used to trace EEH devices for VFs
+	 * in error state or not. It's set in eeh_report_error(). If
+	 * it's not set, eeh_report_{reset,resume}() won't be called
+	 * for the VF EEH device.
+	 */
+	edev->in_error = false;
+	dev->dev.archdata.edev = NULL;
+	if (!(edev->pe->state & EEH_PE_KEEP))
+		eeh_rmv_from_parent_pe(edev);
+	else
+		edev->mode |= EEH_DEV_DISCONNECTED;
 }
 
 int eeh_unfreeze_pe(struct eeh_pe *pe)
diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
index c4cc8fc..5614fd83 100644
--- a/arch/powerpc/kernel/eeh_sysfs.c
+++ b/arch/powerpc/kernel/eeh_sysfs.c
@@ -175,22 +175,23 @@ void eeh_sysfs_remove_device(struct pci_dev *pdev)
 {
 	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
 
+	if (!edev) {
+		WARN_ON(eeh_enabled());
+		return;
+	}
+
+	edev->mode &= ~EEH_DEV_SYSFS;
+
 	/*
 	 * The parent directory might have been removed. We needn't
 	 * continue for that case.
 	 */
-	if (!pdev->dev.kobj.sd) {
-		if (edev)
-			edev->mode &= ~EEH_DEV_SYSFS;
+	if (!pdev->dev.kobj.sd)
 		return;
-	}
 
 	device_remove_file(&pdev->dev, &dev_attr_eeh_mode);
 	device_remove_file(&pdev->dev, &dev_attr_eeh_pe_config_addr);
 	device_remove_file(&pdev->dev, &dev_attr_eeh_pe_state);
 
 	eeh_notify_resume_remove(pdev);
-
-	if (edev)
-		edev->mode &= ~EEH_DEV_SYSFS;
 }
-- 
2.9.5


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

* Re: [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges
  2019-07-15  8:56 ` [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges Oliver O'Halloran
@ 2019-07-16  3:53   ` Sam Bobroff
  2020-01-29  5:17   ` Michael Ellerman
  1 sibling, 0 replies; 12+ messages in thread
From: Sam Bobroff @ 2019-07-16  3:53 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

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

On Mon, Jul 15, 2019 at 06:56:08PM +1000, Oliver O'Halloran wrote:
> At the point where we start inserting ranges into the EEH address cache the
> binding between pci_dev and eeh_dev has already been set up. Instead of
> consulting the pci_dn tree we can retrieve the eeh_dev directly using
> pci_dev_to_eeh_dev().
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

In fact the binding between pci_dev and eeh_dev is set up right before
the calls to eeh_addr_cache_insert_dev() (see eeh_addr_cache_build() and
eeh_add_device_tree_late()) so this looks clearly correct to me.

A few simple tests of EEH recovery still succeed as well.

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>

> ---
>  arch/powerpc/kernel/eeh_cache.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
> index 3204723..908ba69 100644
> --- a/arch/powerpc/kernel/eeh_cache.c
> +++ b/arch/powerpc/kernel/eeh_cache.c
> @@ -156,18 +156,10 @@ eeh_addr_cache_insert(struct pci_dev *dev, resource_size_t alo,
>  
>  static void __eeh_addr_cache_insert_dev(struct pci_dev *dev)
>  {
> -	struct pci_dn *pdn;
>  	struct eeh_dev *edev;
>  	int i;
>  
> -	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> -	if (!pdn) {
> -		pr_warn("PCI: no pci dn found for dev=%s\n",
> -			pci_name(dev));
> -		return;
> -	}
> -
> -	edev = pdn_to_eeh_dev(pdn);
> +	edev = pci_dev_to_eeh_dev(dev);
>  	if (!edev) {
>  		pr_warn("PCI: no EEH dev found for %s\n",
>  			pci_name(dev));
> -- 
> 2.9.5
> 

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

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

* Re: [PATCH 2/5] powerpc/eeh_sysfs: Fix incorrect comment
  2019-07-15  8:56 ` [PATCH 2/5] powerpc/eeh_sysfs: Fix incorrect comment Oliver O'Halloran
@ 2019-07-16  3:54   ` Sam Bobroff
  0 siblings, 0 replies; 12+ messages in thread
From: Sam Bobroff @ 2019-07-16  3:54 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

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

On Mon, Jul 15, 2019 at 06:56:09PM +1000, Oliver O'Halloran wrote:
> The EEH_ATTR_SHOW() helper is used to display fields from struct eeh_dev
> not struct pci_dn.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Good.

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>

> ---
>  arch/powerpc/kernel/eeh_sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index 3fa04dd..6a2c2886f 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -30,7 +30,7 @@
>  /**
>   * EEH_SHOW_ATTR -- Create sysfs entry for eeh statistic
>   * @_name: name of file in sysfs directory
> - * @_memb: name of member in struct pci_dn to access
> + * @_memb: name of member in struct eeh_dev to access
>   * @_format: printf format for display
>   *
>   * All of the attributes look very similar, so just
> -- 
> 2.9.5
> 

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

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

* Re: [PATCH 3/5] powerpc/eeh_sysfs: ifdef pseries sr-iov sysfs properties
  2019-07-15  8:56 ` [PATCH 3/5] powerpc/eeh_sysfs: ifdef pseries sr-iov sysfs properties Oliver O'Halloran
@ 2019-07-16  3:54   ` Sam Bobroff
  0 siblings, 0 replies; 12+ messages in thread
From: Sam Bobroff @ 2019-07-16  3:54 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

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

On Mon, Jul 15, 2019 at 06:56:10PM +1000, Oliver O'Halloran wrote:
> There are several EEH sysfs properties that only exists when the
> "ibm,is-open-sriov-pf" property appears in the device tree node of the PCI
> device. This used on pseries to indicate to the guest that the hypervisor
> allows the guest to configure the SR-IOV capability. Doing this requires
> some handshaking between the guest, hypervisor and userspace when a VF is
> EEH frozen which is why these properties exist.
> 
> This is all dead code on non-pseries platforms so wrap it in an #ifdef
> CONFIG_PPC_PSERIES to make the dependency clearer.

Looks good and a test compile without pSeries platform support
configured seemed to work, so:

Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>

> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  arch/powerpc/kernel/eeh_sysfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index 6a2c2886f..3adf8cd 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -91,7 +91,7 @@ static ssize_t eeh_pe_state_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(eeh_pe_state);
>  
> -#ifdef CONFIG_PCI_IOV
> +#if defined(CONFIG_PCI_IOV) && defined(CONFIG_PPC_PSERIES)
>  static ssize_t eeh_notify_resume_show(struct device *dev,
>  				      struct device_attribute *attr, char *buf)
>  {
> @@ -148,7 +148,7 @@ static void eeh_notify_resume_remove(struct pci_dev *pdev)
>  #else
>  static inline int eeh_notify_resume_add(struct pci_dev *pdev) { return 0; }
>  static inline void eeh_notify_resume_remove(struct pci_dev *pdev) { }
> -#endif /* CONFIG_PCI_IOV */
> +#endif /* CONFIG_PCI_IOV && CONFIG PPC_PSERIES*/
>  
>  void eeh_sysfs_add_device(struct pci_dev *pdev)
>  {
> -- 
> 2.9.5
> 

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

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

* Re: [PATCH 4/5] powerpc/eeh_sysfs: Remove double pci_dn lookup.
  2019-07-15  8:56 ` [PATCH 4/5] powerpc/eeh_sysfs: Remove double pci_dn lookup Oliver O'Halloran
@ 2019-07-16  3:55   ` Sam Bobroff
  0 siblings, 0 replies; 12+ messages in thread
From: Sam Bobroff @ 2019-07-16  3:55 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

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

On Mon, Jul 15, 2019 at 06:56:11PM +1000, Oliver O'Halloran wrote:
> In eeh_notify_resume_show() the pci_dn for the device is looked up once in
> the declaration block and then once after checking for a NULL eeh_dev.
> Remove the second lookup since it's pointless.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>

> ---
>  arch/powerpc/kernel/eeh_sysfs.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index 3adf8cd..c4cc8fc 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -102,7 +102,6 @@ static ssize_t eeh_notify_resume_show(struct device *dev,
>  	if (!edev || !edev->pe)
>  		return -ENODEV;
>  
> -	pdn = pci_get_pdn(pdev);
>  	return sprintf(buf, "%d\n", pdn->last_allow_rc);
>  }
>  
> -- 
> 2.9.5
> 

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

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

* Re: [PATCH 5/5] powerpc/eeh_sysfs: Make clearing EEH_DEV_SYSFS saner
  2019-07-15  8:56 ` [PATCH 5/5] powerpc/eeh_sysfs: Make clearing EEH_DEV_SYSFS saner Oliver O'Halloran
@ 2019-07-16  4:00   ` Sam Bobroff
  0 siblings, 0 replies; 12+ messages in thread
From: Sam Bobroff @ 2019-07-16  4:00 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linuxppc-dev

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

On Mon, Jul 15, 2019 at 06:56:12PM +1000, Oliver O'Halloran wrote:
> The eeh_sysfs_remove_device() function is supposed to clear the
> EEH_DEV_SYSFS flag since it indicates the EEH sysfs entries have been added
> for a pci_dev.
> 
> When the sysfs files are removed eeh_remove_device() the eeh_dev and the
> pci_dev have already been de-associated. This then causes the
> pci_dev_to_eeh_dev() call in eeh_sysfs_remove_device() to return NULL so
> the flag can't be cleared from the still-live eeh_dev. This problem is
> worked around in the caller by clearing the flag manually. However, this
> behaviour doesn't make a whole lot of sense, so this patch fixes it by:
> 
> a) Re-ordering eeh_remove_device() so that eeh_sysfs_remove_device() is
>    called before de-associating the pci_dev and eeh_dev.
> 
> b) Making eeh_sysfs_remove_device() emit a warning if there's no
>    corresponding eeh_dev for a pci_dev. The paths where the sysfs
>    files are only reachable if EEH was setup for the device
>    for the device in the first place so hitting this warning
>    indicates a programming error.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Good cleanup, although it looks like "for the device" got duplicated in
the last part of the commit message.

Simple EEH tests still succeeed.

Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
Tested-by: Sam Bobroff <sbobroff@linux.ibm.com>

> +	if (!(edev->pe->state & EEH_PE_KEEP))
> +		eeh_rmv_from_parent_pe(edev);
> +	else
> +		edev->mode |= EEH_DEV_DISCONNECTED;

> ---
>  arch/powerpc/kernel/eeh.c       | 30 +++++++++++++++++-------------
>  arch/powerpc/kernel/eeh_sysfs.c | 15 ++++++++-------
>  2 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index f192d57..6e24896 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -1203,7 +1203,6 @@ void eeh_add_device_late(struct pci_dev *dev)
>  		eeh_rmv_from_parent_pe(edev);
>  		eeh_addr_cache_rmv_dev(edev->pdev);
>  		eeh_sysfs_remove_device(edev->pdev);
> -		edev->mode &= ~EEH_DEV_SYSFS;
>  
>  		/*
>  		 * We definitely should have the PCI device removed
> @@ -1306,17 +1305,11 @@ void eeh_remove_device(struct pci_dev *dev)
>  	edev->pdev = NULL;
>  
>  	/*
> -	 * The flag "in_error" is used to trace EEH devices for VFs
> -	 * in error state or not. It's set in eeh_report_error(). If
> -	 * it's not set, eeh_report_{reset,resume}() won't be called
> -	 * for the VF EEH device.
> +	 * eeh_sysfs_remove_device() uses pci_dev_to_eeh_dev() so we need to
> +	 * remove the sysfs files before clearing dev.archdata.edev
>  	 */
> -	edev->in_error = false;
> -	dev->dev.archdata.edev = NULL;
> -	if (!(edev->pe->state & EEH_PE_KEEP))
> -		eeh_rmv_from_parent_pe(edev);
> -	else
> -		edev->mode |= EEH_DEV_DISCONNECTED;
> +	if (edev->mode & EEH_DEV_SYSFS)
> +		eeh_sysfs_remove_device(dev);
>  
>  	/*
>  	 * We're removing from the PCI subsystem, that means
> @@ -1327,8 +1320,19 @@ void eeh_remove_device(struct pci_dev *dev)
>  	edev->mode |= EEH_DEV_NO_HANDLER;
>  
>  	eeh_addr_cache_rmv_dev(dev);
> -	eeh_sysfs_remove_device(dev);
> -	edev->mode &= ~EEH_DEV_SYSFS;
> +
> +	/*
> +	 * The flag "in_error" is used to trace EEH devices for VFs
> +	 * in error state or not. It's set in eeh_report_error(). If
> +	 * it's not set, eeh_report_{reset,resume}() won't be called
> +	 * for the VF EEH device.
> +	 */
> +	edev->in_error = false;
> +	dev->dev.archdata.edev = NULL;
> +	if (!(edev->pe->state & EEH_PE_KEEP))
> +		eeh_rmv_from_parent_pe(edev);
> +	else
> +		edev->mode |= EEH_DEV_DISCONNECTED;
>  }
>  
>  int eeh_unfreeze_pe(struct eeh_pe *pe)
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index c4cc8fc..5614fd83 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -175,22 +175,23 @@ void eeh_sysfs_remove_device(struct pci_dev *pdev)
>  {
>  	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>  
> +	if (!edev) {
> +		WARN_ON(eeh_enabled());
> +		return;
> +	}
> +
> +	edev->mode &= ~EEH_DEV_SYSFS;
> +
>  	/*
>  	 * The parent directory might have been removed. We needn't
>  	 * continue for that case.
>  	 */
> -	if (!pdev->dev.kobj.sd) {
> -		if (edev)
> -			edev->mode &= ~EEH_DEV_SYSFS;
> +	if (!pdev->dev.kobj.sd)
>  		return;
> -	}
>  
>  	device_remove_file(&pdev->dev, &dev_attr_eeh_mode);
>  	device_remove_file(&pdev->dev, &dev_attr_eeh_pe_config_addr);
>  	device_remove_file(&pdev->dev, &dev_attr_eeh_pe_state);
>  
>  	eeh_notify_resume_remove(pdev);
> -
> -	if (edev)
> -		edev->mode &= ~EEH_DEV_SYSFS;
>  }
> -- 
> 2.9.5
> 

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

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

* Re: [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges
  2019-07-15  8:56 ` [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges Oliver O'Halloran
  2019-07-16  3:53   ` Sam Bobroff
@ 2020-01-29  5:17   ` Michael Ellerman
  1 sibling, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2020-01-29  5:17 UTC (permalink / raw)
  To: Oliver O'Halloran, linuxppc-dev; +Cc: sbobroff, Oliver O'Halloran

On Mon, 2019-07-15 at 08:56:08 UTC, Oliver O'Halloran wrote:
> At the point where we start inserting ranges into the EEH address cache the
> binding between pci_dev and eeh_dev has already been set up. Instead of
> consulting the pci_dn tree we can retrieve the eeh_dev directly using
> pci_dev_to_eeh_dev().
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/b1268f4cdba71c0cd40b533778812340d36de8ae

cheers

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

end of thread, other threads:[~2020-01-29  5:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-15  8:56 Misc EEH fixes Oliver O'Halloran
2019-07-15  8:56 ` [PATCH 1/5] powerpc/eeh_cache: Don't use pci_dn when inserting new ranges Oliver O'Halloran
2019-07-16  3:53   ` Sam Bobroff
2020-01-29  5:17   ` Michael Ellerman
2019-07-15  8:56 ` [PATCH 2/5] powerpc/eeh_sysfs: Fix incorrect comment Oliver O'Halloran
2019-07-16  3:54   ` Sam Bobroff
2019-07-15  8:56 ` [PATCH 3/5] powerpc/eeh_sysfs: ifdef pseries sr-iov sysfs properties Oliver O'Halloran
2019-07-16  3:54   ` Sam Bobroff
2019-07-15  8:56 ` [PATCH 4/5] powerpc/eeh_sysfs: Remove double pci_dn lookup Oliver O'Halloran
2019-07-16  3:55   ` Sam Bobroff
2019-07-15  8:56 ` [PATCH 5/5] powerpc/eeh_sysfs: Make clearing EEH_DEV_SYSFS saner Oliver O'Halloran
2019-07-16  4:00   ` Sam Bobroff

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).