On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote: > It was possible for tx queues to be stuck locked > if AP CSA finalization failed. I think it's clearer if you say "tx queue stopping" or something to avoid confusion with the word "locking". Same applies to the commit subject. [...] > It is still possible to have tx queues stopped > after CSA failure but as soon as offending > interfaces are stopped from userspace (stop_ap or > ifdown) tx queues are woken up properly. Isn't there any way to avoid this? I mean, if you have identified some cases, why not fix them too? :) [...] > @@ -3092,8 +3122,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work) > goto unlock; > > ieee80211_csa_finalize(sdata); > + if (!ieee80211_csa_needs_block_tx(local)) > + ieee80211_wake_queues_by_reason(&local->hw, > + IEEE80211_MAX_QUEUE_MAP, > + IEEE80211_QUEUE_STOP_REASON_CSA); This should remain inside ieee80211_csa_finalize() as it was before, because otherwise you won't wake up the queues in case of an immediate switch. Look at the bottom of your new __ieee80211_channel_switch(), we call ieee80211_csa_finalize() directly if the beacon didn't change. Actually, in the beacon-didn't-change case, we should probably not even stop the queues? [...] > @@ -3271,15 +3307,15 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, > return err; > > sdata->csa_radar_required = params->radar_required; > - > - if (params->block_tx) > - ieee80211_stop_queues_by_reason(&local->hw, > - IEEE80211_MAX_QUEUE_MAP, > - IEEE80211_QUEUE_STOP_REASON_CSA); > - > sdata->csa_chandef = params->chandef; > + sdata->csa_block_tx = params->block_tx; > sdata->vif.csa_active = true; > > + if (sdata->csa_block_tx) > + ieee80211_wake_queues_by_reason(&local->hw, > + IEEE80211_MAX_QUEUE_MAP, > + IEEE80211_QUEUE_STOP_REASON_CSA); Shouldn't this be *stop* queues? You can call ieee80211_stop_queues_by_reason() even if it is already stopped for this reason, but maybe you could call ieee80211_csa_needs_block_tx() here to avoid eventually calling it multiple times. I think this is also a bit more consistent. if(ieee80211_csa_needs_block_tx(...)) ieee80211_stop_queues_by_reason(...) [...] > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c > index bbc2175..2ca1472 100644 > --- a/net/mac80211/mlme.c > +++ b/net/mac80211/mlme.c > @@ -952,15 +952,18 @@ static void ieee80211_chswitch_work(struct work_struct *work) > /* XXX: shouldn't really modify cfg80211-owned data! */ > ifmgd->associated->channel = sdata->csa_chandef.chan; > > - /* XXX: wait for a beacon first? */ > - ieee80211_wake_queues_by_reason(&local->hw, > - IEEE80211_MAX_QUEUE_MAP, > - IEEE80211_QUEUE_STOP_REASON_CSA); > - > ieee80211_bss_info_change_notify(sdata, changed); > > out: > + mutex_lock(&local->mtx); > sdata->vif.csa_active = false; > + /* XXX: wait for a beacon first? */ > + if (!ieee80211_csa_needs_block_tx(local)) > + ieee80211_wake_queues_by_reason(&local->hw, > + IEEE80211_MAX_QUEUE_MAP, > + IEEE80211_QUEUE_STOP_REASON_CSA); > + mutex_unlock(&local->mtx); Instead of moving this to the out label, I guess the right place for it would be in ieee80211_set_disassoc(). Then we would catch the csa_connection_drop_work cases plus a few other cases that seem to be missing(?). [...] > @@ -1077,12 +1080,16 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata, > mutex_unlock(&local->chanctx_mtx); > > sdata->csa_chandef = csa_ie.chandef; > + > + mutex_lock(&local->mtx); > sdata->vif.csa_active = true; > + sdata->csa_block_tx = csa_ie.mode; > > - if (csa_ie.mode) > + if (sdata->csa_block_tx) > ieee80211_stop_queues_by_reason(&local->hw, > - IEEE80211_MAX_QUEUE_MAP, > - IEEE80211_QUEUE_STOP_REASON_CSA); > + IEEE80211_MAX_QUEUE_MAP, > + IEEE80211_QUEUE_STOP_REASON_CSA); > + mutex_unlock(&local->mtx); Same thing as before, maybe it's more consistent to call ieee80211_csa_needs_block_tx()? [...] > @@ -2035,10 +2043,14 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata) > WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY, > true, frame_buf); > ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED; > + > + mutex_lock(&local->mtx); > sdata->vif.csa_active = false; > - ieee80211_wake_queues_by_reason(&sdata->local->hw, > + if (!ieee80211_csa_needs_block_tx(local)) > + ieee80211_wake_queues_by_reason(&local->hw, > IEEE80211_MAX_QUEUE_MAP, > IEEE80211_QUEUE_STOP_REASON_CSA); > + mutex_unlock(&local->mtx); As I said before, it's probably better to do this in ieee80211_set_disassoc(). But maybe in a separate patch? -- Luca. -- 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
