| Message ID | ad8cf11e3151fd39c6a9deedc0098d50274c75e4.1553050609.git.sbobroff@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (de3c83c2fd2b87cf68214eda76dfa66989d78cb6) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 43 lines checked |
On 20/03/2019 13:58, Sam Bobroff wrote: > Move the EEH enabled message into it's own function so that future > work can call it from multiple places. > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > --- > arch/powerpc/include/asm/eeh.h | 3 +++ > arch/powerpc/kernel/eeh.c | 16 +++++++++++----- > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > index fe4cf7208890..e217ccda55d0 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -289,6 +289,7 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe); > > struct eeh_dev *eeh_dev_init(struct pci_dn *pdn); > void eeh_dev_phb_init_dynamic(struct pci_controller *phb); > +void eeh_show_enabled(void); > void eeh_probe_devices(void); > int __init eeh_ops_register(struct eeh_ops *ops); > int __exit eeh_ops_unregister(const char *name); > @@ -338,6 +339,8 @@ static inline bool eeh_enabled(void) > return false; > } > > +static inline void eeh_show_enabled(void) { } > + > static inline bool eeh_phb_enabled(void) > { > return false; > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > index b14d89547895..3dcff29cb9b3 100644 > --- a/arch/powerpc/kernel/eeh.c > +++ b/arch/powerpc/kernel/eeh.c > @@ -163,6 +163,16 @@ static int __init eeh_setup(char *str) > } > __setup("eeh=", eeh_setup); > > +void eeh_show_enabled(void) > +{ > + if (eeh_has_flag(EEH_FORCE_DISABLED)) > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (by eeh=off)\n"); > + else if (eeh_enabled()) I'd make it eeh_has_flag(EEH_ENABLED) for clarity. > + pr_info("EEH: PCI Enhanced I/O Error Handling ENABLED (capable adapter found)\n"); > + else > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (no capable adapter found)\n"); > +} > + > /* > * This routine captures assorted PCI configuration space data > * for the indicated PCI device, and puts them into a buffer > @@ -1166,11 +1176,7 @@ void eeh_probe_devices(void) > pdn = hose->pci_data; > traverse_pci_dn(pdn, eeh_ops->probe, NULL); > } > - if (eeh_enabled()) > - pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n"); > - else > - pr_info("EEH: No capable adapters found\n"); > - > + eeh_show_enabled(); This line moves later in the series so I'd just merge this patch into 8/8 to reduce number of lines moving withing the patchset. In general the whole point of the EEH_ENABLED flag is fading away. Its meaning now is that "at least somewhere in the box for at least one device with enabled EEH" which does not seem extremely useful as we have a pci_dev or pe pretty much everywhere we look at eeh_enabled() and pdev->dev.archdata.edev can tell if eeh is enabled for a device. Although I am pretty sure this is in your list already :) > } > > /** >
On Wed, Mar 20, 2019 at 5:06 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > > On 20/03/2019 13:58, Sam Bobroff wrote: > > Move the EEH enabled message into it's own function so that future > > work can call it from multiple places. > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > > --- > > arch/powerpc/include/asm/eeh.h | 3 +++ > > arch/powerpc/kernel/eeh.c | 16 +++++++++++----- > > 2 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > > index fe4cf7208890..e217ccda55d0 100644 > > --- a/arch/powerpc/include/asm/eeh.h > > +++ b/arch/powerpc/include/asm/eeh.h > > @@ -289,6 +289,7 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe); > > > > struct eeh_dev *eeh_dev_init(struct pci_dn *pdn); > > void eeh_dev_phb_init_dynamic(struct pci_controller *phb); > > +void eeh_show_enabled(void); > > void eeh_probe_devices(void); > > int __init eeh_ops_register(struct eeh_ops *ops); > > int __exit eeh_ops_unregister(const char *name); > > @@ -338,6 +339,8 @@ static inline bool eeh_enabled(void) > > return false; > > } > > > > +static inline void eeh_show_enabled(void) { } > > + > > static inline bool eeh_phb_enabled(void) > > { > > return false; > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > > index b14d89547895..3dcff29cb9b3 100644 > > --- a/arch/powerpc/kernel/eeh.c > > +++ b/arch/powerpc/kernel/eeh.c > > @@ -163,6 +163,16 @@ static int __init eeh_setup(char *str) > > } > > __setup("eeh=", eeh_setup); > > > > +void eeh_show_enabled(void) > > +{ > > + if (eeh_has_flag(EEH_FORCE_DISABLED)) > > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (by eeh=off)\n"); > > + else if (eeh_enabled()) > > > I'd make it eeh_has_flag(EEH_ENABLED) for clarity. > > > > + pr_info("EEH: PCI Enhanced I/O Error Handling ENABLED (capable adapter found)\n"); > > + else > > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (no capable adapter found)\n"); > > +} > > + > > /* > > * This routine captures assorted PCI configuration space data > > * for the indicated PCI device, and puts them into a buffer > > @@ -1166,11 +1176,7 @@ void eeh_probe_devices(void) > > pdn = hose->pci_data; > > traverse_pci_dn(pdn, eeh_ops->probe, NULL); > > } > > - if (eeh_enabled()) > > - pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n"); > > - else > > - pr_info("EEH: No capable adapters found\n"); > > - > > + eeh_show_enabled(); > > > This line moves later in the series so I'd just merge this patch into > 8/8 to reduce number of lines moving withing the patchset. > > In general the whole point of the EEH_ENABLED flag is fading away. Its > meaning now is that "at least somewhere in the box for at least one > device with enabled EEH" which does not seem extremely useful as we have > a pci_dev or pe pretty much everywhere we look at eeh_enabled() and > pdev->dev.archdata.edev can tell if eeh is enabled for a device. > Although I am pretty sure this is in your list already :) The other function is to disable attempting to detect EEH errors when we get 0xFFs from an MMIO load, but I don't think anyone ever disables it. > > } > > > > /** > > > > -- > Alexey
On Wed, Mar 20, 2019 at 05:02:23PM +1100, Alexey Kardashevskiy wrote: > > > On 20/03/2019 13:58, Sam Bobroff wrote: > > Move the EEH enabled message into it's own function so that future > > work can call it from multiple places. > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > > --- > > arch/powerpc/include/asm/eeh.h | 3 +++ > > arch/powerpc/kernel/eeh.c | 16 +++++++++++----- > > 2 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > > index fe4cf7208890..e217ccda55d0 100644 > > --- a/arch/powerpc/include/asm/eeh.h > > +++ b/arch/powerpc/include/asm/eeh.h > > @@ -289,6 +289,7 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe); > > > > struct eeh_dev *eeh_dev_init(struct pci_dn *pdn); > > void eeh_dev_phb_init_dynamic(struct pci_controller *phb); > > +void eeh_show_enabled(void); > > void eeh_probe_devices(void); > > int __init eeh_ops_register(struct eeh_ops *ops); > > int __exit eeh_ops_unregister(const char *name); > > @@ -338,6 +339,8 @@ static inline bool eeh_enabled(void) > > return false; > > } > > > > +static inline void eeh_show_enabled(void) { } > > + > > static inline bool eeh_phb_enabled(void) > > { > > return false; > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > > index b14d89547895..3dcff29cb9b3 100644 > > --- a/arch/powerpc/kernel/eeh.c > > +++ b/arch/powerpc/kernel/eeh.c > > @@ -163,6 +163,16 @@ static int __init eeh_setup(char *str) > > } > > __setup("eeh=", eeh_setup); > > > > +void eeh_show_enabled(void) > > +{ > > + if (eeh_has_flag(EEH_FORCE_DISABLED)) > > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (by eeh=off)\n"); > > + else if (eeh_enabled()) > > > I'd make it eeh_has_flag(EEH_ENABLED) for clarity. OK, sounds good. > > > + pr_info("EEH: PCI Enhanced I/O Error Handling ENABLED (capable adapter found)\n"); > > + else > > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (no capable adapter found)\n"); > > +} > > + > > /* > > * This routine captures assorted PCI configuration space data > > * for the indicated PCI device, and puts them into a buffer > > @@ -1166,11 +1176,7 @@ void eeh_probe_devices(void) > > pdn = hose->pci_data; > > traverse_pci_dn(pdn, eeh_ops->probe, NULL); > > } > > - if (eeh_enabled()) > > - pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n"); > > - else > > - pr_info("EEH: No capable adapters found\n"); > > - > > + eeh_show_enabled(); > > > This line moves later in the series so I'd just merge this patch into > 8/8 to reduce number of lines moving withing the patchset. Oh, good idea. I'll do it. > In general the whole point of the EEH_ENABLED flag is fading away. Its > meaning now is that "at least somewhere in the box for at least one > device with enabled EEH" which does not seem extremely useful as we have > a pci_dev or pe pretty much everywhere we look at eeh_enabled() and > pdev->dev.archdata.edev can tell if eeh is enabled for a device. > Although I am pretty sure this is in your list already :) Yes. :-) > > > } > > > > /** > > > > -- > Alexey >
On Wed, 2019-03-20 at 13:58 +1100, Sam Bobroff wrote: > Move the EEH enabled message into it's own function so that future > work can call it from multiple places. > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > --- > arch/powerpc/include/asm/eeh.h | 3 +++ > arch/powerpc/kernel/eeh.c | 16 +++++++++++----- > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > index fe4cf7208890..e217ccda55d0 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -289,6 +289,7 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe); > > struct eeh_dev *eeh_dev_init(struct pci_dn *pdn); > void eeh_dev_phb_init_dynamic(struct pci_controller *phb); > +void eeh_show_enabled(void); > void eeh_probe_devices(void); > int __init eeh_ops_register(struct eeh_ops *ops); > int __exit eeh_ops_unregister(const char *name); > @@ -338,6 +339,8 @@ static inline bool eeh_enabled(void) > return false; > } > > +static inline void eeh_show_enabled(void) { } > + > static inline bool eeh_phb_enabled(void) > { > return false; > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > index b14d89547895..3dcff29cb9b3 100644 > --- a/arch/powerpc/kernel/eeh.c > +++ b/arch/powerpc/kernel/eeh.c > @@ -163,6 +163,16 @@ static int __init eeh_setup(char *str) > } > __setup("eeh=", eeh_setup); > > +void eeh_show_enabled(void) > +{ > + if (eeh_has_flag(EEH_FORCE_DISABLED)) > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (by eeh=off)\n"); The allcaps looks kind of stupid. > + else if (eeh_enabled()) > + pr_info("EEH: PCI Enhanced I/O Error Handling ENABLED (capable adapter found)\n"); > + else > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (no capable adapter found)\n"); If we really have to keep these messages then make it say "no EEH capable buses found" or something. EEH support has absolutely nothing to do with the adapters and I'm not even sure we can get the "DISABLED" message on PowerNV since the root port will always be there. > +} > + > /* > * This routine captures assorted PCI configuration space data > * for the indicated PCI device, and puts them into a buffer > @@ -1166,11 +1176,7 @@ void eeh_probe_devices(void) > pdn = hose->pci_data; > traverse_pci_dn(pdn, eeh_ops->probe, NULL); > } > - if (eeh_enabled()) > - pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n"); > - else > - pr_info("EEH: No capable adapters found\n"); > - > + eeh_show_enabled(); > } > > /**
On Thu, Apr 18, 2019 at 08:01:36PM +1000, Oliver O'Halloran wrote: > On Wed, 2019-03-20 at 13:58 +1100, Sam Bobroff wrote: > > Move the EEH enabled message into it's own function so that future > > work can call it from multiple places. > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > > --- > > arch/powerpc/include/asm/eeh.h | 3 +++ > > arch/powerpc/kernel/eeh.c | 16 +++++++++++----- > > 2 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > > index fe4cf7208890..e217ccda55d0 100644 > > --- a/arch/powerpc/include/asm/eeh.h > > +++ b/arch/powerpc/include/asm/eeh.h > > @@ -289,6 +289,7 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe); > > > > struct eeh_dev *eeh_dev_init(struct pci_dn *pdn); > > void eeh_dev_phb_init_dynamic(struct pci_controller *phb); > > +void eeh_show_enabled(void); > > void eeh_probe_devices(void); > > int __init eeh_ops_register(struct eeh_ops *ops); > > int __exit eeh_ops_unregister(const char *name); > > @@ -338,6 +339,8 @@ static inline bool eeh_enabled(void) > > return false; > > } > > > > +static inline void eeh_show_enabled(void) { } > > + > > static inline bool eeh_phb_enabled(void) > > { > > return false; > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > > index b14d89547895..3dcff29cb9b3 100644 > > --- a/arch/powerpc/kernel/eeh.c > > +++ b/arch/powerpc/kernel/eeh.c > > @@ -163,6 +163,16 @@ static int __init eeh_setup(char *str) > > } > > __setup("eeh=", eeh_setup); > > > > +void eeh_show_enabled(void) > > +{ > > + if (eeh_has_flag(EEH_FORCE_DISABLED)) > > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (by eeh=off)\n"); > > The allcaps looks kind of stupid. Argh, true! > > + else if (eeh_enabled()) > > + pr_info("EEH: PCI Enhanced I/O Error Handling ENABLED (capable adapter found)\n"); > > + else > > + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (no capable adapter found)\n"); > > If we really have to keep these messages then make it say "no EEH > capable buses found" or something. EEH support has absolutely nothing I don't think it's critical, but I'd like something that lets you determine if EEH_ENABLED is set or not and why, since it's helpful for debugging. And I know what you mean about EEH support and adapters, but (before these patches anyway) if you don't have an EEH capable adapter in the machine at boot time, you won't ever get EEH recovery because EEH_ENABLED won't be set. (I think we want to clean that up, so this probably only matters until we do that.) > to do with the adapters and I'm not even sure we can get the "DISABLED" > message on PowerNV since the root port will always be there. Yes, you'd have to force it off via the command line. Anyway, I'll try to improve the messages (and no more caps) :-) > > +} > > + > > /* > > * This routine captures assorted PCI configuration space data > > * for the indicated PCI device, and puts them into a buffer > > @@ -1166,11 +1176,7 @@ void eeh_probe_devices(void) > > pdn = hose->pci_data; > > traverse_pci_dn(pdn, eeh_ops->probe, NULL); > > } > > - if (eeh_enabled()) > > - pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n"); > > - else > > - pr_info("EEH: No capable adapters found\n"); > > - > > + eeh_show_enabled(); > > } > > > > /** >
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index fe4cf7208890..e217ccda55d0 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -289,6 +289,7 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe); struct eeh_dev *eeh_dev_init(struct pci_dn *pdn); void eeh_dev_phb_init_dynamic(struct pci_controller *phb); +void eeh_show_enabled(void); void eeh_probe_devices(void); int __init eeh_ops_register(struct eeh_ops *ops); int __exit eeh_ops_unregister(const char *name); @@ -338,6 +339,8 @@ static inline bool eeh_enabled(void) return false; } +static inline void eeh_show_enabled(void) { } + static inline bool eeh_phb_enabled(void) { return false; diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c index b14d89547895..3dcff29cb9b3 100644 --- a/arch/powerpc/kernel/eeh.c +++ b/arch/powerpc/kernel/eeh.c @@ -163,6 +163,16 @@ static int __init eeh_setup(char *str) } __setup("eeh=", eeh_setup); +void eeh_show_enabled(void) +{ + if (eeh_has_flag(EEH_FORCE_DISABLED)) + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (by eeh=off)\n"); + else if (eeh_enabled()) + pr_info("EEH: PCI Enhanced I/O Error Handling ENABLED (capable adapter found)\n"); + else + pr_info("EEH: PCI Enhanced I/O Error Handling DISABLED (no capable adapter found)\n"); +} + /* * This routine captures assorted PCI configuration space data * for the indicated PCI device, and puts them into a buffer @@ -1166,11 +1176,7 @@ void eeh_probe_devices(void) pdn = hose->pci_data; traverse_pci_dn(pdn, eeh_ops->probe, NULL); } - if (eeh_enabled()) - pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n"); - else - pr_info("EEH: No capable adapters found\n"); - + eeh_show_enabled(); } /**
Move the EEH enabled message into it's own function so that future work can call it from multiple places. Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> --- arch/powerpc/include/asm/eeh.h | 3 +++ arch/powerpc/kernel/eeh.c | 16 +++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-)