2010-12-20 08:36:23

by Ismael Luceno

Subject: [PATCH 1/2] rt2x00: Write the frame *after* padding (rt2800 usb devices)

Signed-off-by: Ismael Luceno <[email protected]>
---
drivers/net/wireless/rt2x00/rt2800usb.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 3e0205d..85185de 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -378,9 +378,10 @@ static void rt2800usb_write_tx_data(struct queue_entry *entry,
* | TXINFO | TXWI | 802.11 header | L2 pad | payload | pad | USB end pad |
* |<------------- tx_pkt_len ------------->|
*/
- rt2800_write_tx_data(entry, txdesc);
- padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
- memset(skb_put(entry->skb, padding_len), 0, padding_len);
+ padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
+ memset(skb_put(entry->skb, padding_len), 0, padding_len);
+
+ rt2800_write_tx_data(entry, txdesc);
}

/*
--
1.7.3.4

2010-12-20 11:45:43

by Ivo Van Doorn

Subject: Re: [PATCH 2/2] rt2x00: Fix panic on frame padding for rt2800 usb devices (due to not enough skb tailroom)

Hi,

> Signed-off-by: Ismael Luceno <[email protected]>

Please add a short description why this change is needed,
I only know why because of a discussion over private mail...

> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index 85185de..d284e1e 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -369,7 +369,8 @@ static void rt2800usb_write_tx_desc(struct queue_entry *entry,
> ?static void rt2800usb_write_tx_data(struct queue_entry *entry,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct txentry_desc *txdesc)
> ?{
> - ? ? ? u8 padding_len;
> + ? ? ? unsigned int padding_len;
> + ? ? ? int err;
>
> ? ? ? ?/*
> ? ? ? ? * pad(1~3 bytes) is added after each 802.11 payload.
> @@ -379,8 +380,14 @@ static void rt2800usb_write_tx_data(struct queue_entry *entry,
> ? ? ? ? * ? ? ? ? ? ? ? ? |<------------- tx_pkt_len ------------->|
> ? ? ? ? */
> ? ? ? ?padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
> - ? ? ? memset(skb_put(entry->skb, padding_len), 0, padding_len);
>
> + ? ? ? err = skb_pad(entry->skb, padding_len);
> + ? ? ? if (unlikely(err)) {
> + ? ? ? ? ? ? ? WARNING(entry->queue->rt2x00dev, "TX SKB padding error, out of memory\n");
> + ? ? ? ? ? ? ? return;
> + ? ? ? }

Please use skb_padto instead, that will actually check if the padding
is needed or not...

Ivo

2010-12-20 10:31:35

by Helmut Schaa

Subject: Re: [PATCH 1/2] rt2x00: Write the frame *after* padding (rt2800 usb devices)

