[3/7] powerpc/eeh_cache: Add a way to dump the EEH address cache

Message ID 20190208030802.10805-3-oohall@gmail.com
State Changes Requested
Headers show
Series
  • [1/7] powerpc/eeh: Use debugfs_create_u32 for eeh_max_freezes
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch warning total: 0 errors, 0 warnings, 2 checks, 62 lines checked
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Oliver O'Halloran Feb. 8, 2019, 3:07 a.m.
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(-)

Comments

kbuild test robot Feb. 8, 2019, 9 a.m. | #1
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
Michael Ellerman Feb. 8, 2019, 9:47 a.m. | #2
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
Oliver O'Halloran Feb. 8, 2019, 1:14 p.m. | #3
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
Michael Ellerman Feb. 11, 2019, 2:16 a.m. | #4
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

Patch

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