Hi,
when I load b43 with hwtkip=1 system hangs after
' b43-phy0 debug: Using hardware based encryption for keyidx: 0, mac:...'
throwing the below BUG.
This is a non-SMP non-preemtive kernel. It looks like we are entering
b43_op_update_tkip_key() already holding the wl->mutex. We lock mutex in
b43_interrupt_thread_handler().
This problem is present in stable and also in compat-wireless-2010-01-22
shapshot if I haven't missed something. As a quick and dirty fix I have
removed wl->mutex lock/unlock from b43_op_update_tkip_key(). Box survived
one day (until now) and a ~3GB file transfer over WLAN using hwtkip in
this way.
Jan 22 23:00:29 localhost kernel: [ 865.743138] b43-phy0 debug: Using
hardware based encryption for keyidx: 0, mac: ....
Jan 22 23:00:29 localhost kernel: [ 865.751462] BUG: scheduling while
atomic: irq/10-b43/1914/0x00000100
Jan 22 23:00:29 localhost kernel: [ 865.751646] Modules linked in: b43
ssb mac80211 cfg80211 rfkill snd_via82xx snd_ac97_codec ac97_bus
snd_mpu401_uart snd_seq_midi pcmcia snd_rawmidi 8139too pcmcia_core
firmware_class 8139cp [last unloaded: compat_firmware_class]
Jan 22 23:00:29 localhost kernel: [ 865.752866] Pid: 1914, comm:
irq/10-b43 Not tainted 2.6.32.3 #7
Jan 22 23:00:29 localhost kernel: [ 865.753014] Call Trace:
Jan 22 23:00:29 localhost kernel: [ 865.753191] [<c13583ee>] ?
schedule+0x3e/0x420
Jan 22 23:00:29 localhost kernel: [ 865.753374] [<c101c41d>] ?
enqueue_entity+0x7d/0x2d0
Jan 22 23:00:29 localhost kernel: [ 865.753543] [<c1358d36>] ?
__mutex_lock_common+0x76/0xc0
Jan 22 23:00:29 localhost kernel: [ 865.753823] [<c4ae1904>] ?
rx_tkip_phase1_write+0x74/0x100 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.753998] [<c1358d8f>] ?
__mutex_lock_slowpath+0xf/0x20
Jan 22 23:00:29 localhost kernel: [ 865.754167] [<c1358c69>] ?
mutex_lock+0x9/0x10
Jan 22 23:00:29 localhost kernel: [ 865.754329] [<c1358c69>] ?
mutex_lock+0x9/0x10
Jan 22 23:00:29 localhost kernel: [ 865.754539] [<c4ae1a12>] ?
b43_op_update_tkip_key+0x82/0x90 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.754761] [<c4ae1990>] ?
b43_op_update_tkip_key+0x0/0x90 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.755068] [<c4a6a17a>] ?
ieee80211_tkip_decrypt_data+0x11a/0x190 [mac80211]
Jan 22 23:00:29 localhost kernel: [ 865.755335] [<c103981d>] ?
getnstimeofday+0x4d/0xd0
Jan 22 23:00:29 localhost kernel: [ 865.755555] [<c4a61a37>] ?
ieee80211_crypto_tkip_decrypt+0xa7/0xf0 [mac80211]
Jan 22 23:00:29 localhost kernel: [ 865.755871] [<c4a6d02e>] ?
ieee80211_invoke_rx_handlers+0x75e/0x18b0 [mac80211]
Jan 22 23:00:29 localhost kernel: [ 865.756175] [<c12da735>] ?
sock_sendmsg+0x95/0xb0
Jan 22 23:00:29 localhost kernel: [ 865.756343] [<c101c41d>] ?
enqueue_entity+0x7d/0x2d0
Jan 22 23:00:29 localhost kernel: [ 865.756573] [<c4a6bff4>] ?
prepare_for_handlers+0x24/0x2f0 [mac80211]
Jan 22 23:00:29 localhost kernel: [ 865.756813] [<c4a6e7af>] ?
ieee80211_rx+0x62f/0x690 [mac80211]
Jan 22 23:00:29 localhost kernel: [ 865.757085] [<c4af1b6e>] ?
b43_rx+0x43e/0x490 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.757321] [<c4af556b>] ?
setup_rx_descbuffer+0x3b/0x160 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.757549] [<c4af0008>] ?
b43_lpphy_op_init+0x668/0xf10 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.757776] [<c4af4f06>] ?
ssb_dma_sync_single_for_device+0x56/0x60 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.758020] [<c4af590d>] ?
b43_dma_rx+0x22d/0x290 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.758245] [<c4ae5910>] ?
b43_do_interrupt_thread+0x5d0/0x750 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.758476] [<c4ae5aa5>] ?
b43_interrupt_thread_handler+0x15/0x30 [b43]
Jan 22 23:00:29 localhost kernel: [ 865.758678] [<c1064bcf>] ?
irq_thread+0x6f/0x140
Jan 22 23:00:29 localhost kernel: [ 865.758842] [<c1064b60>] ?
irq_thread+0x0/0x140
Jan 22 23:00:29 localhost kernel: [ 865.759022] [<c1032b90>] ?
kthread+0x60/0x70
Jan 22 23:00:29 localhost kernel: [ 865.759183] [<c1032b30>] ?
kthread+0x0/0x70
Jan 22 23:00:29 localhost kernel: [ 865.759352] [<c1003177>] ?
kernel_thread_helper+0x7/0x10
Jan 22 23:02:31 localhost kernel: imklog 3.18.6, log source = /proc/kmsg
started.
BR
Csaba
On Saturday 23 January 2010 19:53:48 Johannes Berg wrote:
> Actually you can't, because you can't acquire the mutex here since we're
> in an atomic section. So relying on it being already locked is probably
> safer. However, you can't actually sleep anyway...
I'm OK with removing the lock from the callback, if it is guaranteed that
the callback is only called on behalf of a RX call.
We can just remove the lock and add a comment explaining why it is not locked...
--
Greetings, Michael.
On 01/23/2010 01:00 PM, Michael Buesch wrote:
> On Saturday 23 January 2010 19:53:48 Johannes Berg wrote:
>> Actually you can't, because you can't acquire the mutex here since we're
>> in an atomic section. So relying on it being already locked is probably
>> safer. However, you can't actually sleep anyway...
>
> I'm OK with removing the lock from the callback, if it is guaranteed that
> the callback is only called on behalf of a RX call.
>
> We can just remove the lock and add a comment explaining why it is not locked...
That works for me.
Larry
On Saturday 23 January 2010 19:34:19 Larry Finger wrote:
> On 01/23/2010 12:19 PM, Michael Buesch wrote:
> > On Saturday 23 January 2010 18:47:42 Johannes Berg wrote:
> >>
> >> I think this might be because update_tkip_key is called from within the
> >> RX path, so it's probably not even safe to use mutexes to start with?
> >
> > Well, if mac80211 does a callback into the driver on behalf of a driver call,
> > that is broken design. It would break for all locks, not just mutexes.
> > We should probably switch back to ieee80211_rx_irqsafe to workaround these
> > circular locking problems.
>
> Michael, I'll let you fix this. I do confirm that the mutex is locked when the
> update_tkip_key callback is invoked.
>
> Was this problem introduced with the switch to interrupt threads? If so, then
> the fix needs to be applied to 2.6.32 and 2.6.33.
Well, not really. It was a separate patch that converted ieee80211_rx_irqsafe
to ieee80211_rx and a later patch changed that to ieee80211_rx_ni.
--
Greetings, Michael.
On Sun, 2010-01-24 at 12:45 +0100, Michael Buesch wrote:
> On Sunday 24 January 2010 12:32:50 Johannes Berg wrote:
> > > >>> We can just remove the lock and add a comment explaining why it is not locked...
> >
> > Mind you, it won't work on SDIO hardware since the callback cannot
> > sleep, contrary to what Kalle documented :(
>
> I don't understand. Why wouldn't SDIO be allowed to sleep?
Because this is called within the RX path code which does
rcu_read_lock() around it.
Really, somebody just needs to go and add key TODO in mac80211 to pick
up phase1 key changes and update them in the driver, that gets rid of
all the locking and atomic problems.
johannes
On Sunday 24 January 2010 12:32:50 Johannes Berg wrote:
> > >>> We can just remove the lock and add a comment explaining why it is not locked...
>
> Mind you, it won't work on SDIO hardware since the callback cannot
> sleep, contrary to what Kalle documented :(
I don't understand. Why wouldn't SDIO be allowed to sleep?
b43 is fully preemptible, except for the lowlevel PCI interrupt handler.
--
Greetings, Michael.
On 01/23/2010 12:19 PM, Michael Buesch wrote:
> On Saturday 23 January 2010 18:47:42 Johannes Berg wrote:
>>
>> I think this might be because update_tkip_key is called from within the
>> RX path, so it's probably not even safe to use mutexes to start with?
>
> Well, if mac80211 does a callback into the driver on behalf of a driver call,
> that is broken design. It would break for all locks, not just mutexes.
> We should probably switch back to ieee80211_rx_irqsafe to workaround these
> circular locking problems.
Michael, I'll let you fix this. I do confirm that the mutex is locked when the
update_tkip_key callback is invoked.
Was this problem introduced with the switch to interrupt threads? If so, then
the fix needs to be applied to 2.6.32 and 2.6.33.
Larry
On Sat, 2010-01-23 at 19:19 +0100, Michael Buesch wrote:
> > > - mutex_lock(&wl->mutex);
> > > + if (!mutex_is_locked(&wl->mutex)) {
> >
> > I think this might be because update_tkip_key is called from within the
> > RX path, so it's probably not even safe to use mutexes to start with?
>
> Well, if mac80211 does a callback into the driver on behalf of a driver call,
> that is broken design. It would break for all locks, not just mutexes.
Yes, but
> We should probably switch back to ieee80211_rx_irqsafe to workaround these
> circular locking problems.
Actually you can't, because you can't acquire the mutex here since we're
in an atomic section. So relying on it being already locked is probably
safer. However, you can't actually sleep anyway...
the proper fix would be to delay the call in mac80211 to a work.
johannes
Csaba,
in case you read the list -- your email isn't working at all. That will
get you kicked off the list, and really isn't a good way to try to
communicate with people.
johannes
On Saturday 23 January 2010 18:47:42 Johannes Berg wrote:
> On Sat, 2010-01-23 at 11:20 -0600, Larry Finger wrote:
>
> > > This is a non-SMP non-preemtive kernel. It looks like we are entering
> > > b43_op_update_tkip_key() already holding the wl->mutex. We lock mutex in
> > > b43_interrupt_thread_handler().
> > >
> > > This problem is present in stable and also in compat-wireless-2010-01-22
> > > shapshot if I haven't missed something. As a quick and dirty fix I have
> > > removed wl->mutex lock/unlock from b43_op_update_tkip_key(). Box survived
> > > one day (until now) and a ~3GB file transfer over WLAN using hwtkip in
> > > this way.
> >
> > Your analysis seem to be correct. I tested with the following patch, which tests
> > to see if the mutex is locked on entry. If not, it logs a message, locks the
> > mutex and sets a flag to indicate that the mutex should be unlocked on exit.
> > This patch is not SMP-safe, but is merely for testing. The printk statement has
> > not triggered after about 1 hour of testing. I'll give it a bit more testing
> > before a final patch is submitted.
> >
> > Index: wireless-testing/drivers/net/wireless/b43/main.c
> > ===================================================================
> > --- wireless-testing.orig/drivers/net/wireless/b43/main.c
> > +++ wireless-testing/drivers/net/wireless/b43/main.c
> > @@ -850,11 +850,16 @@ static void b43_op_update_tkip_key(struc
> > struct b43_wl *wl = hw_to_b43_wl(hw);
> > struct b43_wldev *dev;
> > int index = keyconf->hw_key_idx;
> > + bool locked_here = 0;
> >
> > if (B43_WARN_ON(!modparam_hwtkip))
> > return;
> >
> > - mutex_lock(&wl->mutex);
> > + if (!mutex_is_locked(&wl->mutex)) {
>
> I think this might be because update_tkip_key is called from within the
> RX path, so it's probably not even safe to use mutexes to start with?
Well, if mac80211 does a callback into the driver on behalf of a driver call,
that is broken design. It would break for all locks, not just mutexes.
We should probably switch back to ieee80211_rx_irqsafe to workaround these
circular locking problems.
--
Greetings, Michael.
On Saturday 23 January 2010 20:31:43 Larry Finger wrote:
> On 01/23/2010 01:00 PM, Michael Buesch wrote:
> > On Saturday 23 January 2010 19:53:48 Johannes Berg wrote:
> >> Actually you can't, because you can't acquire the mutex here since we're
> >> in an atomic section. So relying on it being already locked is probably
> >> safer. However, you can't actually sleep anyway...
> >
> > I'm OK with removing the lock from the callback, if it is guaranteed that
> > the callback is only called on behalf of a RX call.
> >
> > We can just remove the lock and add a comment explaining why it is not locked...
>
> That works for me.
>
untested
---
drivers/net/wireless/b43/main.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
--- wireless-testing.orig/drivers/net/wireless/b43/main.c
+++ wireless-testing/drivers/net/wireless/b43/main.c
@@ -856,22 +856,19 @@ static void b43_op_update_tkip_key(struc
if (B43_WARN_ON(!modparam_hwtkip))
return;
- mutex_lock(&wl->mutex);
-
+ /* This is only called from the RX path though mac80211, where
+ * our mutex is already locked. */
+ B43_WARN_ON(!mutex_is_locked(&wl->mutex));
dev = wl->current_dev;
- if (!dev || b43_status(dev) < B43_STAT_INITIALIZED)
- goto out_unlock;
+ B43_WARN_ON(!dev || b43_status(dev) < B43_STAT_INITIALIZED);
keymac_write(dev, index, NULL); /* First zero out mac to avoid race */
rx_tkip_phase1_write(dev, index, iv32, phase1key);
/* only pairwise TKIP keys are supported right now */
if (WARN_ON(!sta))
- goto out_unlock;
+ return;
keymac_write(dev, index, sta->addr);
-
-out_unlock:
- mutex_unlock(&wl->mutex);
}
static void do_key_write(struct b43_wldev *dev,
--
Greetings, Michael.
On Sat, 2010-01-23 at 12:34 -0600, Larry Finger wrote:
> > Well, if mac80211 does a callback into the driver on behalf of a driver call,
> > that is broken design. It would break for all locks, not just mutexes.
> > We should probably switch back to ieee80211_rx_irqsafe to workaround these
> > circular locking problems.
Yeah it's due to the switch.
> Michael, I'll let you fix this. I do confirm that the mutex is locked when the
> update_tkip_key callback is invoked.
>
> Was this problem introduced with the switch to interrupt threads? If so, then
> the fix needs to be applied to 2.6.32 and 2.6.33.
Which is due to that, indeed.
I'm open to changing mac80211 to delay that, but I don't really care too
much about that right now.
johannes
On Sat, 2010-01-23 at 23:58 -0600, Larry Finger wrote:
> On 01/23/2010 01:38 PM, Michael Buesch wrote:
> > On Saturday 23 January 2010 20:31:43 Larry Finger wrote:
> >> On 01/23/2010 01:00 PM, Michael Buesch wrote:
> >>> On Saturday 23 January 2010 19:53:48 Johannes Berg wrote:
> >>>> Actually you can't, because you can't acquire the mutex here since we're
> >>>> in an atomic section. So relying on it being already locked is probably
> >>>> safer. However, you can't actually sleep anyway...
> >>>
> >>> I'm OK with removing the lock from the callback, if it is guaranteed that
> >>> the callback is only called on behalf of a RX call.
> >>>
> >>> We can just remove the lock and add a comment explaining why it is not locked...
Mind you, it won't work on SDIO hardware since the callback cannot
sleep, contrary to what Kalle documented :(
Why is it not giving everybody warnings in might_sleep()?
johannes
On 01/23/2010 01:38 PM, Michael Buesch wrote:
> On Saturday 23 January 2010 20:31:43 Larry Finger wrote:
>> On 01/23/2010 01:00 PM, Michael Buesch wrote:
>>> On Saturday 23 January 2010 19:53:48 Johannes Berg wrote:
>>>> Actually you can't, because you can't acquire the mutex here since we're
>>>> in an atomic section. So relying on it being already locked is probably
>>>> safer. However, you can't actually sleep anyway...
>>>
>>> I'm OK with removing the lock from the callback, if it is guaranteed that
>>> the callback is only called on behalf of a RX call.
>>>
>>> We can just remove the lock and add a comment explaining why it is not locked...
>>
>> That works for me.
>>
>
>
> untested
>
> ---
Tested-by: Larry Finger <[email protected]>
---
On 01/23/2010 02:46 AM, [email protected] wrote:
> Hi,
>
> when I load b43 with hwtkip=1 system hangs after
> ' b43-phy0 debug: Using hardware based encryption for keyidx: 0, mac:...'
> throwing the below BUG.
>
> This is a non-SMP non-preemtive kernel. It looks like we are entering
> b43_op_update_tkip_key() already holding the wl->mutex. We lock mutex in
> b43_interrupt_thread_handler().
>
> This problem is present in stable and also in compat-wireless-2010-01-22
> shapshot if I haven't missed something. As a quick and dirty fix I have
> removed wl->mutex lock/unlock from b43_op_update_tkip_key(). Box survived
> one day (until now) and a ~3GB file transfer over WLAN using hwtkip in
> this way.
Your analysis seem to be correct. I tested with the following patch, which tests
to see if the mutex is locked on entry. If not, it logs a message, locks the
mutex and sets a flag to indicate that the mutex should be unlocked on exit.
This patch is not SMP-safe, but is merely for testing. The printk statement has
not triggered after about 1 hour of testing. I'll give it a bit more testing
before a final patch is submitted.
Index: wireless-testing/drivers/net/wireless/b43/main.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43/main.c
+++ wireless-testing/drivers/net/wireless/b43/main.c
@@ -850,11 +850,16 @@ static void b43_op_update_tkip_key(struc
struct b43_wl *wl = hw_to_b43_wl(hw);
struct b43_wldev *dev;
int index = keyconf->hw_key_idx;
+ bool locked_here = 0;
if (B43_WARN_ON(!modparam_hwtkip))
return;
- mutex_lock(&wl->mutex);
+ if (!mutex_is_locked(&wl->mutex)) {
+ printk(KERN_DEBUG "b43: mutex not locked in %s\n", __func__);
+ mutex_lock(&wl->mutex);
+ locked_here = 1;
+ }
dev = wl->current_dev;
if (!dev || b43_status(dev) < B43_STAT_INITIALIZED)
@@ -866,7 +871,8 @@ static void b43_op_update_tkip_key(struc
keymac_write(dev, index, addr);
out_unlock:
- mutex_unlock(&wl->mutex);
+ if (locked_here)
+ mutex_unlock(&wl->mutex);
}
static void do_key_write(struct b43_wldev *dev,
==================================================
Larry
On Sunday 24 January 2010 12:47:52 Johannes Berg wrote:
> On Sun, 2010-01-24 at 12:45 +0100, Michael Buesch wrote:
> > On Sunday 24 January 2010 12:32:50 Johannes Berg wrote:
> > > > >>> We can just remove the lock and add a comment explaining why it is not locked...
> > >
> > > Mind you, it won't work on SDIO hardware since the callback cannot
> > > sleep, contrary to what Kalle documented :(
> >
> > I don't understand. Why wouldn't SDIO be allowed to sleep?
>
> Because this is called within the RX path code which does
> rcu_read_lock() around it.
Ok, I see. I can live with that, because tkip hwcrypto defaults to off.
> Really, somebody just needs to go and add key TODO in mac80211 to pick
> up phase1 key changes and update them in the driver, that gets rid of
> all the locking and atomic problems.
Too bad I don't understand all that tkip key handling stuff. :)
--
Greetings, Michael.
On Sat, 2010-01-23 at 11:20 -0600, Larry Finger wrote:
> > This is a non-SMP non-preemtive kernel. It looks like we are entering
> > b43_op_update_tkip_key() already holding the wl->mutex. We lock mutex in
> > b43_interrupt_thread_handler().
> >
> > This problem is present in stable and also in compat-wireless-2010-01-22
> > shapshot if I haven't missed something. As a quick and dirty fix I have
> > removed wl->mutex lock/unlock from b43_op_update_tkip_key(). Box survived
> > one day (until now) and a ~3GB file transfer over WLAN using hwtkip in
> > this way.
>
> Your analysis seem to be correct. I tested with the following patch, which tests
> to see if the mutex is locked on entry. If not, it logs a message, locks the
> mutex and sets a flag to indicate that the mutex should be unlocked on exit.
> This patch is not SMP-safe, but is merely for testing. The printk statement has
> not triggered after about 1 hour of testing. I'll give it a bit more testing
> before a final patch is submitted.
>
> Index: wireless-testing/drivers/net/wireless/b43/main.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/b43/main.c
> +++ wireless-testing/drivers/net/wireless/b43/main.c
> @@ -850,11 +850,16 @@ static void b43_op_update_tkip_key(struc
> struct b43_wl *wl = hw_to_b43_wl(hw);
> struct b43_wldev *dev;
> int index = keyconf->hw_key_idx;
> + bool locked_here = 0;
>
> if (B43_WARN_ON(!modparam_hwtkip))
> return;
>
> - mutex_lock(&wl->mutex);
> + if (!mutex_is_locked(&wl->mutex)) {
I think this might be because update_tkip_key is called from within the
RX path, so it's probably not even safe to use mutexes to start with?
johannes
Johannes Berg <[email protected]> writes:
>> >>> We can just remove the lock and add a comment explaining why it is not locked...
>
> Mind you, it won't work on SDIO hardware since the callback cannot
> sleep, contrary to what Kalle documented :(
Oh, sorry about that. Back then I tried to be careful and I checked
the context few calls up. But I didn't check far enough and realise
that all RX code is inside rcu critical section.
I'll send a patch to fix this shortly.
> Why is it not giving everybody warnings in might_sleep()?
Very good question. We should get warnings from both iwlwifi and b43,
but I have no idea why we haven't heard anything.
--
Kalle Valo