Fix a couple of more bugs in the L2 padding code:
1. Compute the amount of L2 padding correctly (in 2 places).
2. Trim the skb correctly when the L2 padding has been applied.
Signed-off-by: Gertjan van Wingerde <[email protected]>
---
drivers/net/wireless/rt2x00/rt2x00queue.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index b8f0954..562a344 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);
if (header_align == payload_align) {
/*
@@ -216,6 +216,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
memmove(skb->data + header_length + l2pad,
skb->data + header_length + l2pad + payload_align,
frame_length - header_length);
+ skb_trim(skb, frame_length + l2pad);
skbdesc->flags |= SKBDESC_L2_PADDED;
}
}
@@ -346,7 +347,7 @@ static void rt2x00queue_create_tx_descriptor(struct queue_entry *entry,
* Header and alignment information.
*/
txdesc->header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
- txdesc->l2pad = ALIGN_SIZE(entry->skb, txdesc->header_length);
+ txdesc->l2pad = 4 - (txdesc->header_length & 3);
/*
* Check whether this frame is to be acked.
--
1.6.5.3
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index b8f0954..562a344 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);
Not sure about this, the l2pad should be the total bytes between the header and payload.
What if we have a frame for which both the header and the payload should be moved?
The code in this function handles multiple scenarios, one where both the payload and
header must be moved or when only one of both should be moved. In each case the l2pad
depends on both the payload and header alignment sizes.
> if (header_align == payload_align) {
> /*
> @@ -216,6 +216,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
> memmove(skb->data + header_length + l2pad,
> skb->data + header_length + l2pad + payload_align,
> frame_length - header_length);
> + skb_trim(skb, frame_length + l2pad);
> skbdesc->flags |= SKBDESC_L2_PADDED;
> }
> }
> @@ -346,7 +347,7 @@ static void rt2x00queue_create_tx_descriptor(struct queue_entry *entry,
> * Header and alignment information.
> */
> txdesc->header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
> - txdesc->l2pad = ALIGN_SIZE(entry->skb, txdesc->header_length);
> + txdesc->l2pad = 4 - (txdesc->header_length & 3);
ALIGN_SIZE() depends on the actual address of the payload, and thus should be better
and indicating the amount of padding which is required. However the bug might be that we need
to do the same calculation as in rt2x00queue_insert_l2pad() and make it depend on the ALIGN_SIZE
values from header and payload.
Ivo
Gertjan van Wingerde a ?crit :
> On 11/28/09 22:26, Ivo van Doorn wrote:
>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>> index b8f0954..562a344 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);
Humm... is header_length = 24, then your formula gives l2pad = 4. If so,
this is wrong. Do I miss something?
BTW, I'm trying to prepare some patches for rt2800usb and padding. I
must admit the current framework is a bit complex compared to other
drivers (ath9k for instance).
To give something similar to ath9k where the pad position is computed
based on the hdr->frame_control field only (my guess is that's what the
HW does anyway), we have :
int rt2800usb_padpos(__le16 frame_control)
{
int padpos = 24;
if (ieee80211_is_data(frame_control)) {
padpos = ieee80211_hdrlen(frame_control);
}
return padpos;
}
then later : int padsize = padpos & 3;
then the usual check before doing the real padding or unpadding :
if (padsize && skb->len>padpos) {
do padding or unpadding
}
My 2 cents,
Benoit
On 11/29/09 00:55, Benoit PAPILLAULT wrote:
> Gertjan van Wingerde a ?crit :
>> On 11/28/09 22:26, Ivo van Doorn wrote:
>>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>>> index b8f0954..562a344 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);
>
> Humm... is header_length = 24, then your formula gives l2pad = 4. If so,
> this is wrong. Do I miss something?
No, you are right. The formula needs another & 3 on the overall result to account for that situation.
So, it should be:
unsigned int l2pad = (4 - (header_length & 3)) & 3;
>
> BTW, I'm trying to prepare some patches for rt2800usb and padding. I
> must admit the current framework is a bit complex compared to other
> drivers (ath9k for instance).
>
> To give something similar to ath9k where the pad position is computed
> based on the hdr->frame_control field only (my guess is that's what the
> HW does anyway), we have :
>
> int rt2800usb_padpos(__le16 frame_control)
> {
> int padpos = 24;
> if (ieee80211_is_data(frame_control)) {
> padpos = ieee80211_hdrlen(frame_control);
> }
> return padpos;
> }
>
> then later : int padsize = padpos & 3;
>
> then the usual check before doing the real padding or unpadding :
> if (padsize && skb->len>padpos) {
> do padding or unpadding
> }
>
To be honest, I don't think padding sizes depend the type of frame. It is just a matter of the
header_length. So, I don't think we have to complicate this by looking at the type of frame.
---
Gertjan.
Gertjan van Wingerde <[email protected]>
writes:
> On 11/29/09 00:55, Benoit PAPILLAULT wrote:
>> Gertjan van Wingerde a écrit :
>>> On 11/28/09 22:26, Ivo van Doorn wrote:
>>>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>>>>> index b8f0954..562a344 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);
>>
>> Humm... is header_length = 24, then your formula gives l2pad = 4. If so,
>> this is wrong. Do I miss something?
>
> No, you are right. The formula needs another & 3 on the overall result to account for that situation.
> So, it should be:
>
> unsigned int l2pad = (4 - (header_length & 3)) & 3;
aka
unsigned int l2pad = -header_length & 3;
Andreas.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
On 11/28/09 22:26, Ivo van Doorn wrote:
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> index b8f0954..562a344 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);
>
> Not sure about this, the l2pad should be the total bytes between the header and payload.
> What if we have a frame for which both the header and the payload should be moved?
That is taken into consideration here. The beauty of this is that in order to calculate the
number of bytes between the header and the payload you don't even have to consider the current
locations of both in the buffer, because it is only a function of the header length. I.e. if a
header is 24 bytes long, then no L2 padding is needed, as a properly aligned header will result
in a properly aligned payload. If a header is 26 bytes long, then 2 bytes of L2 padding is needed
as there are 2 bytes between the end of the header and the next 4-bytes boundary. This is all
independent of whether the current header and the current payload are properly aligned.
So, the value of l2pad shouldn't be used to determine if any alignment needs to be done, it only
will tell you how many padding bytes will be required between the header and the payload.
Also, note that the same formula is used in the rt2x00queue_remove_l2pad function, and this is basically
copied from there.
>
> The code in this function handles multiple scenarios, one where both the payload and
> header must be moved or when only one of both should be moved. In each case the l2pad
> depends on both the payload and header alignment sizes.
>
>> if (header_align == payload_align) {
>> /*
>> @@ -216,6 +216,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
>> memmove(skb->data + header_length + l2pad,
>> skb->data + header_length + l2pad + payload_align,
>> frame_length - header_length);
>> + skb_trim(skb, frame_length + l2pad);
>> skbdesc->flags |= SKBDESC_L2_PADDED;
>> }
>> }
>> @@ -346,7 +347,7 @@ static void rt2x00queue_create_tx_descriptor(struct queue_entry *entry,
>> * Header and alignment information.
>> */
>> txdesc->header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
>> - txdesc->l2pad = ALIGN_SIZE(entry->skb, txdesc->header_length);
>> + txdesc->l2pad = 4 - (txdesc->header_length & 3);
>
> ALIGN_SIZE() depends on the actual address of the payload, and thus should be better
> and indicating the amount of padding which is required. However the bug might be that we need
> to do the same calculation as in rt2x00queue_insert_l2pad() and make it depend on the ALIGN_SIZE
> values from header and payload.
>
Yep, ALIGN_SIZE does. But the number of l2pad bytes doesn't depend on the actual address of the
payload, it only depends on the length of the header. See above for the explanation.
---
Gertjan.