> > > +EXPORT_SYMBOL(ieee80211_start_tx_ba_session);
> >
> > Why is this exported? I thought this is the first step of enabling
> > aggregation?
>
> this is the trigger to aggregation, but a load aware entity should
> call it. in my next series of patches i'll work on this issue, but
> basically i would like to give the option to start/stop aggregations
> to other modules (e.g. rate scaling module)
Ah. Let only export it then though, please put the export into that
patch series. Maybe a rate control algorithm should indicate this in
some other way via the callbacks anyway?
> > > + /* Tell the driver to stop its HW queue */
> > > + if (local->ops->ampdu_action)
> > > + ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_TX_STOP,
> > > + ra, tid, NULL);
> >
> > Does it really stop the hw queue? Isn't that more like "stop
> > aggregation"? Does the hardware actually have queues for each of these?
> > Hm. I'm familiar enough with this I think.
> >
>
> I'll remove this comment anyhow, as it is misleading, but the basic
> idea is what i wrote in 0/10 - first we need to drain all A-MPDUs,
> only then we can delBA, otherwise we are confusing the receiver.
Ok, I'm trying to learn 11n here too, does the hardware need extra
queues for the A-MPDU feature?
> > > + /* case HW denied going back to legacy */
> > > + if (ret) {
> > > + printk(KERN_ERR "Driver could not stop aggregations\n");
> > > + *state = HT_AGG_STATE_OPERATIONAL;
> > > + ieee80211_wake_queue(hw, sta->tid_to_tx_q[tid]);
> > > + goto stop_BA_exit;
> >
> > Is there really a use case for a driver denying this? IOW, can you think
> > of a good reason why a driver would deny it?
>
> can't think about one right now, but i am not familiar with other HWs.
> maybe a self contained HW decision based on frames load can lead to
> that. in any case i would like to give the driver this option.
Ok. Let's put
WARN_ON(ret != -EBUSY)
into that if (ret) { block though to force the driver to give exactly
that error code (and document that) so that callers of
ieee80211_stop_tx_ba_session() can discover that the driver failed.
> > > + /* avoid ordering issues: we are the only one that can modify
> > > + * the content of the qdiscs */
> > > + spin_lock_bh(&local->mdev->queue_lock);
> > > + /* remove the queue for this aggregation */
> > > + ieee80211_ht_agg_queue_remove(local, sta, tid, 1);
> > > + spin_unlock_bh(&local->mdev->queue_lock);
> > > +
> > > + /* Not scheduling the device leads to a stall in the TX when qdisc */
> > > + /* contains more than ~220 packets... */
> > > + netif_schedule(local->mdev);
> >
> > This seems a bit weird, can you explain a bit more?
>
> i'll change the comment here to a clearer one. since the qdisc has
> just been requeued then heavy load (~220 frames queued) might lead us
> to miss a softirq for this qdisc. we can't use ieee80211_wake_queue as
> the qdisc is not necessarily stopped, so this is a solution for it.
Ok. I don't think I've fully understood it yet, but I haven't really
looked into the qdisc stuff at all yet.
> this is a use of an existing mechanism, I wouldn't like another queue
> just for this purpose, though a misuse, i agree. i can either stay
> with this code, use cb for this purpose, or create a new struct in
> mac80211.h, somehting like ieee80211_ra_tid and pass it. what do you
> think os the best?
I think we should use something other than an skb queue here with some
other struct, that other struct would be in skb->cb for the existing skb
cases and would just sit in some new struct ieee80211_ra_tid too for
this case. It's a bit of rework here but I think it'd help the code to
be more readable. Not sure if we should also do it for the unreliable
queue.
> > > + /* check if the TID waits for addBA response */
> > > + spin_lock_bh(&sta->ampdu_mlme.ampdu_tx);
> > > + if (!(*state & HT_ADDBA_REQUESTED_MSK)) {
> > > + spin_unlock_bh(&sta->ampdu_mlme.ampdu_tx);
> > > + *state = HT_AGG_STATE_IDLE;
> > > + printk(KERN_DEBUG "timer expired on tid %d but we are not "
> > > + "expecting addBA response there", tid);
> > > + goto timer_expired_exit;
> > > + }
> >
> > Couldn't this also happen when the response was just received while the
> > timer expires but the response grabs the ampdu_tx spinlock before the
> > timer does? Seems that should be in a comment and not worry about a
> > printk.
>
> the condition is on whether addBA request was sent or not for this
> address and TID, not on the addBA request.
Oh, hmm, ok. But that actually leads me to another problem,
HT_ADDBA_REQUESTED_MSK is never cleared anywhere.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
