[PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Larry Finger]

Commit a2db6842873c8e5a70652f278d469128cb52db70 changed the way that
sysfs attributes are handled. With lockdep checking enabled, a firmware
loading request from b43 generates the following BUG:

BUG: key ffff8800b85f4870 not in .data!
------------[ cut here ]------------
WARNING: at kernel/lockdep.c:2706 lockdep_init_map+0x236/0x5d0()
Hardware name: HP Pavilion dv2700 Notebook PC
Pid: 2283, comm: NetworkManager Not tainted 2.6.34-rc1-wl #320
Call Trace:
[<ffffffff81046068>] warn_slowpath_common+0x78/0xb0
[<ffffffff810460af>] warn_slowpath_null+0xf/0x20
[<ffffffff810765e6>] lockdep_init_map+0x236/0x5d0
[<ffffffff811633aa>] sysfs_add_file_mode+0x6a/0xc0
[<ffffffff8116340c>] sysfs_add_file+0xc/0x10
[<ffffffff81165ae1>] sysfs_create_bin_file+0x21/0x30
[<ffffffff8124f6b7>] _request_firmware+0x267/0x630
[<ffffffff8124fb0e>] request_firmware+0xe/0x10
[<ffffffffa04038d2>] b43_do_request_fw+0x92/0x230 [b43]
(rest of dump snipped)

Fixed by initing the attribute.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