Am Montag, 20. Dezember 2010 schrieb Julian Calaby:
> On Mon, Dec 20, 2010 at 19:55, Helmut Schaa <[email protected]> wrote:
> > Hi,
> >
> > Am Montag, 20. Dezember 2010 schrieb Ismael Luceno:
> >> Signed-off-by: Ismael Luceno <[email protected]>
> >> ---
> >> drivers/net/wireless/rt2x00/rt2800usb.c | 7 ++++---
> >> 1 files changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> >> index 3e0205d..85185de 100644
> >> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> >> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> >> @@ -378,9 +378,10 @@ static void rt2800usb_write_tx_data(struct queue_entry *entry,
> >> * | TXINFO | TXWI | 802.11 header | L2 pad | payload | pad | USB end pad |
> >> * |<------------- tx_pkt_len ------------->|
> >> */
> >> - rt2800_write_tx_data(entry, txdesc);
> >> - padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
> >> - memset(skb_put(entry->skb, padding_len), 0, padding_len);
> >> + padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
> >> + memset(skb_put(entry->skb, padding_len), 0, padding_len);
> >> +
> >> + rt2800_write_tx_data(entry, txdesc);
> >
> > Could you please explain this change in more detail please?
> > Why is it needed? rt2800_write_tx_data only adds the TXWI to the front of the
> > buffer, doing that before or after the padding doesn't make a difference, no?
>
> Semantically it makes more sense this way, why modify a entry before
> writing it - but maybe the issue is that the name of the function
> makes it's function ambiguous, maybe renaming it to something along
> the lines of "rt2800_prepend_TXWI" would make it's purpose clearer.

Correct.

Helmut

2010-12-20 08:36:32

by Ismael Luceno

Subject: [PATCH 2/2] rt2x00: Fix panic on frame padding for rt2800 usb devices (due to not enough skb tailroom)

Signed-off-by: Ismael Luceno <[email protected]>
---
drivers/net/wireless/rt2x00/rt2800usb.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 85185de..d284e1e 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -369,7 +369,8 @@ static void rt2800usb_write_tx_desc(struct queue_entry *entry,
static void rt2800usb_write_tx_data(struct queue_entry *entry,
struct txentry_desc *txdesc)
{
- u8 padding_len;
+ unsigned int padding_len;
+ int err;

/*
* pad(1~3 bytes) is added after each 802.11 payload.
@@ -379,8 +380,14 @@ static void rt2800usb_write_tx_data(struct queue_entry *entry,
* |<------------- tx_pkt_len ------------->|
*/
padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
- memset(skb_put(entry->skb, padding_len), 0, padding_len);

+ err = skb_pad(entry->skb, padding_len);
+ if (unlikely(err)) {
+ WARNING(entry->queue->rt2x00dev, "TX SKB padding error, out of memory\n");
+ return;
+ }
+
+ entry->skb->len += padding_len;
rt2800_write_tx_data(entry, txdesc);
}

--
1.7.3.4

2010-12-20 09:31:10

by Julian Calaby

Subject: Re: [PATCH 1/2] rt2x00: Write the frame *after* padding (rt2800 usb devices)

On Mon, Dec 20, 2010 at 19:55, Helmut Schaa <[email protected]> wrote:
> Hi,
>
> Am Montag, 20. Dezember 2010 schrieb Ismael Luceno:
>> Signed-off-by: Ismael Luceno <[email protected]>
>> ---
>> ?drivers/net/wireless/rt2x00/rt2800usb.c | ? ?7 ++++---
>> ?1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
>> index 3e0205d..85185de 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
>> @@ -378,9 +378,10 @@ static void rt2800usb_write_tx_data(struct queue_entry *entry,
>> ? ? ? ?* | TXINFO | TXWI | 802.11 header | L2 pad | payload | pad | USB end pad |
>> ? ? ? ?* ? ? ? ? ? ? ? ? |<------------- tx_pkt_len ------------->|
>> ? ? ? ?*/
>> - ? ? ? ?rt2800_write_tx_data(entry, txdesc);
>> - ? ? ? ?padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
>> - ? ? ? ?memset(skb_put(entry->skb, padding_len), 0, padding_len);
>> + ? ? padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
>> + ? ? memset(skb_put(entry->skb, padding_len), 0, padding_len);
>> +
>> + ? ? rt2800_write_tx_data(entry, txdesc);
>
> Could you please explain this change in more detail please?
> Why is it needed? rt2800_write_tx_data only adds the TXWI to the front of the
> buffer, doing that before or after the padding doesn't make a difference, no?

Semantically it makes more sense this way, why modify a entry before
writing it - but maybe the issue is that the name of the function
makes it's function ambiguous, maybe renaming it to something along
the lines of "rt2800_prepend_TXWI" would make it's purpose clearer.

Thanks,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

2010-12-20 08:56:38

by Helmut Schaa

Subject: Re: [PATCH 1/2] rt2x00: Write the frame *after* padding (rt2800 usb devices)

Hi,

Am Montag, 20. Dezember 2010 schrieb Ismael Luceno:
> Signed-off-by: Ismael Luceno <[email protected]>
> ---
> drivers/net/wireless/rt2x00/rt2800usb.c | 7 ++++---
> 1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index 3e0205d..85185de 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -378,9 +378,10 @@ static void rt2800usb_write_tx_data(struct queue_entry *entry,
> * | TXINFO | TXWI | 802.11 header | L2 pad | payload | pad | USB end pad |
> * |<------------- tx_pkt_len ------------->|
> */
> - rt2800_write_tx_data(entry, txdesc);
> - padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
> - memset(skb_put(entry->skb, padding_len), 0, padding_len);
> + padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
> + memset(skb_put(entry->skb, padding_len), 0, padding_len);
> +
> + rt2800_write_tx_data(entry, txdesc);

Could you please explain this change in more detail please?
Why is it needed? rt2800_write_tx_data only adds the TXWI to the front of the
buffer, doing that before or after the padding doesn't make a difference, no?

Thanks,
Helmut