> > > + 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:
signature.asc
Description: This is a digitally signed message part