Index: wireless-testing/drivers/base/firmware_class.c
===================================================================
--- wireless-testing.orig/drivers/base/firmware_class.c
+++ wireless-testing/drivers/base/firmware_class.c
@@ -442,6 +442,7 @@ static int fw_setup_device(struct firmwa
 	fw_priv = dev_get_drvdata(f_dev);
 
 	fw_priv->fw = fw;
+	sysfs_bin_attr_init(&fw_priv->attr_data);
 	retval = sysfs_create_bin_file(&f_dev->kobj, &fw_priv->attr_data);
 	if (retval) {
 		dev_err(device, "%s: sysfs_create_bin_file failed\n", __func__);

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Linus Torvalds]

On Sat, 13 Mar 2010, Larry Finger wrote:
>
> Commit a2db6842873c8e5a70652f278d469128cb52db70 changed the way that
> sysfs attributes are handled. With lockdep checking enabled, a firmware
> loading request from b43 generates the following BUG:
> 
> BUG: key ffff8800b85f4870 not in .data!
> ------------[ cut here ]------------
> WARNING: at kernel/lockdep.c:2706 lockdep_init_map+0x236/0x5d0()

I don't think we can fix these problems this way.

Lookie here:

	[torvalds@i5 linux]$ git grep sysfs_create_bin_file | wc
	     68     319    5999
	[torvalds@i5 linux]$ git grep sysfs_bin_attr_init | wc
	      8      24     522

and you sent in patches to fix _two_ of the remaining 60 cases.

Now, there may be some reason why it's not needed for the others, and 
those two are special, but I somewhat doubt it.

Eric - that patch of yours is obviously broken. Either I need to revert 
it, or you need to fix it. Not these kinds of "fix random 
sysfs_create_bin_file() drivers one by one" things.

		Linus

---
> Hardware name: HP Pavilion dv2700 Notebook PC
> Pid: 2283, comm: NetworkManager Not tainted 2.6.34-rc1-wl #320
> Call Trace:
> [<ffffffff81046068>] warn_slowpath_common+0x78/0xb0
> [<ffffffff810460af>] warn_slowpath_null+0xf/0x20
> [<ffffffff810765e6>] lockdep_init_map+0x236/0x5d0
> [<ffffffff811633aa>] sysfs_add_file_mode+0x6a/0xc0
> [<ffffffff8116340c>] sysfs_add_file+0xc/0x10
> [<ffffffff81165ae1>] sysfs_create_bin_file+0x21/0x30
> [<ffffffff8124f6b7>] _request_firmware+0x267/0x630
> [<ffffffff8124fb0e>] request_firmware+0xe/0x10
> [<ffffffffa04038d2>] b43_do_request_fw+0x92/0x230 [b43]
> (rest of dump snipped)
> 
> Fixed by initing the attribute.
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> 
> Index: wireless-testing/drivers/base/firmware_class.c
> ===================================================================
> --- wireless-testing.orig/drivers/base/firmware_class.c
> +++ wireless-testing/drivers/base/firmware_class.c
> @@ -442,6 +442,7 @@ static int fw_setup_device(struct firmwa
>  	fw_priv = dev_get_drvdata(f_dev);
>  
>  	fw_priv->fw = fw;
> +	sysfs_bin_attr_init(&fw_priv->attr_data);
>  	retval = sysfs_create_bin_file(&f_dev->kobj, &fw_priv->attr_data);
>  	if (retval) {
>  		dev_err(device, "%s: sysfs_create_bin_file failed\n", __func__);
> 

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Jiri Kosina]

On Sat, 13 Mar 2010, Linus Torvalds wrote:

> > Commit a2db6842873c8e5a70652f278d469128cb52db70 changed the way that
> > sysfs attributes are handled. With lockdep checking enabled, a firmware
> > loading request from b43 generates the following BUG:
> > 
> > BUG: key ffff8800b85f4870 not in .data!
> > ------------[ cut here ]------------
> > WARNING: at kernel/lockdep.c:2706 lockdep_init_map+0x236/0x5d0()
> 
> I don't think we can fix these problems this way.
> 
> Lookie here:
> 
> 	[torvalds@i5 linux]$ git grep sysfs_create_bin_file | wc
> 	     68     319    5999
> 	[torvalds@i5 linux]$ git grep sysfs_bin_attr_init | wc
> 	      8      24     522
> 
> and you sent in patches to fix _two_ of the remaining 60 cases.
>
> Now, there may be some reason why it's not needed for the others, and 
> those two are special, but I somewhat doubt it.

Well, the special initialization shouldn't be needed for all the 68 cases 
as far as I can see. It's needed only for dynamically allocated sysfs 
attributes, the static ones are fine.

> Eric - that patch of yours is obviously broken. Either I need to revert 
> it, or you need to fix it. Not these kinds of "fix random 
> sysfs_create_bin_file() drivers one by one" things.
>
> ---
> > Hardware name: HP Pavilion dv2700 Notebook PC
> > Pid: 2283, comm: NetworkManager Not tainted 2.6.34-rc1-wl #320
> > Call Trace:
> > [<ffffffff81046068>] warn_slowpath_common+0x78/0xb0
> > [<ffffffff810460af>] warn_slowpath_null+0xf/0x20
> > [<ffffffff810765e6>] lockdep_init_map+0x236/0x5d0
> > [<ffffffff811633aa>] sysfs_add_file_mode+0x6a/0xc0
> > [<ffffffff8116340c>] sysfs_add_file+0xc/0x10
> > [<ffffffff81165ae1>] sysfs_create_bin_file+0x21/0x30
> > [<ffffffff8124f6b7>] _request_firmware+0x267/0x630
> > [<ffffffff8124fb0e>] request_firmware+0xe/0x10
> > [<ffffffffa04038d2>] b43_do_request_fw+0x92/0x230 [b43]
> > (rest of dump snipped)
> > 
> > Fixed by initing the attribute.
> > 
> > Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> > ---
> > 
> > Index: wireless-testing/drivers/base/firmware_class.c
> > ===================================================================
> > --- wireless-testing.orig/drivers/base/firmware_class.c
> > +++ wireless-testing/drivers/base/firmware_class.c
> > @@ -442,6 +442,7 @@ static int fw_setup_device(struct firmwa
> >  	fw_priv = dev_get_drvdata(f_dev);
> >  
> >  	fw_priv->fw = fw;
> > +	sysfs_bin_attr_init(&fw_priv->attr_data);
> >  	retval = sysfs_create_bin_file(&f_dev->kobj, &fw_priv->attr_data);
> >  	if (retval) {
> >  		dev_err(device, "%s: sysfs_create_bin_file failed\n", __func__);
> > 

FWIW, I have submitted this exact patch to Greg a few days ago, and he 
should have it queued already.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Linus Torvalds]

On Sat, 13 Mar 2010, Jiri Kosina wrote:
> 
> Well, the special initialization shouldn't be needed for all the 68 cases 
> as far as I can see. It's needed only for dynamically allocated sysfs 
> attributes, the static ones are fine.

So does anybody have a 'grep' for it?

Because we sure as hell don't want to find them one by one, especially 
since this thing only shows up in a fairly rare configuration (most people 
do _not_ enable lockdep).

So that commit really _is_ broken. I'm seriously going to just revert it 
if the plan of action is nothing better than "wait for people to report 
yet another missed conversion". This is _not_ how we convert users.

			Linus

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Eric W. Biederman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sat, 13 Mar 2010, Jiri Kosina wrote:
>> 
>> Well, the special initialization shouldn't be needed for all the 68 cases 
>> as far as I can see. It's needed only for dynamically allocated sysfs 
>> attributes, the static ones are fine.
>
> So does anybody have a 'grep' for it?
>
> Because we sure as hell don't want to find them one by one, especially 
> since this thing only shows up in a fairly rare configuration (most people 
> do _not_ enable lockdep).

It also only affects those fairly rare lockdep users as well, and the only
affect is to throw a nasty warning message.  Isn't lockdep all about throwing
nasty warning messages?

> So that commit really _is_ broken. I'm seriously going to just revert it 
> if the plan of action is nothing better than "wait for people to report 
> yet another missed conversion". This is _not_ how we convert users.

Linus I think this can be the start of a new development tradition.
The annual birthday scolding from Linus.  I am certain we will all
appreciate the attention.

Unfortunately I have been extremely busy this last week and haven't
been able to do anything so you have only seen my backup plan in
effect where reasonable kernel developers notice the instances I
missed and on a case by case basis trivially fix it.  I believe Greg
has been queuing up many of those case by case fixes.  A few have been
flowing through other maintainers.

Wolfram Sang was figuring out if he could get coccinelle (whatever
that is) to find more of them.

It is my intention to go through all of the reports make certain everything
is getting fixed, and also to look through and see if I can find any more
cases that I have overlooked.

Eric

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Linus Torvalds]

On Sat, 13 Mar 2010, Eric W. Biederman wrote:
> 
> It also only affects those fairly rare lockdep users as well, and the only
> affect is to throw a nasty warning message.  Isn't lockdep all about throwing
> nasty warning messages?

Hmm. The report has that "BUG: " message in it (and in the subject line), 
but you're right - it ends up being just a warning, not actually a real 
BUG() (which is a machine killer).

So yeah - it's not as bad as I thought. Sorry.

[ And that "BUG:" in turn seems to be due to Ingo for some reason wanting 
  to confuse BUG_ON() messages (which have that "BUG: " prefix thing) with 
  whatever warning conditions he adds.

  Our warnings used to have that bug too (see commit 8f53b6fcc4: "Don't 
  call a warnign a bug. It's a warning.").

  Ingo: can we agree to not put "BUG: " messages in warnings, ok? It may 
  be a bug (lower-case) that triggers them, but that whole "BUG()" thing 
  has it's own semantics with rather more serious consequences than some 
  warning that lets things continue.

  So I - and I suspect others - react rather more strongly to "BUG:" than 
  to "WARNING:" or to just some regular innocuous message without the
  associations of the machine likely being dead as a result. ]

				Linus

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 13 Mar 2010, Eric W. Biederman wrote:
> > 
> > It also only affects those fairly rare lockdep users as well, and the only
> > affect is to throw a nasty warning message.  Isn't lockdep all about throwing
> > nasty warning messages?
> 
> Hmm. The report has that "BUG: " message in it (and in the subject line), 
> but you're right - it ends up being just a warning, not actually a real 
> BUG() (which is a machine killer).
> 
> So yeah - it's not as bad as I thought. Sorry.
> 
> [ And that "BUG:" in turn seems to be due to Ingo for some reason wanting 
>   to confuse BUG_ON() messages (which have that "BUG: " prefix thing) with 
>   whatever warning conditions he adds.
> 
>   Our warnings used to have that bug too (see commit 8f53b6fcc4: "Don't 
>   call a warnign a bug. It's a warning.").
> 
>   Ingo: can we agree to not put "BUG: " messages in warnings, ok? It may 
>   be a bug (lower-case) that triggers them, but that whole "BUG()" thing 
>   has it's own semantics with rather more serious consequences than some 
>   warning that lets things continue.

Sure - will change those too over to the "INFO: " pattern we've been using for 
some time. All new warnings that come via our trees use 'INFO: ', the 'BUG: ' 
ones are there for historic reasons.

There's a few that are external to lockdep and are likely fatal conditions:

	printk(  "[ BUG: bad unlock balance detected! ]\n");
	printk(  "[ BUG: bad contention detected! ]\n");
	printk(  "[ BUG: held lock freed! ]\n");
	printk(  "[ BUG: lock held at task exit time! ]\n");

(these things often tend to cause hangs/crashes later on.)

and then there's a few that are mostly internal to lockdep, and should never 
be fatal:

		printk("BUG: MAX_STACK_TRACE_ENTRIES too low!\n");
		printk("BUG: MAX_LOCKDEP_KEYS too low!\n");
		printk("BUG: MAX_LOCKDEP_ENTRIES too low!\n");
		printk("BUG: MAX_LOCKDEP_CHAINS too low!\n");
		printk("BUG: key %p not in .data!\n", key);
		printk("BUG: MAX_LOCKDEP_SUBCLASSES too low!\n");
		printk("BUG: MAX_LOCK_DEPTH too low!\n");

[ there's rare exceptions - i've seen 'BUG: key' + real crash on a few occasions,
  when the warning was caused by memory corruption. But typically the warning 
  is not fatal, and this is what matters to the severity of the message. ]

So i'm wondering whether we should/could keep those first four with a 'BUG: ' 
message, as lockdep wont crash the machine in the BUG() fashion. The other 7 
should definitely be less alarming messages.

	Ingo

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Linus Torvalds]

On Sun, 14 Mar 2010, Ingo Molnar wrote:
> > 
> >   Ingo: can we agree to not put "BUG: " messages in warnings, ok? It may 
> >   be a bug (lower-case) that triggers them, but that whole "BUG()" thing 
> >   has it's own semantics with rather more serious consequences than some 
> >   warning that lets things continue.
> 
> Sure - will change those too over to the "INFO: " pattern we've been using for 
> some time. All new warnings that come via our trees use 'INFO: ', the 'BUG: ' 
> ones are there for historic reasons.

Yeah, I assumed so. I just did a quick "git blame" to see where the code 
came from, I didn't delve any deeper.

> There's a few that are external to lockdep and are likely fatal conditions:
> 
> 	printk(  "[ BUG: bad unlock balance detected! ]\n");
> 	printk(  "[ BUG: bad contention detected! ]\n");
> 	printk(  "[ BUG: held lock freed! ]\n");
> 	printk(  "[ BUG: lock held at task exit time! ]\n");
> 
> (these things often tend to cause hangs/crashes later on.)
> 
> and then there's a few that are mostly internal to lockdep, and should never 
> be fatal:
> 
> 		printk("BUG: MAX_STACK_TRACE_ENTRIES too low!\n");
> 		printk("BUG: MAX_LOCKDEP_KEYS too low!\n");
> 		printk("BUG: MAX_LOCKDEP_ENTRIES too low!\n");
> 		printk("BUG: MAX_LOCKDEP_CHAINS too low!\n");
> 		printk("BUG: key %p not in .data!\n", key);
> 		printk("BUG: MAX_LOCKDEP_SUBCLASSES too low!\n");
> 		printk("BUG: MAX_LOCK_DEPTH too low!\n");
> 
> [ there's rare exceptions - i've seen 'BUG: key' + real crash on a few occasions,
>   when the warning was caused by memory corruption. But typically the warning 
>   is not fatal, and this is what matters to the severity of the message. ]
> 
> So i'm wondering whether we should/could keep those first four with a 'BUG: ' 
> message, as lockdep wont crash the machine in the BUG() fashion. The other 7 
> should definitely be less alarming messages.

At least my personal "mental expectation" is that BUG() implies that there 
was not even a try at recovering from the situation (ie our traditional 
"panic()" behavior), and that we didn't even continue. IOW, we actually 
terminated a process or effectively killed the machine.

If it's "just" a case of "something is wrong, but I'm just reporting it 
and continuing", then warning/info would be better. At least that's what 
my personal expectations are, and why I reacted so strongly to the whole 
BUG thing in this thread.

Btw, tangentially on a similar kind of "expectations of a debug message 
with call trace": I wonder if those things could be made to trigger all 
the fancy new automatic oops reporting.

The simplest thing to do would be to just replace _all_ of the printk + 
dump_stack with just "WARN_ON()", and then append the lockdep info later. 
At least then the fact that lockdep triggered would be noted by modern 
user space (and perhaps logged to kerneloops etc). 

A fancier thing might be to print the lockdep state _inside_ the whole 
"--- [ cut here ] ---" region, so that the lockdep stuff also gets logged, 
but I don't think we have the infrastructure to do that cleanly now (ie 
wa have that whole "warn_slowpath_*()" thing, but it allows for a single 
line printout format, not for a generic "print out debug info" function)

			Linus

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Greg KH]

On Sat, Mar 13, 2010 at 09:46:46PM -0800, Linus Torvalds wrote:
> 
> 
> On Sat, 13 Mar 2010, Eric W. Biederman wrote:
> > 
> > It also only affects those fairly rare lockdep users as well, and the only
> > affect is to throw a nasty warning message.  Isn't lockdep all about throwing
> > nasty warning messages?
> 
> Hmm. The report has that "BUG: " message in it (and in the subject line), 
> but you're right - it ends up being just a warning, not actually a real 
> BUG() (which is a machine killer).
> 
> So yeah - it's not as bad as I thought. Sorry.

It's a messy warning, I have patches that should fix the remaining ones
that I will send to you tomorrow.

thanks,

greg k-h

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Sat, Mar 13, 2010 at 09:46:46PM -0800, Linus Torvalds wrote:
>> 
>> 
>> On Sat, 13 Mar 2010, Eric W. Biederman wrote:
>> > 
>> > It also only affects those fairly rare lockdep users as well, and the only
>> > affect is to throw a nasty warning message.  Isn't lockdep all about throwing
>> > nasty warning messages?
>> 
>> Hmm. The report has that "BUG: " message in it (and in the subject line), 
>> but you're right - it ends up being just a warning, not actually a real 
>> BUG() (which is a machine killer).
>> 
>> So yeah - it's not as bad as I thought. Sorry.
>
> It's a messy warning, I have patches that should fix the remaining ones
> that I will send to you tomorrow.

Greg if you have it that is great.  I started looking through the emails
I received and it looks like you weren't copied on all of them.  At this
point I will double check after you send out your changes to Linus.  That
we have everything I have seen reported.

Eric

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Greg KH]

On Sun, Mar 14, 2010 at 11:30:48AM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Sat, Mar 13, 2010 at 09:46:46PM -0800, Linus Torvalds wrote:
> >> 
> >> 
> >> On Sat, 13 Mar 2010, Eric W. Biederman wrote:
> >> > 
> >> > It also only affects those fairly rare lockdep users as well, and the only
> >> > affect is to throw a nasty warning message.  Isn't lockdep all about throwing
> >> > nasty warning messages?
> >> 
> >> Hmm. The report has that "BUG: " message in it (and in the subject line), 
> >> but you're right - it ends up being just a warning, not actually a real 
> >> BUG() (which is a machine killer).
> >> 
> >> So yeah - it's not as bad as I thought. Sorry.
> >
> > It's a messy warning, I have patches that should fix the remaining ones
> > that I will send to you tomorrow.
> 
> Greg if you have it that is great.  I started looking through the emails
> I received and it looks like you weren't copied on all of them.  At this
> point I will double check after you send out your changes to Linus.  That
> we have everything I have seen reported.

Why not just forward them all to me, that way I know I have them all :)

