On 6 June 2014 08:31, Coelho, Luciano <luciano.coelho@xxxxxxxxx> wrote: > On Fri, 2014-06-06 at 07:51 +0200, Michal Kazior wrote: >> On 5 June 2014 18:26, Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx> wrote: [...] >> > @@ -3146,9 +3145,8 @@ static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata) >> > cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef); >> > >> > if (!ieee80211_csa_needs_block_tx(local)) >> > - ieee80211_wake_queues_by_reason(&local->hw, >> > - IEEE80211_MAX_QUEUE_MAP, >> > - IEEE80211_QUEUE_STOP_REASON_CSA); >> > + ieee80211_wake_vif_queues(local, sdata, >> > + IEEE80211_QUEUE_STOP_REASON_CSA); >> >> I don't think this is going to work with the upcomming multi-vif >> channel switching. >> >> The ieee80211_csa_needs_block_tx() will become false upon *last* >> interface switch completion. This means preceeding interface csa >> finalizations won't call ieee80211_wake_vif_queues() so you actually >> end up not waking all of the queues when you finish csa. > > I think the right way to do it would be to traverse the vifs that > switched and wake up their queues when the switch is finished. > > >> I think this can be solved with: >> >> if (!ieee80211_csa_needs_block_tx(local)) >> list_for_each(sdata, &local->interfaces, list) >> ieee80211_wake_vif_queues(local, sdata, REASON_CSA); >> >> But then I guess it's just a convoluted way of saying: >> >> if (!ieee80211_csa_needs_block_tx(local)) >> ieee80211_wake_queues_by_reason(hw, REASON_CSA); >> >> IOW waking should remain as it was while stopping can be done per-vif. >> >> This goes for all instances calling wake_vif_queues() in the patch. > > Actually I had it like this in my first version, I had only added the > stop part, leaving the wake as it was. But then I thought that we would > be restarting queues from other vifs that we don't know about. If > another vif is switching separately, we would restart its queues at the > wrong time. I think we shouldn't assume that all vifs are part of the > same switch. Well, not really. There is no "vif switching separately" when it comes to block_tx. The condition checks if ANY interface still needs queues to be locked for the *REASON_CSA*. If the condition is false (i.e. "no interface needs block_tx") then you are safe to wake *all* queues for *REASON_CSA* on all interfaces because channel switches that don't need block_tx don't stop their queues for REASON_CSA in the first place. You could argue current queue locking for CSA is not efficient and you'd be right. It was a simplification I came up with to get things at least work correctly not efficiently. >> Generally I don't think having wake_vif_queues() is a good idea. It's >> really tricky. Sure, you can have a stop_vif_queues() which will >> behave as you might expect (i.e. first call stops queues and >> overlapping vif-queue mappings are not a concern). But >> wake_vif_queues() seems to have little practical use to me without any >> kind of per-queue reference counting. > > Yeah, this could be a problem for vifs that are sharing the same queues. > But that case would be really tricky with channel-switch anyway. because > if the channel of one vif is changed but not the other, the queue would > probably have to be split... Hmm. That's a very good point. Can we even re-map this during the switch now? If we can't then doesn't it imply that if there are shared vif-queue mappings then channel switching should be limited/prevented? I.e. it might be okay if you have 2 vifs sharing a queue to switch to the exact same channel but it won't work if you try to switch them to separate channels. Single-channel devices won't have to worry about it (single-channel combination cases for that matter). Michał -- 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
