b43 problem with led trigger registration

b43 problem with led trigger registration

[Johannes Berg]

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

Hi,

Just booted into my shiny new #everything and got:

[   40.209739] BUG: sleeping function called from invalid context at kernel/rwsem.c:47
[   40.220119] in_atomic():1, irqs_disabled():0
[   40.230514] Call Trace:
[   40.240799] [eec2fc90] [c0009198] show_stack+0x4c/0x1ac (unreliable)
[   40.251247] [eec2fcd0] [c0027a3c] __might_sleep+0xd0/0xf0
[   40.261620] [eec2fce0] [c004c1d0] down_write+0x24/0x64
[   40.271974] [eec2fcf0] [c026347c] led_trigger_register+0xbc/0x118
[   40.282273] [eec2fd10] [f250246c] ieee80211_led_init+0x68/0x160 [mac80211]
[   40.292837] [eec2fd30] [f24eceb0] ieee80211_register_hw+0x200/0x324 [mac80211]
[   40.303295] [eec2fd40] [f2523234] b43_probe+0x9a8/0xa04 [b43]
[   40.313766] [eec2fda0] [f20b06e8] ssb_device_probe+0x50/0xac [ssb]
[   40.324152] [eec2fdb0] [c0214518] driver_probe_device+0xb8/0x1e8
[   40.334541] [eec2fdd0] [c021487c] __driver_attach+0xf8/0x124
[   40.344833] [eec2fdf0] [c0213674] bus_for_each_dev+0x58/0x94
[   40.355100] [eec2fe20] [c0214328] driver_attach+0x24/0x34
[   40.365321] [eec2fe30] [c0213af4] bus_add_driver+0x98/0x208
[   40.375513] [eec2fe50] [c0214b44] driver_register+0x58/0xa0
[   40.385771] [eec2fe60] [f20aff58] __ssb_driver_register+0x2c/0x3c [ssb]
[   40.396017] [eec2fe70] [f103e034] b43_init+0x34/0xe8 [b43]
[   40.406253] [eec2fe80] [c00587e4] sys_init_module+0x154/0x176c
[   40.416498] [eec2ff40] [c0012054] ret_from_syscall+0x0/0x38

I haven't found yet where the atomic is entered...

johannes

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

Re: b43 problem with led trigger registration

[Larry Finger]

Johannes Berg wrote:
> Hi,
> 
> Just booted into my shiny new #everything and got:
> 
> [   40.209739] BUG: sleeping function called from invalid context at kernel/rwsem.c:47
> [   40.220119] in_atomic():1, irqs_disabled():0
> [   40.230514] Call Trace:
> [   40.240799] [eec2fc90] [c0009198] show_stack+0x4c/0x1ac (unreliable)
> [   40.251247] [eec2fcd0] [c0027a3c] __might_sleep+0xd0/0xf0
> [   40.261620] [eec2fce0] [c004c1d0] down_write+0x24/0x64
> [   40.271974] [eec2fcf0] [c026347c] led_trigger_register+0xbc/0x118
> [   40.282273] [eec2fd10] [f250246c] ieee80211_led_init+0x68/0x160 [mac80211]
> [   40.292837] [eec2fd30] [f24eceb0] ieee80211_register_hw+0x200/0x324 [mac80211]
> [   40.303295] [eec2fd40] [f2523234] b43_probe+0x9a8/0xa04 [b43]
> [   40.313766] [eec2fda0] [f20b06e8] ssb_device_probe+0x50/0xac [ssb]
> [   40.324152] [eec2fdb0] [c0214518] driver_probe_device+0xb8/0x1e8
> [   40.334541] [eec2fdd0] [c021487c] __driver_attach+0xf8/0x124
> [   40.344833] [eec2fdf0] [c0213674] bus_for_each_dev+0x58/0x94
> [   40.355100] [eec2fe20] [c0214328] driver_attach+0x24/0x34
> [   40.365321] [eec2fe30] [c0213af4] bus_add_driver+0x98/0x208
> [   40.375513] [eec2fe50] [c0214b44] driver_register+0x58/0xa0
> [   40.385771] [eec2fe60] [f20aff58] __ssb_driver_register+0x2c/0x3c [ssb]
> [   40.396017] [eec2fe70] [f103e034] b43_init+0x34/0xe8 [b43]
> [   40.406253] [eec2fe80] [c00587e4] sys_init_module+0x154/0x176c
> [   40.416498] [eec2ff40] [c0012054] ret_from_syscall+0x0/0x38
> 
> I haven't found yet where the atomic is entered...