thanks,

greg k-h

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Wolfram Sang]

Eric W. Biederman <ebiederm <at> xmission.com> writes:

> Wolfram Sang was figuring out if he could get coccinelle (whatever
> that is) to find more of them.

Huh, found this accidently, CC helps :)

coccinelle can apply semantic patches, rough description here [1]. So I wrote
one which checks for every c-file in the kernel-tree:

- Is there a structure containing (bin_)attribute?
- Does a function declare a pointer to such a structure?
- Is this pointer then used for a sysfs_create(_bin)_file?
- If so, has there been a call to sysfs(_bin)_attr_init before?

If not -> report. As coccinelle works on an abstract level of the code (not on
source-code level), it can follow code-paths and such. Really nice tool, once
you gathered the information to learn it.

The outcome are those patches:

http://patchwork.kernel.org/patch/85214/

http://patchwork.kernel.org/patch/85212/
(which is pretty much the same like the patch originating this thread. I vote
very much for dropping those patches and use
http://patchwork.kernel.org/patch/84814/ instead!)

http://patchwork.kernel.org/patch/85213/

Eric, if you have a bit of time, acking these patches would help them getting
accepted for the mips/rtc-trees, IMHO.

Regards,

   Wolfram

[1] http://lwn.net/Articles/315686/

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Linus Torvalds]

