Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Christian Kujau]

On Tue, 20 Apr 2010 at 05:19, Rafael J. Wysocki wrote:
> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=15589
> Subject		: 2.6.34-rc1: Badness at fs/proc/generic.c:316
> Submitter	: Christian Kujau <lists@nerdbynature.de>
> Date		: 2010-03-13 23:53 (38 days old)
> Message-ID	: <alpine.DEB.2.01.1003131544340.5493@bogon.housecafe.de>
> References	: http://marc.info/?l=linux-kernel&m=126852442903680&w=2

Yes, unless something in this area has changed from -rc4 to -rc5, this is 
still printed during boot:


device-tree: Duplicate name in /cpus/PowerPC,G4@0, renamed to "l2-cache#1"
name 'pulses/rev'
------------[ cut here ]------------
Badness at fs/proc/generic.c:317
NIP: c00e14b8 LR: c00e14b8 CTR: c01fc2c0
REGS: c045bdc0 TRAP: 0700   Not tainted  (2.6.34-rc4)
MSR: 00029032 <EE,ME,CE,IR,DR>  CR: 22000022  XER: 20000000
TASK = c043b410[0] 'swapper' THREAD: c045a000
GPR00: c00e14b8 c045be70 c043b410 00000024 000012ff ffffffff ffffffff 00000000 
GPR08: ef808320 c0458670 00000000 000012ff 42000028 00000000 00cb3ccc 00cb39b8 
GPR16: 00cd682c 00cb3ca8 00cb38c8 00cb39ac 00240e18 00240e20 00cb3954 00240e24 
GPR24: 00000000 0049b000 c045be98 c045bec8 c0da0a42 c0da0a42 00000006 00000000 
NIP [c00e14b8] __xlate_proc_name+0xd0/0xf8
LR [c00e14b8] __xlate_proc_name+0xd0/0xf8
Call Trace:
[c045be70] [c00e14b8] __xlate_proc_name+0xd0/0xf8 (unreliable)
[c045be90] [c00e1a2c] __proc_create+0x60/0xf0
[c045bec0] [c00e2194] proc_create_data+0x54/0xc4
[c045bee0] [c00e6310] __proc_device_tree_add_prop+0x64/0xd4
[c045bf00] [c00e64b4] proc_device_tree_add_node+0x134/0x164
[c045bf20] [c00e6434] proc_device_tree_add_node+0xb4/0x164
[c045bf40] [c00e6434] proc_device_tree_add_node+0xb4/0x164
[c045bf60] [c00e6434] proc_device_tree_add_node+0xb4/0x164
[c045bf80] [c00e6434] proc_device_tree_add_node+0xb4/0x164
[c045bfa0] [c0421c30] proc_device_tree_init+0x4c/0x78
[c045bfb0] [c0421698] proc_root_init+0xcc/0xf0
[c045bfc0] [c040e798] start_kernel+0x230/0x284
[c045bff0] [00003444] 0x3444
Instruction dump:
93ba0000 38600000 93fb0000 80010024 bb410008 38210020 7c0803a6 4e800020 
3c60c03c 7f84e378 386300a8 48273a45 <0fe00000> 80010024 3860fffe bb410008 


-- 
BOFH excuse #37:

heavy gravity fluctuation, move computer to floor rapidly

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Michael Ellerman]

On Mon, 2010-04-19 at 23:45 -0700, Christian Kujau wrote:
> On Tue, 20 Apr 2010 at 05:19, Rafael J. Wysocki wrote:
> > Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=15589
> > Subject		: 2.6.34-rc1: Badness at fs/proc/generic.c:316
> > Submitter	: Christian Kujau <lists@nerdbynature.de>
> > Date		: 2010-03-13 23:53 (38 days old)
> > Message-ID	: <alpine.DEB.2.01.1003131544340.5493@bogon.housecafe.de>
> > References	: http://marc.info/?l=linux-kernel&m=126852442903680&w=2
> 
> Yes, unless something in this area has changed from -rc4 to -rc5, this is 
> still printed during boot:
> 
> 
> device-tree: Duplicate name in /cpus/PowerPC,G4@0, renamed to "l2-cache#1"
> name 'pulses/rev'
> ------------[ cut here ]------------

