Re: [PATCH] mac80211: hoist sta->lock from reorder release timer

[Christian Lamparter <chunkeey@googlemail.com>]

On Thursday 07 October 2010 23:03:13 Johannes Berg wrote:
> On Wed, 2010-10-06 at 16:21 -0400, John W. Linville wrote:
> 
> > > I think it's probably easier to fix than to revert now? There are only a
> > > handful of fields, and it seemed to me that most of them can easily be
> > > moved under the reorder lock.
> > 
> > I would prefer a fix on top rather than a series of reverts...
> 
> I think this should fix it. Somebody review please?
> 
> johannes
> 
Sure, a little bit. The code itself is fine but as you said
the rx_handler code wasn't written for concurrent/delayed
release timer mechanism.

for example:

Because we can't set IEEE80211_RX_RA_MATCH (since 
it interferes with scanning (as explained in
"mac80211: fix release_reorder_timeout in scan").

We will experience strange results with "ieee80211_rx_h_decrypt":

line: 878
>	/*
>	 * No point in finding a key and decrypting if the frame is neither
>	 * addressed to us nor a multicast frame.
>	 */
>	if (!(status->rx_flags & IEEE80211_RX_RA_MATCH))
>		return RX_CONTINUE;
>
>	/* start without a key */
>	rx->key = NULL;
no software decryption there - not nice but the HW probably does
the decryption for us. - That being said, the stack should be able
to do the software decryption "just in case".

Things are a little bit better with ieee80211_rx_h_sta_process.
It updates some statistics and takes care of sta->last_rx
(which is currently not that important giving HT BA is only supported
for AP/STA operation).

In ieee80211_rx_h_data, there could be another potential problem:
>	if (ieee80211_is_data(hdr->frame_control) &&
>   	 !is_multicast_ether_addr(hdr->addr1) &&
>		 local->hw.conf.dynamic_ps_timeout > 0 && local->ps_sdata) {
>			mod_timer(&local->dynamic_ps_timer, jiffies +
>			msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
>	}
I reckon there could be a "hidden" problem. "jiffies" is now
approx 100ms after the packet was received from the interface.
(Sure, a similar issue was also present in the original
reorder release implementation.)

In order the fix this/my mess we would need to:
 1. move the software decryption before the reordering
   (802.11n-spec (page 11, Figure 6-1) allows this)

(Or:
1. introduce an additional rx_flag for the reorder release case?)

(2. maybe cache the original skb jiffie at some place?)

(3. make a few counters atomic_t, so concurrent tasklets
    can update the stats. Or disable the BHs while processing,
    any rx frames (which is probably what we're going to do, right?))

Regards,
	Christian

Unfortunately, I have to do some other "high priority" right now,
so I'm short of time to do "that" now :-/.