On Sun, 14 Mar 2010, Wolfram Sang wrote:
>
> coccinelle can apply semantic patches, rough description here [1]. So I wrote
> one which checks for every c-file in the kernel-tree:
>
> - Is there a structure containing (bin_)attribute?
> - Does a function declare a pointer to such a structure?
> - Is this pointer then used for a sysfs_create(_bin)_file?
> - If so, has there been a call to sysfs(_bin)_attr_init before?
>
> If not -> report. As coccinelle works on an abstract level of the code (not on
> source-code level), it can follow code-paths and such. Really nice tool, once
> you gathered the information to learn it.
> 
> The outcome are those patches:
> 
> http://patchwork.kernel.org/patch/85214/
>
> http://patchwork.kernel.org/patch/85212/
> (which is pretty much the same like the patch originating this thread. I vote
> very much for dropping those patches and use
> http://patchwork.kernel.org/patch/84814/ instead!)
> 
> http://patchwork.kernel.org/patch/85213/

Hey, goodie.  Can we merge the trivial one-liners into a single patch that 
fixes the issue?

Also, that "firmware_attr_data" thing should probably be marked 'const', 
no? That should make it more obvious that it's ok to share across multiple 
different firmware file, I think.

> Eric, if you have a bit of time, acking these patches would help them getting
> accepted for the mips/rtc-trees, IMHO.

You removed everybody from the Cc: line, so I doubt Eric even sees your 
email. I added the cc's back in.

			Linus

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Linus Torvalds]