Don't cut here, sigh.

> Badness at fs/proc/generic.c:317

Try this 100% unbuilt, 100% untested patch.

cheers


diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index f8650dc..9502b48 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -175,6 +175,24 @@ retry:
        return fixed_name;
 }
 
+static const char *unslash_name(const char *name)
+{
+       char *p, *fixed_name;
+
+       fixed_name = kstrdup(name);
+       if (!fixed_name) {
+               printk(KERN_ERR "device-tree: Out of memory trying to unslash "
+                               "name \"%s\"\n", name);
+               return name;
+       }
+
+       p = fixed_name;
+       while ((p = strstr(p, "/")))
+               *p++ = '_';
+
+       return fixed_name;
+}
+
 /*
  * Process a node, adding entries for its children and its properties.
  */
@@ -211,6 +229,9 @@ void proc_device_tree_add_node(struct device_node *np,
                if (duplicate_name(de, p))
                        p = fixup_name(np, de, p);
 
+               if (strstr(p, "/"))
+                       p = unslash_name(p);
+
                ent = __proc_device_tree_add_prop(de, pp, p);
                if (ent == NULL)
                        break;

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Christian Kujau]

On Tue, 20 Apr 2010 at 22:18, Michael Ellerman wrote:
> Try this 100% unbuilt, 100% untested patch.

I added GFP_KERNEL to kstrdup to make the compile error go away:

fs/proc/proc_devtree.c: In function =E2=80=98unslash_name=E2=80=99:
fs/proc/proc_devtree.c:183: error: too few arguments to function =E2=80=98k=
strdup=E2=80=99
make[2]: *** [fs/proc/proc_devtree.o] Error 1
make[1]: *** [fs/proc] Error 2
make: *** [fs] Error 2

And now 2.6.34-rc5 compiles and boots without the warning. Thanks!=20
New dmesg and /proc/device-tree on:

  http://nerdbynature.de/bits/2.6.34-rc1/xlate_proc_name/

Alexey mentioned that this is "wasteful" - does it make the kernel slower?=
=20
I have not done any performance tests, but I'd rather stick with the=20
warning than make this Powerbook G4 any more slower :-\


Thanks again,
Christian.

diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index ce94801..019581d 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -176,6 +176,24 @@ retry:
 =09return fixed_name;
 }
=20
+static const char *unslash_name(const char *name)
+{
+=09char *p, *fixed_name;
+
+=09fixed_name =3D kstrdup(name, GFP_KERNEL);
+=09if (!fixed_name) {
+=09=09printk(KERN_ERR "device-tree: Out of memory trying to unslash "
+=09=09=09=09"name \"%s\"\n", name);
+=09return name;
+=09}
+
+=09p =3D fixed_name;
+=09while ((p =3D strstr(p, "/")))
+=09=09*p++ =3D '_';
+
+=09return fixed_name;
+}
+
 /*
  * Process a node, adding entries for its children and its properties.
  */
@@ -212,6 +230,9 @@ void proc_device_tree_add_node(struct device_node *np,
 =09=09if (duplicate_name(de, p))
 =09=09=09p =3D fixup_name(np, de, p);
=20
+=09=09if (strstr(p, "/"))
+=09=09=09p =3D unslash_name(p);
+
 =09=09ent =3D __proc_device_tree_add_prop(de, pp, p);
 =09=09if (ent =3D=3D NULL)
 =09=09=09break;
--=20
BOFH excuse #369:

Virus transmitted from computer to sysadmins.

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Andreas Schwab]

Christian Kujau <lists@nerdbynature.de> writes:

> +	while ((p = strstr(p, "/")))

You want to use strchr.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Michael Ellerman]

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

