Re: [rt2x00-users] [PATCH v2] rt2x00: Further L2 padding fixes.

[Benoit PAPILLAULT <benoit.papillault@free.fr>]

Ivo Van Doorn a écrit :
> On Sun, Nov 29, 2009 at 12:47 PM, Gertjan van Wingerde
> <gwingerde@gmail.com> wrote:
>> Fix a couple of more bugs in the L2 padding code:
>> 1. Compute the amount of L2 padding correctly (in 3 places).
>> 2. Trim the skb correctly when the L2 padding has been applied.
>>
>> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
>> ---
>>
>> v2:
>> - Fix bug detect by Benoit Papillault.
>>
>> ---
>>  drivers/net/wireless/rt2x00/rt2x00queue.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> index b8f0954..9082fad 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> @@ -181,7 +181,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
>>        unsigned int frame_length = skb->len;
>>        unsigned int header_align = ALIGN_SIZE(skb, 0);
>>        unsigned int payload_align = ALIGN_SIZE(skb, header_length);
>> -       unsigned int l2pad = 4 - (payload_align - header_align);
>> +       unsigned int l2pad = (4 - (header_length & 3)) & 3;
> 
> We already have this calculation on multiple locations, so perhaps it
> is better to have
> a L2PAD_SIZE macro in rt2x00.h (where ALIGN_SIZE() is also defined).
> 
> Ivo

The formula can be simplified to l2pad = header_length & 3.

I'll sent a patch for unpadding on the RX path that I have tested with
rt2870 usb (I'm still working on the TX path). My feeling is that we are
 not doing the correct computation.

l2pad is the number of padding bytes added by the HW. It is computed
here based on the header_length. However, doing so, we missed two points  :

1. ACK frame is 10 bytes. So header_length will be 10 and then l2pad is
2. However, ACK frames do not need padding, only DATA frames needs
padding. The above formula is then wrong.

2. If we receive a QoS DATA frame whose header_length is 26 according to
frame_control, but skb->len = 20, then ieee80211_get_hdrlen_from_skb()
will returns hdrlen = 0 based on the value of skb->len. However, this
function does not know if skb->len includes said padding or not, or some
other padding! (On RX, rt2870 usb frames are also padded at the end!).

For those reason, I think it's better to have a common function that
computes the padding position (based on frame_control). Once we know the
padding position, we can compute padding size (= padding position & 3,
like above) and we can also compare with the real frame size (contained
in RXWI_W0_MPDU_TOTAL_BYTE_COUNT for instance).

Regards,
Benoit