On Sun, 14 Mar 2010, Linus Torvalds wrote:
> 
> On Sun, 14 Mar 2010, Wolfram Sang wrote:
>
> > http://patchwork.kernel.org/patch/85212/
> > (which is pretty much the same like the patch originating this thread. I vote
> > very much for dropping those patches and use
> > http://patchwork.kernel.org/patch/84814/ instead!)
> 
> Also, that "firmware_attr_data" thing should probably be marked 'const', 
> no? That should make it more obvious that it's ok to share across multiple 
> different firmware file, I think.

Btw, if it wasn't obvious, this was a comment on that

	http://patchwork.kernel.org/patch/84814/

patch that would replace the one-liner in 85212.

			Linus

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Eric W. Biederman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, 14 Mar 2010, Wolfram Sang wrote:
>>
>> coccinelle can apply semantic patches, rough description here [1]. So I wrote
>> one which checks for every c-file in the kernel-tree:
>>
>> - Is there a structure containing (bin_)attribute?
>> - Does a function declare a pointer to such a structure?
>> - Is this pointer then used for a sysfs_create(_bin)_file?
>> - If so, has there been a call to sysfs(_bin)_attr_init before?
>>
>> If not -> report. As coccinelle works on an abstract level of the code (not on
>> source-code level), it can follow code-paths and such. Really nice tool, once
>> you gathered the information to learn it.

Any chance you can post the semantic patch?  If it isn't too hard it
would be nice to have the warm fuzzy of running it myself and seeing
that there are no more of these left in the kernel.

Eric

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Wolfram Sang]

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

On Sun, Mar 14, 2010 at 12:08:55PM -0700, Eric W. Biederman wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Sun, 14 Mar 2010, Wolfram Sang wrote:
> >>
> >> coccinelle can apply semantic patches, rough description here [1]. So I wrote
> >> one which checks for every c-file in the kernel-tree:
> >>
> >> - Is there a structure containing (bin_)attribute?
> >> - Does a function declare a pointer to such a structure?
> >> - Is this pointer then used for a sysfs_create(_bin)_file?
> >> - If so, has there been a call to sysfs(_bin)_attr_init before?
> >>
> >> If not -> report. As coccinelle works on an abstract level of the code (not on
> >> source-code level), it can follow code-paths and such. Really nice tool, once
> >> you gathered the information to learn it.
> 
> Any chance you can post the semantic patch?  If it isn't too hard it
> would be nice to have the warm fuzzy of running it myself and seeing
> that there are no more of these left in the kernel.

