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 :-/.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