Is your system "vanilla" wireless-2.6, or does it include the LED switch patch 
(http://marc.info/?l=linux-wireless&m=119763729709783&w=2)?

Larry

Re: b43 problem with led trigger registration

[Johannes Berg]

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


> Is your system "vanilla" wireless-2.6, or does it include the LED switch patch 
> (http://marc.info/?l=linux-wireless&m=119763729709783&w=2)?

It's "vanilla" wireless-2.6#everything but that patch is present as
commit 81a3136e64abd1a1d93a48c742e4fd50c3565d2a afaict

johannes

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

Re: b43 problem with led trigger registration

[John W. Linville]

On Sun, Dec 16, 2007 at 09:19:34AM -0800, Larry Finger wrote:
> Johannes Berg wrote:
>> Hi,
>>
>> Just booted into my shiny new #everything and got:
>>
>> [   40.209739] BUG: sleeping function called from invalid context at kernel/rwsem.c:47
>> [   40.220119] in_atomic():1, irqs_disabled():0
>> [   40.230514] Call Trace:
>> [   40.240799] [eec2fc90] [c0009198] show_stack+0x4c/0x1ac (unreliable)
>> [   40.251247] [eec2fcd0] [c0027a3c] __might_sleep+0xd0/0xf0
>> [   40.261620] [eec2fce0] [c004c1d0] down_write+0x24/0x64
>> [   40.271974] [eec2fcf0] [c026347c] led_trigger_register+0xbc/0x118
>> [   40.282273] [eec2fd10] [f250246c] ieee80211_led_init+0x68/0x160 [mac80211]
>> [   40.292837] [eec2fd30] [f24eceb0] ieee80211_register_hw+0x200/0x324 [mac80211]
>> [   40.303295] [eec2fd40] [f2523234] b43_probe+0x9a8/0xa04 [b43]
>> [   40.313766] [eec2fda0] [f20b06e8] ssb_device_probe+0x50/0xac [ssb]
>> [   40.324152] [eec2fdb0] [c0214518] driver_probe_device+0xb8/0x1e8
>> [   40.334541] [eec2fdd0] [c021487c] __driver_attach+0xf8/0x124
>> [   40.344833] [eec2fdf0] [c0213674] bus_for_each_dev+0x58/0x94
>> [   40.355100] [eec2fe20] [c0214328] driver_attach+0x24/0x34
>> [   40.365321] [eec2fe30] [c0213af4] bus_add_driver+0x98/0x208
>> [   40.375513] [eec2fe50] [c0214b44] driver_register+0x58/0xa0
>> [   40.385771] [eec2fe60] [f20aff58] __ssb_driver_register+0x2c/0x3c [ssb]
>> [   40.396017] [eec2fe70] [f103e034] b43_init+0x34/0xe8 [b43]
>> [   40.406253] [eec2fe80] [c00587e4] sys_init_module+0x154/0x176c
>> [   40.416498] [eec2ff40] [c0012054] ret_from_syscall+0x0/0x38
>>
>> I haven't found yet where the atomic is entered...
>
> Is your system "vanilla" wireless-2.6, or does it include the LED switch 
> patch (http://marc.info/?l=linux-wireless&m=119763729709783&w=2)?

Not sure what you mean by "vanilla"...that patch is in
wireless-2.6#everything as of last night.

There are not any new mac80211 patches in there yet -- does your fix
depend on a pending mac80211 patch?

FWIW, I don't see anything obvious in a quick code inspection.
I presume that Larry and Michael tested this on the previous iteration
of wireless-2.6 which was based on 2.6.24-rc3.  Any chance that
something in the rc3->rc5 difference accounts for this?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: b43 problem with led trigger registration

[Johannes Berg]

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


> There are not any new mac80211 patches in there yet -- does your fix
> depend on a pending mac80211 patch?

It shouldn't. There weren't any relevant patches to mac80211 that I'm
aware of.

> FWIW, I don't see anything obvious in a quick code inspection.

Me neither.

> I presume that Larry and Michael tested this on the previous iteration
> of wireless-2.6 which was based on 2.6.24-rc3.  Any chance that
> something in the rc3->rc5 difference accounts for this?

Probably. I think lockdep might tell you where the atomic was entered
but I don't have it on this system.

johannes

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

Re: b43 problem with led trigger registration

[Michael Buesch]

On Sunday 16 December 2007 17:20:19 Johannes Berg wrote:
> Hi,
> 
> Just booted into my shiny new #everything and got:
> 
> [   40.209739] BUG: sleeping function called from invalid context at kernel/rwsem.c:47
> [   40.220119] in_atomic():1, irqs_disabled():0
> [   40.230514] Call Trace:
> [   40.240799] [eec2fc90] [c0009198] show_stack+0x4c/0x1ac (unreliable)
> [   40.251247] [eec2fcd0] [c0027a3c] __might_sleep+0xd0/0xf0
> [   40.261620] [eec2fce0] [c004c1d0] down_write+0x24/0x64
> [   40.271974] [eec2fcf0] [c026347c] led_trigger_register+0xbc/0x118
> [   40.282273] [eec2fd10] [f250246c] ieee80211_led_init+0x68/0x160 [mac80211]
> [   40.292837] [eec2fd30] [f24eceb0] ieee80211_register_hw+0x200/0x324 [mac80211]
> [   40.303295] [eec2fd40] [f2523234] b43_probe+0x9a8/0xa04 [b43]
> [   40.313766] [eec2fda0] [f20b06e8] ssb_device_probe+0x50/0xac [ssb]
> [   40.324152] [eec2fdb0] [c0214518] driver_probe_device+0xb8/0x1e8
> [   40.334541] [eec2fdd0] [c021487c] __driver_attach+0xf8/0x124
> [   40.344833] [eec2fdf0] [c0213674] bus_for_each_dev+0x58/0x94
> [   40.355100] [eec2fe20] [c0214328] driver_attach+0x24/0x34
> [   40.365321] [eec2fe30] [c0213af4] bus_add_driver+0x98/0x208
> [   40.375513] [eec2fe50] [c0214b44] driver_register+0x58/0xa0
> [   40.385771] [eec2fe60] [f20aff58] __ssb_driver_register+0x2c/0x3c [ssb]
> [   40.396017] [eec2fe70] [f103e034] b43_init+0x34/0xe8 [b43]
> [   40.406253] [eec2fe80] [c00587e4] sys_init_module+0x154/0x176c
> [   40.416498] [eec2ff40] [c0012054] ret_from_syscall+0x0/0x38
> 
> I haven't found yet where the atomic is entered...
> 
> johannes
> 

I have no idea where we enter atomic state.
I am using a slightly older kernel but also with the patch Larry
mentioned applied. This doesn't happen here.
Could you please try putting an WARN_ON(in_atomic()) assertion into
b43_probe? Though, I'm pretty sure it won't trigger.
I'd rather say mac80211 is aquireing some spinlock in the
register_hw codepath.

Larry, note that this is not related to the b43 LEDs code.
This assertion triggers at the place where we register the
mac80211 TX, RX and radio LED triggers. The place where b43
initialized rfkill and leds is not in the b43_probe stage.

-- 
Greetings Michael.

Re: b43 problem with led trigger registration

[Johannes Berg]

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


> I have no idea where we enter atomic state.

Uh huh...

drivers/leds/led-triggers.c:led_trigger_register:

|        read_lock(&leds_list_lock);
|        list_for_each_entry(led_cdev, &leds_list, node) {
|                down_write(&led_cdev->trigger_lock);

introduced in

commit dc47206e552c0850ad11f7e9a1fca0a3c92f5d65
Author: Richard Purdie <rpurdie@rpsys.net>
Date:   Sat Nov 10 13:29:04 2007 +0000

    leds: Fix led trigger locking bugs

I guess the read_lock needs to be a mutex/rw semaphore.

johannes

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

Re: b43 problem with led trigger registration

[Michael Buesch]

On Sunday 16 December 2007 20:59:36 Johannes Berg wrote:
> 
> > I have no idea where we enter atomic state.
> 
> Uh huh...
> 
> drivers/leds/led-triggers.c:led_trigger_register:
> 
> |        read_lock(&leds_list_lock);
> |        list_for_each_entry(led_cdev, &leds_list, node) {
> |                down_write(&led_cdev->trigger_lock);
> 
> introduced in
> 
> commit dc47206e552c0850ad11f7e9a1fca0a3c92f5d65
> Author: Richard Purdie <rpurdie@rpsys.net>
> Date:   Sat Nov 10 13:29:04 2007 +0000
> 
>     leds: Fix led trigger locking bugs
> 
> I guess the read_lock needs to be a mutex/rw semaphore.

Uh, yes. Was this patch tested at all?

-- 
Greetings Michael.

Re: b43 problem with led trigger registration

[Johannes Berg]

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


> > drivers/leds/led-triggers.c:led_trigger_register:
> > 
> > |        read_lock(&leds_list_lock);
> > |        list_for_each_entry(led_cdev, &leds_list, node) {
> > |                down_write(&led_cdev->trigger_lock);
> > 
> > introduced in
> > 
> > commit dc47206e552c0850ad11f7e9a1fca0a3c92f5d65
> > Author: Richard Purdie <rpurdie@rpsys.net>
> > Date:   Sat Nov 10 13:29:04 2007 +0000
> > 
> >     leds: Fix led trigger locking bugs
> > 
> > I guess the read_lock needs to be a mutex/rw semaphore.
> 
> Uh, yes. Was this patch tested at all?

Not with default triggers, I guess. The code in question only triggers
when a default is assigned to any LED I think.

johannes

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

Re: b43 problem with led trigger registration

[Richard Purdie]

On Sun, 2007-12-16 at 21:38 +0100, Johannes Berg wrote:
> > > drivers/leds/led-triggers.c:led_trigger_register:
> > > 
> > > |        read_lock(&leds_list_lock);
> > > |        list_for_each_entry(led_cdev, &leds_list, node) {
> > > |                down_write(&led_cdev->trigger_lock);
> > > 
> > > introduced in
> > > 
> > > commit dc47206e552c0850ad11f7e9a1fca0a3c92f5d65
> > > Author: Richard Purdie <rpurdie@rpsys.net>
> > > Date:   Sat Nov 10 13:29:04 2007 +0000
> > > 
> > >     leds: Fix led trigger locking bugs
> > > 
> > > I guess the read_lock needs to be a mutex/rw semaphore.
> > 
> > Uh, yes. Was this patch tested at all?
> 
> Not with default triggers, I guess. The code in question only triggers
> when a default is assigned to any LED I think.

Amongst other things it was tested on the Zaurus which has default
triggers. What is the problem you're seeing?

Richard

Re: b43 problem with led trigger registration

[Johannes Berg]

> Amongst other things it was tested on the Zaurus which has default
> triggers. What is the problem you're seeing?

Odd. The problem is this:

> [   40.209739] BUG: sleeping function called from invalid context at kernel/rwsem.c:47
> [   40.220119] in_atomic():1, irqs_disabled():0
> [   40.230514] Call Trace:
> [   40.240799] [eec2fc90] [c0009198] show_stack+0x4c/0x1ac (unreliable)
> [   40.251247] [eec2fcd0] [c0027a3c] __might_sleep+0xd0/0xf0
> [   40.261620] [eec2fce0] [c004c1d0] down_write+0x24/0x64
> [   40.271974] [eec2fcf0] [c026347c] led_trigger_register+0xbc/0x118
> [   40.282273] [eec2fd10] [f250246c] ieee80211_led_init+0x68/0x160 [mac80211]
> [   40.292837] [eec2fd30] [f24eceb0] ieee80211_register_hw+0x200/0x324 [mac80211]
> [   40.303295] [eec2fd40] [f2523234] b43_probe+0x9a8/0xa04 [b43]
> [   40.313766] [eec2fda0] [f20b06e8] ssb_device_probe+0x50/0xac [ssb]
> [   40.324152] [eec2fdb0] [c0214518] driver_probe_device+0xb8/0x1e8
> [   40.334541] [eec2fdd0] [c021487c] __driver_attach+0xf8/0x124
> [   40.344833] [eec2fdf0] [c0213674] bus_for_each_dev+0x58/0x94
> [   40.355100] [eec2fe20] [c0214328] driver_attach+0x24/0x34
> [   40.365321] [eec2fe30] [c0213af4] bus_add_driver+0x98/0x208
> [   40.375513] [eec2fe50] [c0214b44] driver_register+0x58/0xa0
> [   40.385771] [eec2fe60] [f20aff58] __ssb_driver_register+0x2c/0x3c [ssb]
> [   40.396017] [eec2fe70] [f103e034] b43_init+0x34/0xe8 [b43]
> [   40.406253] [eec2fe80] [c00587e4] sys_init_module+0x154/0x176c
> [   40.416498] [eec2ff40] [c0012054] ret_from_syscall+0x0/0x38

johannes

Re: b43 problem with led trigger registration

[Michael Buesch]

On Sunday 16 December 2007 21:43:19 Richard Purdie wrote:
> On Sun, 2007-12-16 at 21:38 +0100, Johannes Berg wrote:
> > > > drivers/leds/led-triggers.c:led_trigger_register:
> > > > 
> > > > |        read_lock(&leds_list_lock);
> > > > |        list_for_each_entry(led_cdev, &leds_list, node) {
> > > > |                down_write(&led_cdev->trigger_lock);
> > > > 
> > > > introduced in
> > > > 
> > > > commit dc47206e552c0850ad11f7e9a1fca0a3c92f5d65
> > > > Author: Richard Purdie <rpurdie@rpsys.net>
> > > > Date:   Sat Nov 10 13:29:04 2007 +0000
> > > > 
> > > >     leds: Fix led trigger locking bugs
> > > > 
> > > > I guess the read_lock needs to be a mutex/rw semaphore.
> > > 
> > > Uh, yes. Was this patch tested at all?
> > 
> > Not with default triggers, I guess. The code in question only triggers
> > when a default is assigned to any LED I think.
> 
> Amongst other things it was tested on the Zaurus which has default
> triggers. What is the problem you're seeing?

You are scheduling (down_write) while holding a spinlock (leds_list_lock).

-- 
Greetings Michael.

Re: b43 problem with led trigger registration

[Richard Purdie]

On Sun, 2007-12-16 at 21:57 +0100, Michael Buesch wrote:
> On Sunday 16 December 2007 21:43:19 Richard Purdie wrote:
> > On Sun, 2007-12-16 at 21:38 +0100, Johannes Berg wrote:
> > > > > drivers/leds/led-triggers.c:led_trigger_register:
> > > > > 
> > > > > |        read_lock(&leds_list_lock);
> > > > > |        list_for_each_entry(led_cdev, &leds_list, node) {
> > > > > |                down_write(&led_cdev->trigger_lock);
> > > > > 
> > > > > introduced in
> > > > > 
> > > > > commit dc47206e552c0850ad11f7e9a1fca0a3c92f5d65
> > > > > Author: Richard Purdie <rpurdie@rpsys.net>
> > > > > Date:   Sat Nov 10 13:29:04 2007 +0000
> > > > > 
> > > > >     leds: Fix led trigger locking bugs
> > > > > 
> > > > > I guess the read_lock needs to be a mutex/rw semaphore.
> > > > 
> > > > Uh, yes. Was this patch tested at all?
> > > 
> > > Not with default triggers, I guess. The code in question only triggers
> > > when a default is assigned to any LED I think.
> > 
> > Amongst other things it was tested on the Zaurus which has default
> > triggers. What is the problem you're seeing?
> 
> You are scheduling (down_write) while holding a spinlock (leds_list_lock).

Eeek, yes, its the LED core's fault.
dc47206e552c0850ad11f7e9a1fca0a3c92f5d65 is needed for various reasons
but that means the patch below is also needed. Its been compile tested,
once its runtime tested I'll push asap.

Richard

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index ba8b04b..64c66b3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -106,9 +106,9 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 		goto err_out;
 
 	/* add to the list of leds */
-	write_lock(&leds_list_lock);
+	down_write(&leds_list_lock);
 	list_add_tail(&led_cdev->node, &leds_list);
-	write_unlock(&leds_list_lock);
+	up_write(&leds_list_lock);
 
 #ifdef CONFIG_LEDS_TRIGGERS
 	init_rwsem(&led_cdev->trigger_lock);
@@ -155,9 +155,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 
 	device_unregister(led_cdev->dev);
 
-	write_lock(&leds_list_lock);
+	down_write(&leds_list_lock);
 	list_del(&led_cdev->node);
-	write_unlock(&leds_list_lock);
+	up_write(&leds_list_lock);
 }
 EXPORT_SYMBOL_GPL(led_classdev_unregister);
 
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 9b015f9..5d1ca10 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -14,11 +14,11 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
-#include <linux/spinlock.h>
+#include <linux/rwsem.h>
 #include <linux/leds.h>
 #include "leds.h"
 
-DEFINE_RWLOCK(leds_list_lock);
+DECLARE_RWSEM(leds_list_lock);
 LIST_HEAD(leds_list);
 
 EXPORT_SYMBOL_GPL(leds_list);
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 0bdb786..13c9026 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -169,7 +169,7 @@ int led_trigger_register(struct led_trigger *trigger)
 	up_write(&triggers_list_lock);
 
 	/* Register with any LEDs that have this as a default trigger */
-	read_lock(&leds_list_lock);
+	down_read(&leds_list_lock);
 	list_for_each_entry(led_cdev, &leds_list, node) {
 		down_write(&led_cdev->trigger_lock);
 		if (!led_cdev->trigger && led_cdev->default_trigger &&
@@ -177,7 +177,7 @@ int led_trigger_register(struct led_trigger *trigger)
 			led_trigger_set(led_cdev, trigger);
 		up_write(&led_cdev->trigger_lock);
 	}
-	read_unlock(&leds_list_lock);
+	up_read(&leds_list_lock);
 
 	return 0;
 }
@@ -212,14 +212,14 @@ void led_trigger_unregister(struct led_trigger *trigger)
 	up_write(&triggers_list_lock);
 
 	/* Remove anyone actively using this trigger */
-	read_lock(&leds_list_lock);
+	down_read(&leds_list_lock);
 	list_for_each_entry(led_cdev, &leds_list, node) {
 		down_write(&led_cdev->trigger_lock);
 		if (led_cdev->trigger == trigger)
 			led_trigger_set(led_cdev, NULL);
 		up_write(&led_cdev->trigger_lock);
 	}
-	read_unlock(&leds_list_lock);
+	up_read(&leds_list_lock);
 }
 
 void led_trigger_unregister_simple(struct led_trigger *trigger)
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index f2f3884..12b6fe9 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -14,6 +14,7 @@
 #define __LEDS_H_INCLUDED
 
 #include <linux/device.h>
+#include <linux/rwsem.h>
 #include <linux/leds.h>
 
 static inline void led_set_brightness(struct led_classdev *led_cdev,
@@ -26,7 +27,7 @@ static inline void led_set_brightness(struct led_classdev *led_cdev,
 		led_cdev->brightness_set(led_cdev, value);
 }
 
-extern rwlock_t leds_list_lock;
+extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
 
 #ifdef CONFIG_LEDS_TRIGGERS

Re: b43 problem with led trigger registration

[Johannes Berg]

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


> Eeek, yes, its the LED core's fault.
> dc47206e552c0850ad11f7e9a1fca0a3c92f5d65 is needed for various reasons
> but that means the patch below is also needed. Its been compile tested,
> once its runtime tested I'll push asap.

Any reason you're using an rw semaphore rather than a simple mutex?
Nothing here is a hot-path, is it, and the led registrations are
mutually exclusive anyway, no?

johannes

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

Re: b43 problem with led trigger registration

[Richard Purdie]

On Sun, 2007-12-16 at 23:17 +0100, Johannes Berg wrote:
> > Eeek, yes, its the LED core's fault.
> > dc47206e552c0850ad11f7e9a1fca0a3c92f5d65 is needed for various reasons
> > but that means the patch below is also needed. Its been compile tested,
> > once its runtime tested I'll push asap.
> 
> Any reason you're using an rw semaphore rather than a simple mutex?
> Nothing here is a hot-path, is it, and the led registrations are
> mutually exclusive anyway, no?

With a rw lock, the for loop of the trigger registration can happen in
parallel with other trigger registrations. This case isn't a hot path as
such but it keeps consistent style with other parts of the LED core
where rw locks are used in hot paths.

It could be replaced with a mutex instead though...

Richard

Re: b43 problem with led trigger registration

[Johannes Berg]

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


On Sun, 2007-12-16 at 22:32 +0000, Richard Purdie wrote:
> On Sun, 2007-12-16 at 23:17 +0100, Johannes Berg wrote:
> > > Eeek, yes, its the LED core's fault.
> > > dc47206e552c0850ad11f7e9a1fca0a3c92f5d65 is needed for various reasons
> > > but that means the patch below is also needed. Its been compile tested,
> > > once its runtime tested I'll push asap.
> > 
> > Any reason you're using an rw semaphore rather than a simple mutex?
> > Nothing here is a hot-path, is it, and the led registrations are
> > mutually exclusive anyway, no?
> 
> With a rw lock, the for loop of the trigger registration can happen in
> parallel with other trigger registrations. This case isn't a hot path as
> such but it keeps consistent style with other parts of the LED core
> where rw locks are used in hot paths.

Ok, just curious really, nothing that I really care about :)

johannes

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

Re: b43 problem with led trigger registration

[Johannes Berg]

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


> Not with default triggers, I guess. The code in question only triggers
> when a default is assigned to any LED I think.

Come to think of it, it only triggers if the LED-trigger is registered
before the LED, unrelated of whether we have defaults or not.

johannes

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

Re: b43 problem with led trigger registration

[Larry Finger]

Michael Buesch wrote:
> On Sunday 16 December 2007 17:20:19 Johannes Berg wrote:
>> Hi,
>>
>> Just booted into my shiny new #everything and got:
>>
>> [   40.209739] BUG: sleeping function called from invalid context at kernel/rwsem.c:47
>> [   40.220119] in_atomic():1, irqs_disabled():0
>> [   40.230514] Call Trace:
>> [   40.240799] [eec2fc90] [c0009198] show_stack+0x4c/0x1ac (unreliable)
>> [   40.251247] [eec2fcd0] [c0027a3c] __might_sleep+0xd0/0xf0
>> [   40.261620] [eec2fce0] [c004c1d0] down_write+0x24/0x64
>> [   40.271974] [eec2fcf0] [c026347c] led_trigger_register+0xbc/0x118
>> [   40.282273] [eec2fd10] [f250246c] ieee80211_led_init+0x68/0x160 [mac80211]
>> [   40.292837] [eec2fd30] [f24eceb0] ieee80211_register_hw+0x200/0x324 [mac80211]
>> [   40.303295] [eec2fd40] [f2523234] b43_probe+0x9a8/0xa04 [b43]
>> [   40.313766] [eec2fda0] [f20b06e8] ssb_device_probe+0x50/0xac [ssb]
>> [   40.324152] [eec2fdb0] [c0214518] driver_probe_device+0xb8/0x1e8
>> [   40.334541] [eec2fdd0] [c021487c] __driver_attach+0xf8/0x124
>> [   40.344833] [eec2fdf0] [c0213674] bus_for_each_dev+0x58/0x94
>> [   40.355100] [eec2fe20] [c0214328] driver_attach+0x24/0x34
>> [   40.365321] [eec2fe30] [c0213af4] bus_add_driver+0x98/0x208
>> [   40.375513] [eec2fe50] [c0214b44] driver_register+0x58/0xa0
>> [   40.385771] [eec2fe60] [f20aff58] __ssb_driver_register+0x2c/0x3c [ssb]
>> [   40.396017] [eec2fe70] [f103e034] b43_init+0x34/0xe8 [b43]
>> [   40.406253] [eec2fe80] [c00587e4] sys_init_module+0x154/0x176c
>> [   40.416498] [eec2ff40] [c0012054] ret_from_syscall+0x0/0x38
>>
>> I haven't found yet where the atomic is entered...
>>
>> johannes
>>
> 
> I have no idea where we enter atomic state.
> I am using a slightly older kernel but also with the patch Larry
> mentioned applied. This doesn't happen here.
> Could you please try putting an WARN_ON(in_atomic()) assertion into
> b43_probe? Though, I'm pretty sure it won't trigger.
> I'd rather say mac80211 is aquireing some spinlock in the
> register_hw codepath.
> 
> Larry, note that this is not related to the b43 LEDs code.
> This assertion triggers at the place where we register the
> mac80211 TX, RX and radio LED triggers. The place where b43
> initialized rfkill and leds is not in the b43_probe stage.

I do not see this problem with my BCM4311/2 on x86_64 hardware.

Larry