(Strange, I sent an introductory message to the series:

http://kerneltrap.org/mailarchive/linux-kernel-janitors/2010/3/12/6848563

But it doesn't show up in the gmane-archives? And I just learned that CCs added
while editing the introductory message will be dropped by git-send-email. This
is why it isn't on lkml. I have to fix that next time.)

Sure, reviewing is very welcome. I am still new to the semantic patches, so a
double check is surely helpful (also adding Julia Lawall to cc, grand master of
this topic).

Here is the patch again for the binary-attributes (I recommend coccinelle
v0.2.2. Run it with 'spatch -sp_file <name> .' form kernel-top-level-dir):

@ init @
identifier struct_name, bin;
@@

        struct struct_name {
                ...
                struct bin_attribute bin;
                ...
        };

@ main extends init @
expression E;
statement S;
identifier name, err;
@@

(
        struct struct_name *name;
|
-       struct struct_name *name = NULL;
+       struct struct_name *name;
)
        ...
(
        sysfs_bin_attr_init(&name->bin);
|
+       sysfs_bin_attr_init(&name->bin);
        if (sysfs_create_bin_file(E, &name->bin))
                S
|
+       sysfs_bin_attr_init(&name->bin);
        err = sysfs_create_bin_file(E, &name->bin);
)


Here is my patch for the non-binary-attributes:

@ init @
identifier struct_name, attr;
@@

        struct struct_name {
                ...
                struct attribute attr;
                ...
        };

@ main extends init @
expression E;
statement S;
identifier name, err;
@@

(
        struct struct_name *name;
|
-       struct struct_name *name = NULL;
+       struct struct_name *name;
)
        ...
(
        sysfs_attr_init(&name->attr);
|
+       sysfs_attr_init(&name->attr);
        if (sysfs_create_file(E, &name->attr))
                S
|
+       sysfs_attr_init(&name->attr);
        err = sysfs_create_file(E, &name->attr);
)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Wolfram Sang]

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

On Sun, Mar 14, 2010 at 11:04:25AM -0700, Linus Torvalds wrote:

> Hey, goodie.  Can we merge the trivial one-liners into a single patch that 
> fixes the issue?

Sure, will send it in a minute.

> Also, that "firmware_attr_data" thing should probably be marked 'const', 
> no? That should make it more obvious that it's ok to share across multiple 
> different firmware file, I think.

I will leave that to Dmitry (adding to CC).

> You removed everybody from the Cc: line, so I doubt Eric even sees your 
> email. I added the cc's back in.

Oops, thanks. I was using gmane to reply to that mail and did probably
something wrong. I'm sorry.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] init dynamic bin_attribute structures

[Wolfram Sang]

Commit 6992f5334995af474c2b58d010d08bc597f0f2fe introduced this requirement.
First, at25 was fixed manually. Then, other occurences were found with
coccinelle and the following semantic patch. Results were reviewed and fixed
up:

@ init @
identifier struct_name, bin;
@@

	struct struct_name {
		...
		struct bin_attribute bin;
		...
	};

@ main extends init @
expression E;
statement S;
identifier name, err;
@@

(
	struct struct_name *name;
|
-	struct struct_name *name = NULL;
+	struct struct_name *name;
)
	...
(
	sysfs_bin_attr_init(&name->bin);
|
+	sysfs_bin_attr_init(&name->bin);
	if (sysfs_create_bin_file(E, &name->bin))
		S
|
+	sysfs_bin_attr_init(&name->bin);
	err = sysfs_create_bin_file(E, &name->bin);
)

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 arch/mips/txx9/generic/setup.c |    1 +
 drivers/misc/eeprom/at25.c     |    1 +
 drivers/rtc/rtc-ds1742.c       |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/mips/txx9/generic/setup.c b/arch/mips/txx9/generic/setup.c
index 7174d83..95184a0 100644
--- a/arch/mips/txx9/generic/setup.c
+++ b/arch/mips/txx9/generic/setup.c
@@ -956,6 +956,7 @@ void __init txx9_sramc_init(struct resource *r)
 	if (!dev->base)
 		goto exit;
 	dev->dev.cls = &txx9_sramc_sysdev_class;
+	sysfs_bin_attr_init(&dev->bindata_attr);
 	dev->bindata_attr.attr.name = "bindata";
 	dev->bindata_attr.attr.mode = S_IRUSR | S_IWUSR;
 	dev->bindata_attr.read = txx9_sram_read;
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index d902d81..d194212 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -347,6 +347,7 @@ static int at25_probe(struct spi_device *spi)
 	 * that's sensitive for read and/or write, like ethernet addresses,
 	 * security codes, board-specific manufacturing calibrations, etc.
 	 */
+	sysfs_bin_attr_init(&at25->bin);
 	at25->bin.attr.name = "eeprom";
 	at25->bin.attr.mode = S_IRUSR;
 	at25->bin.read = at25_bin_read;
diff --git a/drivers/rtc/rtc-ds1742.c b/drivers/rtc/rtc-ds1742.c
index a127336..cad9ceb 100644
--- a/drivers/rtc/rtc-ds1742.c
+++ b/drivers/rtc/rtc-ds1742.c
@@ -184,6 +184,7 @@ static int __devinit ds1742_rtc_probe(struct platform_device *pdev)
 	pdata->size_nvram = pdata->size - RTC_SIZE;
 	pdata->ioaddr_rtc = ioaddr + pdata->size_nvram;
 
+	sysfs_bin_attr_init(&pdata->nvram_attr);
 	pdata->nvram_attr.attr.name = "nvram";
 	pdata->nvram_attr.attr.mode = S_IRUGO | S_IWUSR;
 	pdata->nvram_attr.read = ds1742_nvram_read;
