Re: [PATCH 5/8] mac80211: A-MPDU Rx handling aggregation reordering

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 3534 bytes --]


> > > +     int ordered;
> >
> > That's not an int, and it's only used internally. Can we somehow avoid
> > putting it into the rx status, have it in the txrx status structure
> > instead and give it the proper type (txrx result)? More of a question
> > than a suggestion, I'd like to have that but I'm not sure it's doable.
> 
> well, i was asking myself this same question, and went to
> ieee80211_rx_status as ieee80211_txrx_data derives its data from
> ieee80211_rx_status in __ieee80211_rx, so i have no other way to pass
> info per sk_buff.

Ok.

> type int was chosen as it is not a bool type (can be
> drop/continue/queue) so it can't be fit to flags.

No, I was thinking that it's enum txrx_result or whatever that is named.
But you can't use that type in mac80211.h since it's only defined in
ieee80211_i.h... Maybe you should put the declaration into mac80211.h
and use it here so that the compiler can error check this ordered field
better...

> > > @@ -1252,6 +1254,20 @@ void ieee80211_sta_stop_rx_BA_session(struct net_device *dev, u8 *ra, u16 tid,
> > >               ieee80211_send_delba(dev, ra, tid, 0, reason);
> > >
> > >       /* free the reordering buffer */
> > > +     for (i = 0; i < sta->ampdu_mlme.tid_rx[tid].buf_size; i++) {
> > > +             if (sta->ampdu_mlme.tid_rx[tid].reorder_buf[i]) {
> > > +                     /* release the reordered frames to stack,
> > > +                      * but drop them there */
> > > +                     memcpy(&status, sta->ampdu_mlme.tid_rx[tid].
> > > +                             reorder_buf[i]->cb, sizeof(status));
> > > +                     status.ordered = TXRX_DROP;
> > > +                     __ieee80211_rx(hw, sta->ampdu_mlme.
> > > +                                    tid_rx[tid].reorder_buf[i],
> > > +                                    &status);
> >
> > This is strange. Why are they even passed to __ieee80211_rx?
> 
> of course i can get rid from there here, but thought it will be more
> systematic and clearer to pass them back to __ieee80211_rx. if you
> have any objection to this (efficiency considerations or like) i will
> reconsider it.

I'm just not sure why you'd want to. As far as I can the frames already
passed __ieee80211_rx(), no? Maybe only as part of the aggregation?

> > > +static inline int seq_less(u16 sq1, u16 sq2)
> > > +{
> > > +     return (((sq1 - sq2) & SEQ_MASK) > (SEQ_MODULO >> 1));
> >
> > Is it really correct to subtract before masking?
> 
> as these values are already "masked" as sequence numbers the answer is yes.
> if this is not the case, and somehow we deal internally with sequence
> numbers > 4095 this function should be the least to worry about  ;-)

Heh, ok, I didn't really look where it was used.

> the fact these 2 #defines are 1 number apart shouldn't confuse. it is
> just a more
> convenient way for the 802.11 reader (used to sequence number limit)
> to look at it. i could have, if i would like, decide that pass
> criteria for seq_less is not 2048, but only the closest 200 frames, in
> that case i would have give it the value of 200.

Ah, ok. Makes sense, can you add a comment?

> > I think I need to take a second look at this patch to even understand
> > the problem it solves.
> >
> 
> no one guarantees you get the frames in the right order... please see
> the patch description for such a case.

Ok, right. Still not quite sure I've understand the solution, I'll take
another look.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]