On Sat, Mar 28, 2009 at 12:52 PM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> On Sat, 2009-03-28 at 12:18 -0700, Luis R. Rodriguez wrote:
>
>> >> That's fine from a logical point of view as compromise here as our
>> >> ampdu start should be relatively quick. Now while I can say this
>> >> from a logical point of view this patch obviously needed more
>> >> testing but lets see how it holds up and I'm glad you're the one
>> >> who wrote it. I'm confident there won't be any issues but that
>> >> is something I cannot gaurantee just based on the review. We now
>> >> need to go out and tests this. All other patches previous to this
>> >> make 100% perfect sense.
>> >
>> > :)
>> > I think it also makes more _sense_ to not ask the driver to handle these
>> > frames, because we won't set the AMPDU flag correctly if the session
>> > start is declined.
>>
>> Hm, wouldn't we send them as normal frames if its declined here eventually?
>
> Yes, we take them off the sta->pending, put them through TX processing
> and send them as normal frames.
OK so you're not suggesting we change this more, just stating that
this makes sense.
>> >> This piece:
>> >>
>> >> > + } else if (*state != HT_AGG_STATE_IDLE) {
>> >> > + /* in progress */
>> >> > + queued = true;
>> >> > + info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> >> > + __skb_queue_tail(&tid_tx->pending, skb);
>> >> > + }
>> >>
>> >> Can probably be ported to stable too, to just TX_DROP.
>> >
>> > No, why? We're handing the frames to the driver. It wants to handle
>> > those frames before agg session is started correctly. That's what I was
>> > referring to before with this making more sense now.
>>
>> OK I see that but I am not sure of what's done to the skb in the old
>> case if the session is note complete yet, nor now if the session gets
>> declined. I can check later, right now I'm feeling lazy.
>
> Before my patch the skb is still passed to the driver, which would need
> to be able to handle it (it == session being stopped). I'm not entirely
> sure ath9k handles it properly but I suspect it does.
Not sure, this begs the question when you were seeing the received
addba response come up first. SInce both iwlagn and ath9k use the irq
safe aggregation cb I was suspecting this would happen in general on
UP boxen and probably even less likely to happen on ath9k as we don't
have to talk to the firmware during the ampdu start action.
>> > Ok that might make some sense, though as a parameter you can easily see
>> > where it's coming from, I think :) I agree that the entire code is a
>> > little brain twister...
>>
>> Yeah.. I had to re-read aggregation code again to get the hang of it.
>> Anything we can do to make
>> it easier for people to pick would be nice. I think I'll go patch up
>> the aggr-tx kdoc too now, unless we
>> plan on nuking some stuff soon again. I suspect the next big change
>> will be to move software based
>> aggregation queue handling and mapping to ACs within mac80211 for that
>> type of hardware which
>> we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
>> and our next generation 11n USB
>> driver.
>
> Right -- but I don't expect to be working on it any time soon myself.
I didn't expect you to. We may have the need first unless chr gets to
ar9170 stuff first.
Luis
--
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