-- 
1.7.0

[PATCH] init dynamic bin_attribute structures

[Wolfram Sang]

Commit 6992f5334995af474c2b58d010d08bc597f0f2fe introduced this requirement.
First, at25 was fixed manually. Then, other occurences were found with
coccinelle and the following semantic patch. Results were reviewed and fixed
up:

@ init @
identifier struct_name, bin;
@@

	struct struct_name {
		...
		struct bin_attribute bin;
		...
	};

@ main extends init @
expression E;
statement S;
identifier name, err;
@@

(
	struct struct_name *name;
|
-	struct struct_name *name = NULL;
+	struct struct_name *name;
)
	...
(
	sysfs_bin_attr_init(&name->bin);
|
+	sysfs_bin_attr_init(&name->bin);
	if (sysfs_create_bin_file(E, &name->bin))
		S
|
+	sysfs_bin_attr_init(&name->bin);
	err = sysfs_create_bin_file(E, &name->bin);
)

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 arch/mips/txx9/generic/setup.c |    1 +
 drivers/misc/eeprom/at25.c     |    1 +
 drivers/rtc/rtc-ds1742.c       |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/mips/txx9/generic/setup.c b/arch/mips/txx9/generic/setup.c
index 7174d83..95184a0 100644
--- a/arch/mips/txx9/generic/setup.c
+++ b/arch/mips/txx9/generic/setup.c
@@ -956,6 +956,7 @@ void __init txx9_sramc_init(struct resource *r)
 	if (!dev->base)
 		goto exit;
 	dev->dev.cls = &txx9_sramc_sysdev_class;
+	sysfs_bin_attr_init(&dev->bindata_attr);
 	dev->bindata_attr.attr.name = "bindata";
 	dev->bindata_attr.attr.mode = S_IRUSR | S_IWUSR;
 	dev->bindata_attr.read = txx9_sram_read;
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index d902d81..d194212 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -347,6 +347,7 @@ static int at25_probe(struct spi_device *spi)
 	 * that's sensitive for read and/or write, like ethernet addresses,
 	 * security codes, board-specific manufacturing calibrations, etc.
 	 */
+	sysfs_bin_attr_init(&at25->bin);
 	at25->bin.attr.name = "eeprom";
 	at25->bin.attr.mode = S_IRUSR;
 	at25->bin.read = at25_bin_read;
diff --git a/drivers/rtc/rtc-ds1742.c b/drivers/rtc/rtc-ds1742.c
index a127336..cad9ceb 100644
--- a/drivers/rtc/rtc-ds1742.c
+++ b/drivers/rtc/rtc-ds1742.c
@@ -184,6 +184,7 @@ static int __devinit ds1742_rtc_probe(struct platform_device *pdev)
 	pdata->size_nvram = pdata->size - RTC_SIZE;
 	pdata->ioaddr_rtc = ioaddr + pdata->size_nvram;
 
+	sysfs_bin_attr_init(&pdata->nvram_attr);
 	pdata->nvram_attr.attr.name = "nvram";
 	pdata->nvram_attr.attr.mode = S_IRUGO | S_IWUSR;
 	pdata->nvram_attr.read = ds1742_nvram_read;
-- 
1.7.0

Re: [PATCH] init dynamic bin_attribute structures

[Jiri Kosina]

On Mon, 15 Mar 2010, Wolfram Sang wrote:

> Commit 6992f5334995af474c2b58d010d08bc597f0f2fe introduced this requirement.
> First, at25 was fixed manually. Then, other occurences were found with
> coccinelle and the following semantic patch. Results were reviewed and fixed
> up:
> 
> @ init @
> identifier struct_name, bin;
> @@
> 
> 	struct struct_name {
> 		...
> 		struct bin_attribute bin;
> 		...
> 	};
> 
> @ main extends init @
> expression E;
> statement S;
> identifier name, err;
> @@
> 
> (
> 	struct struct_name *name;
> |
> -	struct struct_name *name = NULL;
> +	struct struct_name *name;
> )
> 	...
> (
> 	sysfs_bin_attr_init(&name->bin);
> |
> +	sysfs_bin_attr_init(&name->bin);
> 	if (sysfs_create_bin_file(E, &name->bin))
> 		S
> |
> +	sysfs_bin_attr_init(&name->bin);
> 	err = sysfs_create_bin_file(E, &name->bin);
> )
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>  arch/mips/txx9/generic/setup.c |    1 +
>  drivers/misc/eeprom/at25.c     |    1 +
>  drivers/rtc/rtc-ds1742.c       |    1 +
>  3 files changed, 3 insertions(+), 0 deletions(-)

I don't understand how cocinelle works, but the resulting patch seems to 
be incomplete.

The fix is needed at least in firmware loader (for which Greg has already 
queued patch), in ACPI thermal driver [1], etc).

[1] http://lkml.indiana.edu/hypermail/linux/kernel/1003.1/02680.html

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH] init dynamic bin_attribute structures

[Jiri Kosina]

On Mon, 15 Mar 2010, Wolfram Sang wrote:

