Re: [PATCH] mac80211: Clean up work-queues on disassociation.

[Johannes Berg <johannes@sipsolutions.net>]

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