On Fri, 2014-06-06 at 08:44 +0200, Michal Kazior wrote:
> 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.
Right, and this used to make sense when we were stopping *all* hw_queues
when we had block_tx.
Maybe we should make a new function that returns all the queues that our
vif was blocking but that are not blocked by any other vifs. Maybe
something like this:
static u32 ieee80211_vif_queues_to_wake(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
enum queue_stop_reason reason)
{
struct ieee80211_sub_if_data *sdata_iter;
unsigned long queues, queues_iter, q;
int i;
lockdep_assert_held(&local->mtx);
queues = ieee80211_get_vif_queues(local, sdata);
rcu_read_lock();
list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
if (!ieee80211_sdata_running(sdata_iter))
continue;
queues_iter = ieee80211_get_vif_queues(local, sdata_iter);
for_each_set_bit(i, &queues_iter, local->hw.queues) {
q = sdata->vif.hw_queue[i];
if (test_bit(reason, &local->queue_stop_reasons[q]))
queues &= ~q;
}
}
rcu_read_unlock();
return queues;
}
void ieee80211_wake_vif_queues(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
enum queue_stop_reason reason)
{
unsigned int queues = ieee80211_vif_queues_to_wake(local, sdata, reason);
ieee80211_wake_queues_by_reason(&local->hw, queues, reason);
}
> 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).
Yeah, it should probably be prevented for such drivers. But then, at
what point? Prevent the channel switch to occur completely if another
vif shares the same queue? Or fail the switch if the other vif doesn't
join the switch?
--
Luca.
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
