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

[Johannes Berg <johannes@sipsolutions.net>]

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


>  struct ieee80211_rx_status {
> @@ -388,6 +389,7 @@ struct ieee80211_rx_status {
>  	int noise;
>  	int antenna;
>  	int rate;
> +	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.
 
> @@ -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?

> +#define SEQ_MODULO 0x1000
> +#define SEQ_MASK   0xfff
> +
> +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?

> +static inline u16 seq_sub(u16 sq1, u16 sq2)
> +{
> +	return ((sq1 - sq2) & SEQ_MASK);
> +}

Same here. And maybe seq_less should use seq_sub and the SEQ_MODULO
define should be (SEQ_MASK+1)?

I think I need to take a second look at this patch to even understand
the problem it solves.

johannes

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