> Commit 6992f5334995af474c2b58d010d08bc597f0f2fe introduced this requirement.
> First, at25 was fixed manually. Then, other occurences were found with
> coccinelle and the following semantic patch. Results were reviewed and fixed
> up:
> 
> @ init @
> identifier struct_name, bin;
> @@
> 
> 	struct struct_name {
> 		...
> 		struct bin_attribute bin;
> 		...
> 	};
> 
> @ main extends init @
> expression E;
> statement S;
> identifier name, err;
> @@
> 
> (
> 	struct struct_name *name;
> |
> -	struct struct_name *name = NULL;
> +	struct struct_name *name;
> )
> 	...
> (
> 	sysfs_bin_attr_init(&name->bin);
> |
> +	sysfs_bin_attr_init(&name->bin);
> 	if (sysfs_create_bin_file(E, &name->bin))
> 		S
> |
> +	sysfs_bin_attr_init(&name->bin);
> 	err = sysfs_create_bin_file(E, &name->bin);
> )
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>  arch/mips/txx9/generic/setup.c |    1 +
>  drivers/misc/eeprom/at25.c     |    1 +
>  drivers/rtc/rtc-ds1742.c       |    1 +
>  3 files changed, 3 insertions(+), 0 deletions(-)

I don't understand how cocinelle works, but the resulting patch seems to 
be incomplete.

The fix is needed at least in firmware loader (for which Greg has already 
queued patch), in ACPI thermal driver [1], etc).

[1] http://lkml.indiana.edu/hypermail/linux/kernel/1003.1/02680.html

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [rtc-linux] Re: [PATCH] init dynamic bin_attribute structures

[Wolfram Sang]

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


> I don't understand how cocinelle works, but the resulting patch seems to 
> be incomplete.

Okay, will investigate, thanks for the pointer.

> The fix is needed at least in firmware loader (for which Greg has already 
> queued patch), in ACPI thermal driver [1], etc).

Firmware loader should be handled differently, see other mails in the
originating thread, so I skipped it intentionally. at24 also needed a fix, but
that one was just applied via the i2c-tree, so I skipped it, too. Will check
the ACPI-case!

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [rtc-linux] Re: [PATCH] init dynamic bin_attribute structures

[Wolfram Sang]

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


> I don't understand how cocinelle works, but the resulting patch seems to 
> be incomplete.

Okay, will investigate, thanks for the pointer.

> The fix is needed at least in firmware loader (for which Greg has already 
> queued patch), in ACPI thermal driver [1], etc).

Firmware loader should be handled differently, see other mails in the
originating thread, so I skipped it intentionally. at24 also needed a fix, but
that one was just applied via the i2c-tree, so I skipped it, too. Will check
the ACPI-case!

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] init dynamic bin_attribute structures

[Linus Torvalds]

On Mon, 15 Mar 2010, Jiri Kosina wrote:
> 
> I don't understand how cocinelle works, but the resulting patch seems to 
> be incomplete.
> 
> The fix is needed at least in firmware loader (for which Greg has already 
> queued patch), in ACPI thermal driver [1], etc).

The firmware loader should be done differently:

	http://patchwork.kernel.org/patch/84814/

and I suspect that Wolfram may have skipped the thermal one as having been 
done by another patch (that I didn't merge, due to having questions about 
the whole issue, but that I don't have any objections to)

But maybe there is also some limitation to the coccinelle rule too.

		Linus

Re: [PATCH] init dynamic bin_attribute structures

[Linus Torvalds]

On Mon, 15 Mar 2010, Jiri Kosina wrote:
> 
> I don't understand how cocinelle works, but the resulting patch seems to 
> be incomplete.
> 
> The fix is needed at least in firmware loader (for which Greg has already 
> queued patch), in ACPI thermal driver [1], etc).

The firmware loader should be done differently:

	http://patchwork.kernel.org/patch/84814/

and I suspect that Wolfram may have skipped the thermal one as having been 
done by another patch (that I didn't merge, due to having questions about 
the whole issue, but that I don't have any objections to)

But maybe there is also some limitation to the coccinelle rule too.

		Linus

Re: [PATCH] base firmware: Fix BUG from sysfs attributes change in commit a2db6842873c8e5a70652f278d469128cb52db70

[Larry Finger]

On 03/13/2010 04:32 PM, Linus Torvalds wrote:
> 
> 
> On Sat, 13 Mar 2010, Larry Finger wrote:
>>
>> Commit a2db6842873c8e5a70652f278d469128cb52db70 changed the way that
>> sysfs attributes are handled. With lockdep checking enabled, a firmware
>> loading request from b43 generates the following BUG:
>>
>> BUG: key ffff8800b85f4870 not in .data!
>> ------------[ cut here ]------------
>> WARNING: at kernel/lockdep.c:2706 lockdep_init_map+0x236/0x5d0()
> 
> I don't think we can fix these problems this way.
> 
> Lookie here:
> 
> 	[torvalds@i5 linux]$ git grep sysfs_create_bin_file | wc
> 	     68     319    5999
> 	[torvalds@i5 linux]$ git grep sysfs_bin_attr_init | wc
> 	      8      24     522
> 
> and you sent in patches to fix _two_ of the remaining 60 cases.

Those are the two that showed up on my machine, and as Jiri says, not all 68
will be needed. I need them to see any lockdep problems on the driver I'm
developing. I'll watch for developments.

Larry