* [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
* 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 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
* [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
* 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
* [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
* 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
* [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
* 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
* [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 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