On Tue, 2010-04-20 at 11:15 -0700, Christian Kujau wrote:
> On Tue, 20 Apr 2010 at 22:18, Michael Ellerman wrote:
> > Try this 100% unbuilt, 100% untested patch.
> 
> I added GFP_KERNEL to kstrdup to make the compile error go away:
> 
> fs/proc/proc_devtree.c: In function ‘unslash_name’:
> fs/proc/proc_devtree.c:183: error: too few arguments to function ‘kstrdup’
> make[2]: *** [fs/proc/proc_devtree.o] Error 1
> make[1]: *** [fs/proc] Error 2
> make: *** [fs] Error 2

Yeah oops, told you I hadn't built it.

> And now 2.6.34-rc5 compiles and boots without the warning. Thanks! 
> New dmesg and /proc/device-tree on:
> 
>   http://nerdbynature.de/bits/2.6.34-rc1/xlate_proc_name/

Cool, and we see:

./uni-n@f8000000/i2c@f8001000/i2c-bus@1/fan@5c/pulses_rev

Which is the one that needed fixing.

> Alexey mentioned that this is "wasteful" - does it make the kernel slower? 
> I have not done any performance tests, but I'd rather stick with the 
> warning than make this Powerbook G4 any more slower :-\

Maybe a little. It has to check every string to see if it contains a
"/". But then you save the cost of taking an exeception for the WARN,
which might make up the difference. But it's a one time fixup at boot,
so it's not going to be noticeable.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Alexey Dobriyan]

On Tue, Apr 20, 2010 at 10:18:18PM +1000, Michael Ellerman wrote:
> On Mon, 2010-04-19 at 23:45 -0700, Christian Kujau wrote:
> --- a/fs/proc/proc_devtree.c
> +++ b/fs/proc/proc_devtree.c
> @@ -175,6 +175,24 @@ retry:
>         return fixed_name;
>  }
>  
> +static const char *unslash_name(const char *name)
> +{
> +       char *p, *fixed_name;
> +
> +       fixed_name = kstrdup(name);
> +       if (!fixed_name) {
> +               printk(KERN_ERR "device-tree: Out of memory trying to unslash "
> +                               "name \"%s\"\n", name);
> +               return name;
> +       }
> +
> +       p = fixed_name;
> +       while ((p = strstr(p, "/")))
> +               *p++ = '_';

This is wasteful. :-)
Also, I hope we won't spit message every time allocation fail.

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Michael Ellerman]

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

On Wed, 2010-04-21 at 18:55 +0300, Alexey Dobriyan wrote:
> On Tue, Apr 20, 2010 at 10:18:18PM +1000, Michael Ellerman wrote:
> > On Mon, 2010-04-19 at 23:45 -0700, Christian Kujau wrote:
> > --- a/fs/proc/proc_devtree.c
> > +++ b/fs/proc/proc_devtree.c
> > @@ -175,6 +175,24 @@ retry:
> >         return fixed_name;
> >  }
> >  
> > +static const char *unslash_name(const char *name)
> > +{
> > +       char *p, *fixed_name;
> > +
> > +       fixed_name = kstrdup(name);
> > +       if (!fixed_name) {
> > +               printk(KERN_ERR "device-tree: Out of memory trying to unslash "
> > +                               "name \"%s\"\n", name);
> > +               return name;
> > +       }
> > +
> > +       p = fixed_name;
> > +       while ((p = strstr(p, "/")))
> > +               *p++ = '_';
> 
> This is wasteful. :-)

Whatever, patches welcome :)

> Also, I hope we won't spit message every time allocation fail.

We do. Your system is mostly hosed anyway, but feel free to rate limit
it or something.

The error handling in there is a bit dubious, if the alloc fails we just
return the old name, which we know is bogus. It should probably return
NULL and the calling code can check - same for fixup_name().

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Rafael J. Wysocki]

