| Message ID | 1396470747-22892-1-git-send-email-seth.forshee@canonical.com |
|---|---|
| State | New |
| Headers | show |
Looks sensible to me. I don't know the code in detail but removing hw related things sounds like something better be done in a higher layer... Worst case there is some unused structure leaked which would be less fatal than dead locking.
On Wed, Apr 02, 2014 at 03:32:27PM -0500, Seth Forshee wrote: > From: Emil Goode <emilgoode@gmail.com> > > When brcm80211 firmware is not installed networking hangs. > A deadlock happens because we call ieee80211_unregister_hw() > from the .start callback of struct ieee80211_ops. When .start > is called we are under rtnl lock and ieee80211_unregister_hw() > tries to take it again. > > Function call stack: > > dev_change_flags() > __dev_change_flags() > __dev_open() > ASSERT_RTNL() <-- Assert rtnl lock > ops->ndo_open() > > .ndo_open = ieee80211_open, > > ieee80211_open() > ieee80211_do_open() > drv_start() > local->ops->start() > > .start = brcms_ops_start, > > brcms_ops_start() > brcms_remove() > ieee80211_unregister_hw() > rtnl_lock() <-- Here we deadlock > > Introduced by: > commit 25b5632fb35ca61b8ae3eee235edcdc2883f7a5e > ("brcmsmac: request firmware in .start() callback") > > This patch fixes the bug by removing the call to brcms_remove() > and moves the brcms_request_fw() call to the top of the .start > callback to not initiate anything unless firmware is installed. > > Signed-off-by: Emil Goode <emilgoode@gmail.com> > Signed-off-by: John W. Linville <linville@tuxdriver.com> > (cherry picked from commit 8fc1e8c240aab968db658b2d8d079b4391207a36 > git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-testing.git) > BugLink: http://bugs.launchpad.net/bugs/1300416 > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > --- > drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c > index edc5d10..03a56df 100644 > --- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c > +++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c > @@ -426,6 +426,12 @@ static int brcms_ops_start(struct ieee80211_hw *hw) > bool blocked; > int err; > > + if (!wl->ucode.bcm43xx_bomminor) { > + err = brcms_request_fw(wl, wl->wlc->hw->d11core); > + if (err) > + return -ENOENT; > + } > + > ieee80211_wake_queues(hw); > spin_lock_bh(&wl->lock); > blocked = brcms_rfkill_set_hw_state(wl); > @@ -433,14 +439,6 @@ static int brcms_ops_start(struct ieee80211_hw *hw) > if (!blocked) > wiphy_rfkill_stop_polling(wl->pub->ieee_hw->wiphy); > > - if (!wl->ucode.bcm43xx_bomminor) { > - err = brcms_request_fw(wl, wl->wlc->hw->d11core); > - if (err) { > - brcms_remove(wl->wlc->hw->d11core); > - return -ENOENT; > - } > - } > - > spin_lock_bh(&wl->lock); > /* avoid acknowledging frames before a non-monitor device is added */ > wl->mute_tx = true; Looks sane to check for the firmware early and bail without initialising at all. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c index edc5d10..03a56df 100644 --- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c +++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c @@ -426,6 +426,12 @@ static int brcms_ops_start(struct ieee80211_hw *hw) bool blocked; int err; + if (!wl->ucode.bcm43xx_bomminor) { + err = brcms_request_fw(wl, wl->wlc->hw->d11core); + if (err) + return -ENOENT; + } + ieee80211_wake_queues(hw); spin_lock_bh(&wl->lock); blocked = brcms_rfkill_set_hw_state(wl); @@ -433,14 +439,6 @@ static int brcms_ops_start(struct ieee80211_hw *hw) if (!blocked) wiphy_rfkill_stop_polling(wl->pub->ieee_hw->wiphy); - if (!wl->ucode.bcm43xx_bomminor) { - err = brcms_request_fw(wl, wl->wlc->hw->d11core); - if (err) { - brcms_remove(wl->wlc->hw->d11core); - return -ENOENT; - } - } - spin_lock_bh(&wl->lock); /* avoid acknowledging frames before a non-monitor device is added */ wl->mute_tx = true;