On Wednesday 07 October 2009 21:28:40 Larry Finger wrote:
> John W. Linville wrote:
> > On Wed, Oct 07, 2009 at 10:06:05AM -0500, Larry Finger wrote:
> >> In commit 26e5ab35b4c7b1d4cb487a11084520aed9a8d05e entitled "b43: Fix PPC
> >> crash in rfkill polling on unload", the call to stop polling should not have
> >> been placed inside the wl->mutex. The result was incorrect locking messages.
> >>
> >> Signed-off-by: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
> >> ---
> >>
> >> John,
> >>
> >> I had not intended for the previous patch to be applied as I was waiting for
> >> the Bugzilla OP to test. He promised to do that today. In any case, that patch
> >> introduced a locking problem that needs to be fixed.
> >>
> >> Why do the one-liners cause so many problems?
> >>
> >> Larry
> >> ---
> >>
> >> 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
> >> @@ -4501,8 +4501,8 @@ static void b43_op_stop(struct ieee80211
> >>
> >> cancel_work_sync(&(wl->beacon_update_trigger));
> >>
> >> - mutex_lock(&wl->mutex);
> >> wiphy_rfkill_stop_polling(hw->wiphy);
> >> + mutex_lock(&wl->mutex);
> >> if (b43_status(dev) >= B43_STAT_STARTED) {
> >> dev = b43_wireless_core_stop(dev);
> >> if (!dev)
> >
> > OK, but why do we start polling under the lock but stop polling without
> > the lock? Should we start polling without holding the lock too?
>
> I'll test that, but I suspect it doesn't matter. Of course, the reason
> I put the stop under the lock was for symmetry, but then I got the
> following when shutting down:
>
> b43-phy0 debug: Removing Interface type 2
>
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.32-rc3-wl #225
> -------------------------------------------------------
> modprobe/25391 is trying to acquire lock:
> (&(&rfkill->poll_work)->work){+.+...}, at: [<ffffffff81054a7f>]
> __cancel_work_timer+0xd9/0x224
>
> but task is already holding lock:
> (&wl->mutex){+.+.+.}, at: [<ffffffffa02ff3d0>] b43_op_stop+0x30/0x7f
> [b43]
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&wl->mutex){+.+.+.}:
> [<ffffffff81069790>] __lock_acquire+0x140e/0x174d
> [<ffffffff81069b8b>] lock_acquire+0xbc/0xd9
> [<ffffffff8128d420>] mutex_lock_nested+0x58/0x29c
> [<ffffffffa03150ea>] b43_rfkill_poll+0x3a/0xfc [b43]
> [<ffffffffa02c2f33>] ieee80211_rfkill_poll+0x26/0x28 [mac80211]
> [<ffffffffa027c028>] cfg80211_rfkill_poll+0x14/0x16 [cfg80211]
> [<ffffffffa0271081>] rfkill_poll+0x23/0x3d [rfkill]
> [<ffffffff81054224>] worker_thread+0x22c/0x332
> [<ffffffff81057fd8>] kthread+0x7d/0x85
> [<ffffffff8100caba>] child_rip+0xa/0x20
>
> Moving the stop ooutside the lock cured the problem.
>
Just move it right after the existing cancel_work_sync() call
--
Greetings, Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
