On Wed, 2013-02-20 at 06:24 -0800, Ben Greear wrote: > On 02/20/2013 06:13 AM, Johannes Berg wrote: > > On Wed, 2013-02-20 at 06:09 -0800, Ben Greear wrote: > > > >>>> @@ -2668,6 +2681,10 @@ static void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, > >>>> false, frame_buf); > >>>> mutex_unlock(&ifmgd->mtx); > >>>> > >>>> + /* Have to do this outside the ifmgd->mtx lock. */ > >>>> + cancel_work_sync(&ifmgd->monitor_work); > >>>> + cancel_work_sync(&ifmgd->beacon_connection_loss_work); > >>> > >>> OTOH, you do this many many times, and that doesn't seem necessary... > >>> > >>> If the work structs run when we disconnected, that's ok, they just > >>> musn't run after we destroy the interface, so I think it'd be much > >>> better to just put the two lines into ieee80211_mgd_stop() instead of > >>> all the other places. > >> > >> Ok, that sounds promising to me. For that matter, could we just call the sta_quiesce > >> method in mgd_stop? It would be nice to have all of the final timer and work-queue cleanup > >> in a single place. > > > > That might be interesting, but I think we'd have to clear the > > timer_pending thing afterwards? > > Are you talking about this code? > > if (del_timer_sync(&ifmgd->timer)) > set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); > > if (del_timer_sync(&ifmgd->chswitch_timer)) > set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); > > If so, maybe make a helper method that does all of the first part of the sta_quiesce() > logic, call that from both mgd_stop and the new sta_quiesce() method, and then have > sta_quiesce look like: > > sta_cleanup_timers_and_work(); > if (del_timer_sync(&ifmgd->timer)) > set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); > > if (del_timer_sync(&ifmgd->chswitch_timer)) > set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); What part of that would then be "cleanup_timers"? No, I think you should just look at it and set timers_running = 0 after calling the function, or so. johannes -- 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
