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.
> >> 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.
> > 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.
> >> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
> >> > "originating device\n", dev->name);
> >> > #endif
> >> > dev_kfree_skb(skb);
> >> > - return 0;
> >> > + return NETDEV_TX_OK;
> >>
> >> All these NETDEV_TX_OK could've gone into a separate patch.
> >
> > I guess, they're also not strictly necessary since 0 == TX_OK :)
>
> Sure, what I'm trying to say is these patches are pretty hard to
> review because of the topic,
> it just helps to trim them down as much as possible -- but I realize
> even that is painful.
:)
Thanks man, I really appreciate you giving them a close look!
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
