On Fri, 2014-06-06 at 09:56 +0200, Michal Kazior wrote:
> On 6 June 2014 09:37, Coelho, Luciano <luciano.coelho@xxxxxxxxx> wrote:
> > 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);
> > }
>
> Once you set local->queue_stop_reasons[] for an overlapping queue you
> won't be able to wake it with ieee80211_wake_vif_queues() alone.
> You'll need to call ieee80211_wake_queues() to do that. Is that what
> you intended?
More or less. My idea is that only the *last* vif that tries to wake
the overlapping queue, will actually do so.
> >> >> 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?
>
> That's a good question too. I would prefer to leave it until the last
> second, e.g. give the opportunity to re-map in
> drv_switch_vif_chanctx() maybe so driver can fail if it's unable to
> comply?
Yes, I tend to agree.
--
Luca.
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
