| Message ID | 20190208030802.10805-3-oohall@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/7] powerpc/eeh: Use debugfs_create_u32 for eeh_max_freezes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 2 checks, 62 lines checked |
Hi Oliver,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.0-rc4 next-20190207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Oliver-O-Halloran/powerpc-eeh-Use-debugfs_create_u32-for-eeh_max_freezes/20190208-145918
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=powerpc
Note: the linux-review/Oliver-O-Halloran/powerpc-eeh-Use-debugfs_create_u32-for-eeh_max_freezes/20190208-145918 HEAD a8dcd44575537e3e67a44fe3139b273a64c0f620 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
>> arch/powerpc/kernel/eeh.c:1840: error: unterminated #ifdef
#ifdef CONFIG_DEBUG_FS
vim +1840 arch/powerpc/kernel/eeh.c
7f52a526f arch/powerpc/kernel/eeh.c Gavin Shan 2014-04-24 1835
^1da177e4 arch/ppc64/kernel/eeh.c Linus Torvalds 2005-04-16 1836 static int __init eeh_init_proc(void)
^1da177e4 arch/ppc64/kernel/eeh.c Linus Torvalds 2005-04-16 1837 {
7f52a526f arch/powerpc/kernel/eeh.c Gavin Shan 2014-04-24 1838 if (machine_is(pseries) || machine_is(powernv)) {
3f3942aca arch/powerpc/kernel/eeh.c Christoph Hellwig 2018-05-15 1839 proc_create_single("powerpc/eeh", 0, NULL, proc_eeh_show);
7f52a526f arch/powerpc/kernel/eeh.c Gavin Shan 2014-04-24 @1840 #ifdef CONFIG_DEBUG_FS
:::::: The code at line 1840 was first introduced by commit
:::::: 7f52a526f64c69c913f0027fbf43821ff0b3a7d7 powerpc/eeh: Allow to disable EEH
:::::: TO: Gavin Shan <gwshan@linux.vnet.ibm.com>
:::::: CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Oliver O'Halloran <oohall@gmail.com> writes: > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > index f6e65375a8de..d1f0bdf41fac 100644 > --- a/arch/powerpc/kernel/eeh.c > +++ b/arch/powerpc/kernel/eeh.c > @@ -1810,7 +1810,7 @@ static int __init eeh_init_proc(void) > &eeh_enable_dbgfs_ops); > debugfs_create_u32("eeh_max_freezes", 0600, > powerpc_debugfs_root, &eeh_max_freezes); > -#endif > + eeh_cache_debugfs_init(); Oops :) > diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c > index b2c320e0fcef..dba421a577e7 100644 > --- a/arch/powerpc/kernel/eeh_cache.c > +++ b/arch/powerpc/kernel/eeh_cache.c > @@ -298,9 +299,34 @@ void eeh_addr_cache_build(void) > eeh_addr_cache_insert_dev(dev); > eeh_sysfs_add_device(dev); > } > +} > > -#ifdef DEBUG > - /* Verify tree built up above, echo back the list of addrs. */ > - eeh_addr_cache_print(&pci_io_addr_cache_root); > -#endif > +static int eeh_addr_cache_show(struct seq_file *s, void *v) > +{ > + struct rb_node *n = rb_first(&pci_io_addr_cache_root.rb_root); > + struct pci_io_addr_range *piar; > + int cnt = 0; > + > + spin_lock(&pci_io_addr_cache_root.piar_lock); > + while (n) { > + piar = rb_entry(n, struct pci_io_addr_range, rb_node); > + > + seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n", > + (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", cnt, > + &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev)); > + > + n = rb_next(n); > + cnt++; > + } You can write that as a for loop can't you? struct rb_node *n; int i = 0; for (n = rb_first(&pci_io_addr_cache_root.rb_root); n; n = rb_next(n), i++) { piar = rb_entry(n, struct pci_io_addr_range, rb_node); seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n", (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", i, &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev)); } cheers
On Fri, Feb 8, 2019 at 8:47 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > Oliver O'Halloran <oohall@gmail.com> writes: > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > > index f6e65375a8de..d1f0bdf41fac 100644 > > --- a/arch/powerpc/kernel/eeh.c > > +++ b/arch/powerpc/kernel/eeh.c > > @@ -1810,7 +1810,7 @@ static int __init eeh_init_proc(void) > > &eeh_enable_dbgfs_ops); > > debugfs_create_u32("eeh_max_freezes", 0600, > > powerpc_debugfs_root, &eeh_max_freezes); > > -#endif > > + eeh_cache_debugfs_init(); > > Oops :) Yeah :( > > diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c > > index b2c320e0fcef..dba421a577e7 100644 > > --- a/arch/powerpc/kernel/eeh_cache.c > > +++ b/arch/powerpc/kernel/eeh_cache.c > > @@ -298,9 +299,34 @@ void eeh_addr_cache_build(void) > > eeh_addr_cache_insert_dev(dev); > > eeh_sysfs_add_device(dev); > > } > > +} > > > > -#ifdef DEBUG > > - /* Verify tree built up above, echo back the list of addrs. */ > > - eeh_addr_cache_print(&pci_io_addr_cache_root); > > -#endif > > +static int eeh_addr_cache_show(struct seq_file *s, void *v) > > +{ > > + struct rb_node *n = rb_first(&pci_io_addr_cache_root.rb_root); > > + struct pci_io_addr_range *piar; > > + int cnt = 0; > > + > > + spin_lock(&pci_io_addr_cache_root.piar_lock); > > + while (n) { > > + piar = rb_entry(n, struct pci_io_addr_range, rb_node); > > + > > + seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n", > > + (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", cnt, > > + &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev)); > > + > > + n = rb_next(n); > > + cnt++; > > + } > > You can write that as a for loop can't you? > > struct rb_node *n; > int i = 0; > > for (n = rb_first(&pci_io_addr_cache_root.rb_root); n; n = rb_next(n), i++) { IIRC I did try that, but it's too long. 85 cols wide according to my editor. > piar = rb_entry(n, struct pci_io_addr_range, rb_node); > > seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n", > (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", i, > &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev)); > } > > cheers
Oliver <oohall@gmail.com> writes: > On Fri, Feb 8, 2019 at 8:47 PM Michael Ellerman <mpe@ellerman.id.au> wrote: >> Oliver O'Halloran <oohall@gmail.com> writes: >> > diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c >> > index b2c320e0fcef..dba421a577e7 100644 >> > --- a/arch/powerpc/kernel/eeh_cache.c >> > +++ b/arch/powerpc/kernel/eeh_cache.c >> > @@ -298,9 +299,34 @@ void eeh_addr_cache_build(void) >> > eeh_addr_cache_insert_dev(dev); >> > eeh_sysfs_add_device(dev); >> > } >> > +} >> > >> > -#ifdef DEBUG >> > - /* Verify tree built up above, echo back the list of addrs. */ >> > - eeh_addr_cache_print(&pci_io_addr_cache_root); >> > -#endif >> > +static int eeh_addr_cache_show(struct seq_file *s, void *v) >> > +{ >> > + struct rb_node *n = rb_first(&pci_io_addr_cache_root.rb_root); >> > + struct pci_io_addr_range *piar; >> > + int cnt = 0; >> > + >> > + spin_lock(&pci_io_addr_cache_root.piar_lock); >> > + while (n) { >> > + piar = rb_entry(n, struct pci_io_addr_range, rb_node); >> > + >> > + seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n", >> > + (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", cnt, >> > + &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev)); >> > + >> > + n = rb_next(n); >> > + cnt++; >> > + } >> >> You can write that as a for loop can't you? >> >> struct rb_node *n; >> int i = 0; >> >> for (n = rb_first(&pci_io_addr_cache_root.rb_root); n; n = rb_next(n), i++) { > > IIRC I did try that, but it's too long. 85 cols wide according to my editor. Don't care. Long lines aren't inherently evil, they have some downsides but so do the other options. In a case like this 85 columns would be preferable to splitting the line or writing it a while loop. cheers
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index c003628441cc..fc21b6e78e91 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -460,6 +460,9 @@ static inline void eeh_readsl(const volatile void __iomem *addr, void * buf, eeh_check_failure(addr); } + +void eeh_cache_debugfs_init(void); + #endif /* CONFIG_PPC64 */ #endif /* __KERNEL__ */ #endif /* _POWERPC_EEH_H */ diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c index f6e65375a8de..d1f0bdf41fac 100644 --- a/arch/powerpc/kernel/eeh.c +++ b/arch/powerpc/kernel/eeh.c @@ -1810,7 +1810,7 @@ static int __init eeh_init_proc(void) &eeh_enable_dbgfs_ops); debugfs_create_u32("eeh_max_freezes", 0600, powerpc_debugfs_root, &eeh_max_freezes); -#endif + eeh_cache_debugfs_init(); } return 0; diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c index b2c320e0fcef..dba421a577e7 100644 --- a/arch/powerpc/kernel/eeh_cache.c +++ b/arch/powerpc/kernel/eeh_cache.c @@ -26,6 +26,7 @@ #include <linux/spinlock.h> #include <linux/atomic.h> #include <asm/pci-bridge.h> +#include <asm/debugfs.h> #include <asm/ppc-pci.h> @@ -298,9 +299,34 @@ void eeh_addr_cache_build(void) eeh_addr_cache_insert_dev(dev); eeh_sysfs_add_device(dev); } +} -#ifdef DEBUG - /* Verify tree built up above, echo back the list of addrs. */ - eeh_addr_cache_print(&pci_io_addr_cache_root); -#endif +static int eeh_addr_cache_show(struct seq_file *s, void *v) +{ + struct rb_node *n = rb_first(&pci_io_addr_cache_root.rb_root); + struct pci_io_addr_range *piar; + int cnt = 0; + + spin_lock(&pci_io_addr_cache_root.piar_lock); + while (n) { + piar = rb_entry(n, struct pci_io_addr_range, rb_node); + + seq_printf(s, "%s addr range %3d [%pap-%pap]: %s\n", + (piar->flags & IORESOURCE_IO) ? "i/o" : "mem", cnt, + &piar->addr_lo, &piar->addr_hi, pci_name(piar->pcidev)); + + n = rb_next(n); + cnt++; + } + spin_unlock(&pci_io_addr_cache_root.piar_lock); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(eeh_addr_cache); + +void eeh_cache_debugfs_init(void) +{ + debugfs_create_file_unsafe("eeh_address_cache", 0400, + powerpc_debugfs_root, NULL, + &eeh_addr_cache_fops); }
Adds a debugfs file that can be read to view the contents of the EEH address cache. This is pretty similar to the existing eeh_addr_cache_print() function, but that function is intended to debug issues inside of the kernel since it's #ifdef`ed out by default, and writes into the kernel log. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- arch/powerpc/include/asm/eeh.h | 3 +++ arch/powerpc/kernel/eeh.c | 2 +- arch/powerpc/kernel/eeh_cache.c | 34 +++++++++++++++++++++++++++++---- 3 files changed, 34 insertions(+), 5 deletions(-)