On Wednesday 21 April 2010, Michael Ellerman wrote:
> On Wed, 2010-04-21 at 18:55 +0300, Alexey Dobriyan wrote:
> > On Tue, Apr 20, 2010 at 10:18:18PM +1000, Michael Ellerman wrote:
> > > On Mon, 2010-04-19 at 23:45 -0700, Christian Kujau wrote:
> > > --- a/fs/proc/proc_devtree.c
> > > +++ b/fs/proc/proc_devtree.c
> > > @@ -175,6 +175,24 @@ retry:
> > >         return fixed_name;
> > >  }
> > >  
> > > +static const char *unslash_name(const char *name)
> > > +{
> > > +       char *p, *fixed_name;
> > > +
> > > +       fixed_name = kstrdup(name);
> > > +       if (!fixed_name) {
> > > +               printk(KERN_ERR "device-tree: Out of memory trying to unslash "
> > > +                               "name \"%s\"\n", name);
> > > +               return name;
> > > +       }
> > > +
> > > +       p = fixed_name;
> > > +       while ((p = strstr(p, "/")))
> > > +               *p++ = '_';
> > 
> > This is wasteful. :-)
> 
> Whatever, patches welcome :)
> 
> > Also, I hope we won't spit message every time allocation fail.
> 
> We do. Your system is mostly hosed anyway, but feel free to rate limit
> it or something.

OK

Is anyone going to post a clean patch for that with a sign-off?

Rafael

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Christian Kujau]

On Sun, 9 May 2010 at 23:17, Rafael J. Wysocki wrote:
> Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=15589
> Subject		: 2.6.34-rc1: Badness at fs/proc/generic.c:316
> Submitter	: Christian Kujau <lists@nerdbynature.de>
> Date		: 2010-03-13 23:53 (58 days old)
> Message-ID	: <<alpine.DEB.2.01.1003131544340.5493@bogon.housecafe.de>>
> References	: http://marc.info/?l=linux-kernel&m=126852442903680&w=2

The bug is still present in -rc6, but Michael Ellerman has a patch[0] 
which made the warning go away.

@Michael: will you post your patch with a Sign-Off, so that it can be 
          pushed into mainline?

Thanks,
Christian.

[0] http://patchwork.ozlabs.org/patch/50557/

diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
index ce94801..019581d 100644
--- a/fs/proc/proc_devtree.c
+++ b/fs/proc/proc_devtree.c
@@ -176,6 +176,24 @@ retry:
 	return fixed_name;
 }
 
+static const char *unslash_name(const char *name)
+{
+	char *p, *fixed_name;
+
+	fixed_name = kstrdup(name, GFP_KERNEL);
+	if (!fixed_name) {
+		printk(KERN_ERR "device-tree: Out of memory trying to unslash "
+				"name \"%s\"\n", name);
+	return name;
+	}
+
+	p = fixed_name;
+	while ((p = strstr(p, "/")))
+		*p++ = '_';
+
+	return fixed_name;
+}
+
 /*
  * Process a node, adding entries for its children and its properties.
  */
@@ -212,6 +230,9 @@ void proc_device_tree_add_node(struct device_node *np,
 		if (duplicate_name(de, p))
 			p = fixup_name(np, de, p);
 
+		if (strstr(p, "/"))
+			p = unslash_name(p);
+
 		ent = __proc_device_tree_add_prop(de, pp, p);
 		if (ent == NULL)
 			break;
-- 
BOFH excuse #188:

..disk or the processor is on fire.

Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316

[Michael Ellerman]

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

On Sun, 2010-05-09 at 15:27 -0700, Christian Kujau wrote:
> On Sun, 9 May 2010 at 23:17, Rafael J. Wysocki wrote:
> > Bug-Entry	: http://bugzilla.kernel.org/show_bug.cgi?id=15589
> > Subject		: 2.6.34-rc1: Badness at fs/proc/generic.c:316
> > Submitter	: Christian Kujau <lists@nerdbynature.de>
> > Date		: 2010-03-13 23:53 (58 days old)
> > Message-ID	: <<alpine.DEB.2.01.1003131544340.5493@bogon.housecafe.de>>
> > References	: http://marc.info/?l=linux-kernel&m=126852442903680&w=2
> 
> The bug is still present in -rc6, but Michael Ellerman has a patch[0] 
> which made the warning go away.
> 
> @Michael: will you post your patch with a Sign-Off, so that it can be 
>           pushed into mainline?

No, Benh and I decided it's better to just drop those properties all
together. But it's too late in this cycle for a patch like that just to
fix a warning - so we'll do that patch for 35.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]