[PATCH v3 0/4] Further L2 padding fixes.

[PATCH v3 0/4] Further L2 padding fixes.

[Gertjan van Wingerde]

The L2 padding fixes patch has grown a bit and now consists of 4 separate
patches to clean the L2 padding code up and to fix a number of bugs at the
same time.

	1. rt2x00: Further L2 padding fixes.
	2. rt2x00: Remove SKBDESC_L2_PADDED flag.
	3. rt2x00: Reorganize L2 padding inserting function.
	4. rt2x00: Only remove L2 padding in received frames if there is payload.

---
Gertjan.

[PATCH v3 1/4] rt2x00: Further L2 padding fixes.

[Gertjan van Wingerde]

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.

Also introduce a central macro the compute the L2 padding size.

Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2x00.h      |    6 ++++++
 drivers/net/wireless/rt2x00/rt2x00queue.c |    8 +++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
index dcfc8c2..6594d2f 100644
--- a/drivers/net/wireless/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/rt2x00/rt2x00.h
@@ -113,6 +113,12 @@
 	(  ((unsigned long)((__skb)->data + (__header))) & 3 )
 
 /*
+ * Determine the number of L2 padding bytes required between the header and
+ * the payload.
+ */
+#define L2PAD_SIZE(__hdrlen)	(-(__hdrlen) & 3)
+
+/*
  * Constants for extra TX headroom for alignment purposes.
  */
 #define RT2X00_ALIGN_SIZE	4 /* Only whole frame needs alignment */
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index b8f0954..21d5876 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 = L2PAD_SIZE(header_length);
 
 	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;
 	}
 }
@@ -223,7 +224,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
 void rt2x00queue_remove_l2pad(struct sk_buff *skb, unsigned int header_length)
 {
 	struct skb_frame_desc *skbdesc = get_skb_frame_desc(skb);
-	unsigned int l2pad = 4 - (header_length & 3);
+	unsigned int l2pad = L2PAD_SIZE(header_length);
 
 	if (!l2pad || (skbdesc->flags & SKBDESC_L2_PADDED))
 		return;
@@ -346,7 +347,8 @@ 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);
+	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags))
+		txdesc->l2pad = L2PAD_SIZE(txdesc->header_length);
 
 	/*
 	 * Check whether this frame is to be acked.
-- 
1.6.5.3

[PATCH v3 2/4] rt2x00: Remove SKBDESC_L2_PADDED flag.

[Gertjan van Wingerde]

With the improved L2 padding code, this flag is no longer necessary, as the
rt2x00queue_remove_l2pad is capable of detecting by itself if L2 padding is
applied.
For received frames the RX descriptor flag is still being checked.

Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2800pci.c   |    5 +----
 drivers/net/wireless/rt2x00/rt2800usb.c   |    4 +---
 drivers/net/wireless/rt2x00/rt2x00queue.c |    6 +-----
 drivers/net/wireless/rt2x00/rt2x00queue.h |    5 +----
 4 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
index 6fcb2bb..458378c 100644
--- a/drivers/net/wireless/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/rt2x00/rt2800pci.c
@@ -835,7 +835,6 @@ static void rt2800pci_fill_rxdone(struct queue_entry *entry,
 				  struct rxdone_entry_desc *rxdesc)
 {
 	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
-	struct skb_frame_desc *skbdesc = get_skb_frame_desc(entry->skb);
 	struct queue_entry_priv_pci *entry_priv = entry->priv_data;
 	__le32 *rxd = entry_priv->desc;
 	__le32 *rxwi = (__le32 *)entry->skb->data;
@@ -883,10 +882,8 @@ static void rt2800pci_fill_rxdone(struct queue_entry *entry,
 	if (rt2x00_get_field32(rxd3, RXD_W3_MY_BSS))
 		rxdesc->dev_flags |= RXDONE_MY_BSS;
 
-	if (rt2x00_get_field32(rxd3, RXD_W3_L2PAD)) {
+	if (rt2x00_get_field32(rxd3, RXD_W3_L2PAD))
 		rxdesc->dev_flags |= RXDONE_L2PAD;
-		skbdesc->flags |= SKBDESC_L2_PADDED;
-	}
 
 	if (rt2x00_get_field32(rxwi1, RXWI_W1_SHORT_GI))
 		rxdesc->flags |= RX_FLAG_SHORT_GI;
diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 1db8667..13baec4 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -625,10 +625,8 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
 	if (rt2x00_get_field32(rxd0, RXINFO_W0_MY_BSS))
 		rxdesc->dev_flags |= RXDONE_MY_BSS;
 
-	if (rt2x00_get_field32(rxd0, RXINFO_W0_L2PAD)) {
+	if (rt2x00_get_field32(rxd0, RXINFO_W0_L2PAD))
 		rxdesc->dev_flags |= RXDONE_L2PAD;
-		skbdesc->flags |= SKBDESC_L2_PADDED;
-	}
 
 	if (rt2x00_get_field32(rxwi1, RXWI_W1_SHORT_GI))
 		rxdesc->flags |= RX_FLAG_SHORT_GI;
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index 21d5876..719f4ae 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -177,7 +177,6 @@ void rt2x00queue_align_payload(struct sk_buff *skb, unsigned int header_length)
 
 void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
 {
-	struct skb_frame_desc *skbdesc = get_skb_frame_desc(skb);
 	unsigned int frame_length = skb->len;
 	unsigned int header_align = ALIGN_SIZE(skb, 0);
 	unsigned int payload_align = ALIGN_SIZE(skb, header_length);
@@ -198,7 +197,6 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
 		 */
 		skb_push(skb, header_align);
 		memmove(skb->data, skb->data + header_align, header_length);
-		skbdesc->flags |= SKBDESC_L2_PADDED;
 	} else {
 		/*
 		 *
@@ -217,16 +215,14 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
 			skb->data + header_length + l2pad + payload_align,
 			frame_length - header_length);
 		skb_trim(skb, frame_length + l2pad);
-		skbdesc->flags |= SKBDESC_L2_PADDED;
 	}
 }
 
 void rt2x00queue_remove_l2pad(struct sk_buff *skb, unsigned int header_length)
 {
-	struct skb_frame_desc *skbdesc = get_skb_frame_desc(skb);
 	unsigned int l2pad = L2PAD_SIZE(header_length);
 
-	if (!l2pad || (skbdesc->flags & SKBDESC_L2_PADDED))
+	if (!l2pad)
 		return;
 
 	memmove(skb->data + l2pad, skb->data, header_length);
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h
index 70775e5..c1e482b 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.h
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.h
@@ -92,8 +92,6 @@ enum data_queue_qid {
  * @SKBDESC_DMA_MAPPED_TX: &skb_dma field has been mapped for TX
  * @SKBDESC_IV_STRIPPED: Frame contained a IV/EIV provided by
  *	mac80211 but was stripped for processing by the driver.
- * @SKBDESC_L2_PADDED: Payload has been padded for 4-byte alignment,
- *	the padded bytes are located between header and payload.
  * @SKBDESC_NOT_MAC80211: Frame didn't originate from mac80211,
  *	don't try to pass it back.
  */
@@ -101,8 +99,7 @@ enum skb_frame_desc_flags {
 	SKBDESC_DMA_MAPPED_RX = 1 << 0,
 	SKBDESC_DMA_MAPPED_TX = 1 << 1,
 	SKBDESC_IV_STRIPPED = 1 << 2,
-	SKBDESC_L2_PADDED = 1 << 3,
-	SKBDESC_NOT_MAC80211 = 1 << 4,
+	SKBDESC_NOT_MAC80211 = 1 << 3,
 };
 
 /**
-- 
1.6.5.3

[PATCH v3 3/4] rt2x00: Reorganize L2 padding inserting function.

[Gertjan van Wingerde]

Simplify the rt2x00queue_insert_l2pad function by handling the alignment
operations one by one. Do not special case special circumstances.
Basically first perform header alignment, and then perform payload alignment
(if any payload does exist). This results in a properly aligned skb.

The end result is better readable code, with better results, as now L2 padding
is inserted only when a payload is actually present in the frame.

Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2x00queue.c |   60 +++++++++++++----------------
 1 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index 719f4ae..7452fa8 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -177,45 +177,38 @@ void rt2x00queue_align_payload(struct sk_buff *skb, unsigned int header_length)
 
 void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
 {
-	unsigned int frame_length = skb->len;
+	unsigned int payload_length = skb->len - header_length;
 	unsigned int header_align = ALIGN_SIZE(skb, 0);
 	unsigned int payload_align = ALIGN_SIZE(skb, header_length);
 	unsigned int l2pad = L2PAD_SIZE(header_length);
 
-	if (header_align == payload_align) {
-		/*
-		 * Both header and payload must be moved the same
-		 * amount of bytes to align them properly. This means
-		 * we don't use the L2 padding but just move the entire
-		 * frame.
-		 */
-		rt2x00queue_align_frame(skb);
-	} else if (!payload_align) {
-		/*
-		 * Simple L2 padding, only the header needs to be moved,
-		 * the payload is already properly aligned.
-		 */
-		skb_push(skb, header_align);
-		memmove(skb->data, skb->data + header_align, header_length);
-	} else {
-		/*
-		 *
-		 * Complicated L2 padding, both header and payload need
-		 * to be moved. By default we only move to the start
-		 * of the buffer, so our header alignment needs to be
-		 * increased if there is not enough room for the header
-		 * to be moved.
-		 */
-		if (payload_align > header_align)
-			header_align += 4;
+	/*
+	 * Adjust the header alignment if the payload needs to be moved more
+	 * than the header.
+	 */
+	if (payload_align > header_align)
+		header_align += 4;
+
+	/* There is nothing to do if no alignment is needed */
+	if (!header_align)
+		return;
 
-		skb_push(skb, header_align);
-		memmove(skb->data, skb->data + header_align, header_length);
+	/* Reserve the amount of space needed in front of the frame */
+	skb_push(skb, header_align);
+
+	/*
+	 * Move the header.
+	 */
+	memmove(skb->data, skb->data + header_align, header_length);
+
+	/* Move the payload, if present and if required */
+	if (payload_length && payload_align)
 		memmove(skb->data + header_length + l2pad,
 			skb->data + header_length + l2pad + payload_align,
-			frame_length - header_length);
-		skb_trim(skb, frame_length + l2pad);
-	}
+			payload_length);
+
+	/* Trim the skb to the correct size */
+	skb_trim(skb, header_length + l2pad + payload_length);
 }
 
 void rt2x00queue_remove_l2pad(struct sk_buff *skb, unsigned int header_length)
@@ -343,7 +336,8 @@ static void rt2x00queue_create_tx_descriptor(struct queue_entry *entry,
 	 * Header and alignment information.
 	 */
 	txdesc->header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
-	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags))
+	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags) &&
+	    (entry->skb->len > txdesc->header_length))
 		txdesc->l2pad = L2PAD_SIZE(txdesc->header_length);
 
 	/*
-- 
1.6.5.3

[PATCH v3 4/4] rt2x00: Only remove L2 padding in received frames if there is payload.

[Gertjan van Wingerde]

L2 padding will only be present when there is actual payload present.

Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
---
 drivers/net/wireless/rt2x00/rt2x00dev.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index ca2bcc0..b93731b 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -401,7 +401,9 @@ void rt2x00lib_rxdone(struct rt2x00_dev *rt2x00dev,
 	    (rxdesc.flags & RX_FLAG_IV_STRIPPED))
 		rt2x00crypto_rx_insert_iv(entry->skb, header_length,
 					  &rxdesc);
-	else if (rxdesc.dev_flags & RXDONE_L2PAD)
+	else if (header_length &&
+		 (rxdesc.size > header_length) &&
+		 (rxdesc.dev_flags & RXDONE_L2PAD))
 		rt2x00queue_remove_l2pad(entry->skb, header_length);
 	else
 		rt2x00queue_align_payload(entry->skb, header_length);
-- 
1.6.5.3

Re: [PATCH v3 4/4] rt2x00: Only remove L2 padding in received frames if there is payload.

[Ivo van Doorn]

On Monday 30 November 2009, Gertjan van Wingerde wrote:
> L2 padding will only be present when there is actual payload present.
> 
> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
> ---
>  drivers/net/wireless/rt2x00/rt2x00dev.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> index ca2bcc0..b93731b 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> @@ -401,7 +401,9 @@ void rt2x00lib_rxdone(struct rt2x00_dev *rt2x00dev,
>  	    (rxdesc.flags & RX_FLAG_IV_STRIPPED))
>  		rt2x00crypto_rx_insert_iv(entry->skb, header_length,
>  					  &rxdesc);
> -	else if (rxdesc.dev_flags & RXDONE_L2PAD)
> +	else if (header_length &&
> +		 (rxdesc.size > header_length) &&
> +		 (rxdesc.dev_flags & RXDONE_L2PAD))
>  		rt2x00queue_remove_l2pad(entry->skb, header_length);
>  	else
>  		rt2x00queue_align_payload(entry->skb, header_length);

Is there a scenario where header_lenght can be 0?
And if there is, shouldn't we have bailed-out completely already?

Ivo

Re: [PATCH v3 4/4] rt2x00: Only remove L2 padding in received frames if there is payload.

[Gertjan van Wingerde]

On 12/01/09 20:16, Ivo van Doorn wrote:
> On Monday 30 November 2009, Gertjan van Wingerde wrote:
>> L2 padding will only be present when there is actual payload present.
>>
>> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
>> ---
>>  drivers/net/wireless/rt2x00/rt2x00dev.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
>> index ca2bcc0..b93731b 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
>> @@ -401,7 +401,9 @@ void rt2x00lib_rxdone(struct rt2x00_dev *rt2x00dev,
>>  	    (rxdesc.flags & RX_FLAG_IV_STRIPPED))
>>  		rt2x00crypto_rx_insert_iv(entry->skb, header_length,
>>  					  &rxdesc);
>> -	else if (rxdesc.dev_flags & RXDONE_L2PAD)
>> +	else if (header_length &&
>> +		 (rxdesc.size > header_length) &&
>> +		 (rxdesc.dev_flags & RXDONE_L2PAD))
>>  		rt2x00queue_remove_l2pad(entry->skb, header_length);
>>  	else
>>  		rt2x00queue_align_payload(entry->skb, header_length);
> 
> Is there a scenario where header_lenght can be 0?
> And if there is, shouldn't we have bailed-out completely already?
> 

Actually, there is. ieee80211_get_hdrlen_from_skb returns 0 when it detects that
the skb is actually shorter than it should be (according to the frame type indicated
by the frame_control field).
I'm using that here to check whether actually any payload is present, as a value of 0
means that there isn't even a full header.
We should still pass on the frame in that case, for the monitor interfaces.

I agree it is a bit of a trick, but it works.

---
Gertjan.

Re: [PATCH v3 4/4] rt2x00: Only remove L2 padding in received frames if there is payload.

[Ivo van Doorn]

On Tuesday 01 December 2009, Gertjan van Wingerde wrote:
> On 12/01/09 20:16, Ivo van Doorn wrote:
> > On Monday 30 November 2009, Gertjan van Wingerde wrote:
> >> L2 padding will only be present when there is actual payload present.
> >>
> >> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
> >> ---
> >>  drivers/net/wireless/rt2x00/rt2x00dev.c |    4 +++-
> >>  1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> >> index ca2bcc0..b93731b 100644
> >> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> >> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> >> @@ -401,7 +401,9 @@ void rt2x00lib_rxdone(struct rt2x00_dev *rt2x00dev,
> >>  	    (rxdesc.flags & RX_FLAG_IV_STRIPPED))
> >>  		rt2x00crypto_rx_insert_iv(entry->skb, header_length,
> >>  					  &rxdesc);
> >> -	else if (rxdesc.dev_flags & RXDONE_L2PAD)
> >> +	else if (header_length &&
> >> +		 (rxdesc.size > header_length) &&
> >> +		 (rxdesc.dev_flags & RXDONE_L2PAD))
> >>  		rt2x00queue_remove_l2pad(entry->skb, header_length);
> >>  	else
> >>  		rt2x00queue_align_payload(entry->skb, header_length);
> > 
> > Is there a scenario where header_lenght can be 0?
> > And if there is, shouldn't we have bailed-out completely already?
> > 
> 
> Actually, there is. ieee80211_get_hdrlen_from_skb returns 0 when it detects that
> the skb is actually shorter than it should be (according to the frame type indicated
> by the frame_control field).
> I'm using that here to check whether actually any payload is present, as a value of 0
> means that there isn't even a full header.
> We should still pass on the frame in that case, for the monitor interfaces.

Ok, then we should pass it to mac80211, but we better use a goto or something to
skip all frame manipulation (like the rt2x00crypto_rx_insert_iv and rt2x00queue_align_payload)
as well.

Ivo

Re: [PATCH v3 4/4] rt2x00: Only remove L2 padding in received frames if there is payload.

[Gertjan van Wingerde]

On 12/01/09 22:57, Ivo van Doorn wrote:
> On Tuesday 01 December 2009, Gertjan van Wingerde wrote:
>> On 12/01/09 20:16, Ivo van Doorn wrote:
>>> On Monday 30 November 2009, Gertjan van Wingerde wrote:
>>>> L2 padding will only be present when there is actual payload present.
>>>>
>>>> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
>>>> ---
>>>>  drivers/net/wireless/rt2x00/rt2x00dev.c |    4 +++-
>>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
>>>> index ca2bcc0..b93731b 100644
>>>> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
>>>> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
>>>> @@ -401,7 +401,9 @@ void rt2x00lib_rxdone(struct rt2x00_dev *rt2x00dev,
>>>>  	    (rxdesc.flags & RX_FLAG_IV_STRIPPED))
>>>>  		rt2x00crypto_rx_insert_iv(entry->skb, header_length,
>>>>  					  &rxdesc);
>>>> -	else if (rxdesc.dev_flags & RXDONE_L2PAD)
>>>> +	else if (header_length &&
>>>> +		 (rxdesc.size > header_length) &&
>>>> +		 (rxdesc.dev_flags & RXDONE_L2PAD))
>>>>  		rt2x00queue_remove_l2pad(entry->skb, header_length);
>>>>  	else
>>>>  		rt2x00queue_align_payload(entry->skb, header_length);
>>>
>>> Is there a scenario where header_lenght can be 0?
>>> And if there is, shouldn't we have bailed-out completely already?
>>>
>>
>> Actually, there is. ieee80211_get_hdrlen_from_skb returns 0 when it detects that
>> the skb is actually shorter than it should be (according to the frame type indicated
>> by the frame_control field).
>> I'm using that here to check whether actually any payload is present, as a value of 0
>> means that there isn't even a full header.
>> We should still pass on the frame in that case, for the monitor interfaces.
> 
> Ok, then we should pass it to mac80211, but we better use a goto or something to
> skip all frame manipulation (like the rt2x00crypto_rx_insert_iv and rt2x00queue_align_payload)
> as well.
> 

Good point. Can we do that as a follow-up patch?

---
Gertjan.

Re: [PATCH v3 4/4] rt2x00: Only remove L2 padding in received frames if there is payload.

[Ivo Van Doorn]

On Tue, Dec 1, 2009 at 11:09 PM, Gertjan van Wingerde
<gwingerde@gmail.com> wrote:
> On 12/01/09 22:57, Ivo van Doorn wrote:
>> On Tuesday 01 December 2009, Gertjan van Wingerde wrote:
>>> On 12/01/09 20:16, Ivo van Doorn wrote:
>>>> On Monday 30 November 2009, Gertjan van Wingerde wrote:
>>>>> L2 padding will only be present when there is actual payload present.
>>>>>
>>>>> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
>>>>> ---
>>>>>  drivers/net/wireless/rt2x00/rt2x00dev.c |    4 +++-
>>>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
>>>>> index ca2bcc0..b93731b 100644
>>>>> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
>>>>> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
>>>>> @@ -401,7 +401,9 @@ void rt2x00lib_rxdone(struct rt2x00_dev *rt2x00dev,
>>>>>        (rxdesc.flags & RX_FLAG_IV_STRIPPED))
>>>>>            rt2x00crypto_rx_insert_iv(entry->skb, header_length,
>>>>>                                      &rxdesc);
>>>>> -  else if (rxdesc.dev_flags & RXDONE_L2PAD)
>>>>> +  else if (header_length &&
>>>>> +           (rxdesc.size > header_length) &&
>>>>> +           (rxdesc.dev_flags & RXDONE_L2PAD))
>>>>>            rt2x00queue_remove_l2pad(entry->skb, header_length);
>>>>>    else
>>>>>            rt2x00queue_align_payload(entry->skb, header_length);
>>>>
>>>> Is there a scenario where header_lenght can be 0?
>>>> And if there is, shouldn't we have bailed-out completely already?
>>>>
>>>
>>> Actually, there is. ieee80211_get_hdrlen_from_skb returns 0 when it detects that
>>> the skb is actually shorter than it should be (according to the frame type indicated
>>> by the frame_control field).
>>> I'm using that here to check whether actually any payload is present, as a value of 0
>>> means that there isn't even a full header.
>>> We should still pass on the frame in that case, for the monitor interfaces.
>>
>> Ok, then we should pass it to mac80211, but we better use a goto or something to
>> skip all frame manipulation (like the rt2x00crypto_rx_insert_iv and rt2x00queue_align_payload)
>> as well.
>>
>
> Good point. Can we do that as a follow-up patch?

Sure. :)

Ivo

Re: [PATCH v3 4/4] rt2x00: Only remove L2 padding in received frames if there is payload.

[Ivo van Doorn]

On Monday 30 November 2009, Gertjan van Wingerde wrote:
> L2 padding will only be present when there is actual payload present.
> 
> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

Re: [PATCH v3 3/4] rt2x00: Reorganize L2 padding inserting function.

[Ivo van Doorn]

On Monday 30 November 2009, Gertjan van Wingerde wrote:
> Simplify the rt2x00queue_insert_l2pad function by handling the alignment
> operations one by one. Do not special case special circumstances.
> Basically first perform header alignment, and then perform payload alignment
> (if any payload does exist). This results in a properly aligned skb.
> 
> The end result is better readable code, with better results, as now L2 padding
> is inserted only when a payload is actually present in the frame.
> 
> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
> ---
>  drivers/net/wireless/rt2x00/rt2x00queue.c |   60 +++++++++++++----------------
>  1 files changed, 27 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index 719f4ae..7452fa8 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
> @@ -177,45 +177,38 @@ void rt2x00queue_align_payload(struct sk_buff *skb, unsigned int header_length)
>  
>  void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
>  {
> -	unsigned int frame_length = skb->len;
> +	unsigned int payload_length = skb->len - header_length;
>  	unsigned int header_align = ALIGN_SIZE(skb, 0);
>  	unsigned int payload_align = ALIGN_SIZE(skb, header_length);
>  	unsigned int l2pad = L2PAD_SIZE(header_length);
>  
> -	if (header_align == payload_align) {
> -		/*
> -		 * Both header and payload must be moved the same
> -		 * amount of bytes to align them properly. This means
> -		 * we don't use the L2 padding but just move the entire
> -		 * frame.
> -		 */
> -		rt2x00queue_align_frame(skb);
> -	} else if (!payload_align) {
> -		/*
> -		 * Simple L2 padding, only the header needs to be moved,
> -		 * the payload is already properly aligned.
> -		 */
> -		skb_push(skb, header_align);
> -		memmove(skb->data, skb->data + header_align, header_length);
> -	} else {
> -		/*
> -		 *
> -		 * Complicated L2 padding, both header and payload need
> -		 * to be moved. By default we only move to the start
> -		 * of the buffer, so our header alignment needs to be
> -		 * increased if there is not enough room for the header
> -		 * to be moved.
> -		 */
> -		if (payload_align > header_align)
> -			header_align += 4;
> +	/*
> +	 * Adjust the header alignment if the payload needs to be moved more
> +	 * than the header.
> +	 */
> +	if (payload_align > header_align)
> +		header_align += 4;
> +
> +	/* There is nothing to do if no alignment is needed */
> +	if (!header_align)
> +		return;
>  
> -		skb_push(skb, header_align);
> -		memmove(skb->data, skb->data + header_align, header_length);
> +	/* Reserve the amount of space needed in front of the frame */
> +	skb_push(skb, header_align);
> +
> +	/*
> +	 * Move the header.
> +	 */
> +	memmove(skb->data, skb->data + header_align, header_length);
> +
> +	/* Move the payload, if present and if required */
> +	if (payload_length && payload_align)
>  		memmove(skb->data + header_length + l2pad,
>  			skb->data + header_length + l2pad + payload_align,
> -			frame_length - header_length);
> -		skb_trim(skb, frame_length + l2pad);
> -	}
> +			payload_length);
> +
> +	/* Trim the skb to the correct size */
> +	skb_trim(skb, header_length + l2pad + payload_length);
>  }
>  
>  void rt2x00queue_remove_l2pad(struct sk_buff *skb, unsigned int header_length)
> @@ -343,7 +336,8 @@ static void rt2x00queue_create_tx_descriptor(struct queue_entry *entry,
>  	 * Header and alignment information.
>  	 */
>  	txdesc->header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
> -	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags))
> +	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags) &&
> +	    (entry->skb->len > txdesc->header_length))
>  		txdesc->l2pad = L2PAD_SIZE(txdesc->header_length);

This check is a bit unclear, are you not simply checking for ieee80211_is_data() or
ieee80211_is_data_present()?

Ivo

Re: [PATCH v3 3/4] rt2x00: Reorganize L2 padding inserting function.

[Gertjan van Wingerde]

On 12/01/09 20:13, Ivo van Doorn wrote:
> On Monday 30 November 2009, Gertjan van Wingerde wrote:
>> Simplify the rt2x00queue_insert_l2pad function by handling the alignment
>> operations one by one. Do not special case special circumstances.
>> Basically first perform header alignment, and then perform payload alignment
>> (if any payload does exist). This results in a properly aligned skb.
>>
>> The end result is better readable code, with better results, as now L2 padding
>> is inserted only when a payload is actually present in the frame.
>>
>> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>
>> ---
>>  drivers/net/wireless/rt2x00/rt2x00queue.c |   60 +++++++++++++----------------
>>  1 files changed, 27 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> index 719f4ae..7452fa8 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
>> @@ -177,45 +177,38 @@ void rt2x00queue_align_payload(struct sk_buff *skb, unsigned int header_length)
>>  
>>  void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
>>  {
>> -	unsigned int frame_length = skb->len;
>> +	unsigned int payload_length = skb->len - header_length;
>>  	unsigned int header_align = ALIGN_SIZE(skb, 0);
>>  	unsigned int payload_align = ALIGN_SIZE(skb, header_length);
>>  	unsigned int l2pad = L2PAD_SIZE(header_length);
>>  
>> -	if (header_align == payload_align) {
>> -		/*
>> -		 * Both header and payload must be moved the same
>> -		 * amount of bytes to align them properly. This means
>> -		 * we don't use the L2 padding but just move the entire
>> -		 * frame.
>> -		 */
>> -		rt2x00queue_align_frame(skb);
>> -	} else if (!payload_align) {
>> -		/*
>> -		 * Simple L2 padding, only the header needs to be moved,
>> -		 * the payload is already properly aligned.
>> -		 */
>> -		skb_push(skb, header_align);
>> -		memmove(skb->data, skb->data + header_align, header_length);
>> -	} else {
>> -		/*
>> -		 *
>> -		 * Complicated L2 padding, both header and payload need
>> -		 * to be moved. By default we only move to the start
>> -		 * of the buffer, so our header alignment needs to be
>> -		 * increased if there is not enough room for the header
>> -		 * to be moved.
>> -		 */
>> -		if (payload_align > header_align)
>> -			header_align += 4;
>> +	/*
>> +	 * Adjust the header alignment if the payload needs to be moved more
>> +	 * than the header.
>> +	 */
>> +	if (payload_align > header_align)
>> +		header_align += 4;
>> +
>> +	/* There is nothing to do if no alignment is needed */
>> +	if (!header_align)
>> +		return;
>>  
>> -		skb_push(skb, header_align);
>> -		memmove(skb->data, skb->data + header_align, header_length);
>> +	/* Reserve the amount of space needed in front of the frame */
>> +	skb_push(skb, header_align);
>> +
>> +	/*
>> +	 * Move the header.
>> +	 */
>> +	memmove(skb->data, skb->data + header_align, header_length);
>> +
>> +	/* Move the payload, if present and if required */
>> +	if (payload_length && payload_align)
>>  		memmove(skb->data + header_length + l2pad,
>>  			skb->data + header_length + l2pad + payload_align,
>> -			frame_length - header_length);
>> -		skb_trim(skb, frame_length + l2pad);
>> -	}
>> +			payload_length);
>> +
>> +	/* Trim the skb to the correct size */
>> +	skb_trim(skb, header_length + l2pad + payload_length);
>>  }
>>  
>>  void rt2x00queue_remove_l2pad(struct sk_buff *skb, unsigned int header_length)
>> @@ -343,7 +336,8 @@ static void rt2x00queue_create_tx_descriptor(struct queue_entry *entry,
>>  	 * Header and alignment information.
>>  	 */
>>  	txdesc->header_length = ieee80211_get_hdrlen_from_skb(entry->skb);
>> -	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags))
>> +	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags) &&
>> +	    (entry->skb->len > txdesc->header_length))
>>  		txdesc->l2pad = L2PAD_SIZE(txdesc->header_length);
> 
> This check is a bit unclear, are you not simply checking for ieee80211_is_data() or
> ieee80211_is_data_present()?
> 

Well, this check is more based on the actual contents of the skb, where ieee802111_is_data
and ieee80211_is_data_present only indicate, based on the frame_control field, whether any
data should be present.

Personally, I think this is safer, as it matches the actual data at hand. But I guess if
absolutely necessary ieee80211_is_data_present can be used as well.

---
Gertjan.

Re: [PATCH v3 3/4] rt2x00: Reorganize L2 padding inserting function.

[Ivo van Doorn]

On Monday 30 November 2009, Gertjan van Wingerde wrote:
> Simplify the rt2x00queue_insert_l2pad function by handling the alignment
> operations one by one. Do not special case special circumstances.
> Basically first perform header alignment, and then perform payload alignment
> (if any payload does exist). This results in a properly aligned skb.
> 
> The end result is better readable code, with better results, as now L2 padding
> is inserted only when a payload is actually present in the frame.
> 
> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

Re: [PATCH v3 2/4] rt2x00: Remove SKBDESC_L2_PADDED flag.

[Ivo van Doorn]

On Monday 30 November 2009, Gertjan van Wingerde wrote:
> With the improved L2 padding code, this flag is no longer necessary, as the
> rt2x00queue_remove_l2pad is capable of detecting by itself if L2 padding is
> applied.
> For received frames the RX descriptor flag is still being checked.
> 
> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  drivers/net/wireless/rt2x00/rt2800pci.c   |    5 +----
>  drivers/net/wireless/rt2x00/rt2800usb.c   |    4 +---
>  drivers/net/wireless/rt2x00/rt2x00queue.c |    6 +-----
>  drivers/net/wireless/rt2x00/rt2x00queue.h |    5 +----
>  4 files changed, 4 insertions(+), 16 deletions(-)
> 

Re: [PATCH v3 1/4] rt2x00: Further L2 padding fixes.

[Ivo van Doorn]

On Monday 30 November 2009, Gertjan van Wingerde 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.
> 
> Also introduce a central macro the compute the L2 padding size.
> 
> Signed-off-by: Gertjan van Wingerde <gwingerde@gmail.com>

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  drivers/net/wireless/rt2x00/rt2x00.h      |    6 ++++++
>  drivers/net/wireless/rt2x00/rt2x00queue.c |    8 +++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
> index dcfc8c2..6594d2f 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00.h
> @@ -113,6 +113,12 @@
>  	(  ((unsigned long)((__skb)->data + (__header))) & 3 )
>  
>  /*
> + * Determine the number of L2 padding bytes required between the header and
> + * the payload.
> + */
> +#define L2PAD_SIZE(__hdrlen)	(-(__hdrlen) & 3)
> +
> +/*
>   * Constants for extra TX headroom for alignment purposes.
>   */
>  #define RT2X00_ALIGN_SIZE	4 /* Only whole frame needs alignment */
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index b8f0954..21d5876 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 = L2PAD_SIZE(header_length);
>  
>  	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;
>  	}
>  }
> @@ -223,7 +224,7 @@ void rt2x00queue_insert_l2pad(struct sk_buff *skb, unsigned int header_length)
>  void rt2x00queue_remove_l2pad(struct sk_buff *skb, unsigned int header_length)
>  {
>  	struct skb_frame_desc *skbdesc = get_skb_frame_desc(skb);
> -	unsigned int l2pad = 4 - (header_length & 3);
> +	unsigned int l2pad = L2PAD_SIZE(header_length);
>  
>  	if (!l2pad || (skbdesc->flags & SKBDESC_L2_PADDED))
>  		return;
> @@ -346,7 +347,8 @@ 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);
> +	if (test_bit(DRIVER_REQUIRE_L2PAD, &rt2x00dev->flags))
> +		txdesc->l2pad = L2PAD_SIZE(txdesc->header_length);
>  
>  	/*
>  	 * Check whether this frame is to be acked.

Re: [rt2x00-users] [PATCH v3 0/4] Further L2 padding fixes.

[Benoit PAPILLAULT]

Gertjan van Wingerde a écrit :
> The L2 padding fixes patch has grown a bit and now consists of 4 separate
> patches to clean the L2 padding code up and to fix a number of bugs at the
> same time.
> 
> 	1. rt2x00: Further L2 padding fixes.
> 	2. rt2x00: Remove SKBDESC_L2_PADDED flag.
> 	3. rt2x00: Reorganize L2 padding inserting function.
> 	4. rt2x00: Only remove L2 padding in received frames if there is payload.
> 
> ---
> Gertjan.

Thanks Gertjan for the patches, I know that padding is a bit of
nightware. I've testing your tree and here are the results:

1. On TX : It fails for control frames with hdrlen=10 since it will
produce l2pad = 2 in your case, where it should be 0. In all other
cases, it works!

Solution : padding is only needed for data frames for rt28x devices, so
I think it's better to something like rt2xqueue_padpos
[http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=95ddf076c13062d1026025d97ba511f880a1792d]

2. On RX : It fails for data frames with hdrlen=26 or 30. So it looks
like it's not unpadding at all. I think your code is correct however,
but it still depends on RXDONE_L2PAD which is not properly set

Solution : applies the 2 patches I just posted
[http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=8fee77832720708a09c50f9002edcc68dc5bd0a7]
and
[http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=b8fce373e9de892113b32322a95ca5bc7da56389]

or get rid of RXDONE_L2PAD all together + padpos function :-)

Regards,
Benoit

Re: [rt2x00-users] [PATCH v3 0/4] Further L2 padding fixes.

[Gertjan van Wingerde]

On 12/01/09 00:46, Benoit PAPILLAULT wrote:
> Gertjan van Wingerde a écrit :
>> The L2 padding fixes patch has grown a bit and now consists of 4 separate
>> patches to clean the L2 padding code up and to fix a number of bugs at the
>> same time.
>>
>> 	1. rt2x00: Further L2 padding fixes.
>> 	2. rt2x00: Remove SKBDESC_L2_PADDED flag.
>> 	3. rt2x00: Reorganize L2 padding inserting function.
>> 	4. rt2x00: Only remove L2 padding in received frames if there is payload.
>>
>> ---
>> Gertjan.
> 
> Thanks Gertjan for the patches, I know that padding is a bit of
> nightware. I've testing your tree and here are the results:
> 
> 1. On TX : It fails for control frames with hdrlen=10 since it will
> produce l2pad = 2 in your case, where it should be 0. In all other
> cases, it works!
> 

Hmm. That's strange, as patch 3 of the series should have handled this. Basically this patch
checks if a frame actually has payload, and refrains from inserting l2pad and payload alignment
in that case.

How did you detect this failure?
What did you observe on the "wire", i.e. how was the frame malformed because of this?

Also, would it be possible to get some of the skb details of a frame that fails?


> Solution : padding is only needed for data frames for rt28x devices, so
> I think it's better to something like rt2xqueue_padpos
> [http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=95ddf076c13062d1026025d97ba511f880a1792d]
> 

I'm still not fond of using detailed ieee 802.11 frame format knowledge to do this. I prefer
to use standard mac80211 functions with a bit of detection on the actual skb.

---
Gertjan.

Re: [rt2x00-users] [PATCH v3 0/4] Further L2 padding fixes.

[Benoit PAPILLAULT]

Gertjan van Wingerde a écrit :
> On 12/01/09 00:46, Benoit PAPILLAULT wrote:
>   
>> Gertjan van Wingerde a écrit :
>>     
>>> The L2 padding fixes patch has grown a bit and now consists of 4 separate
>>> patches to clean the L2 padding code up and to fix a number of bugs at the
>>> same time.
>>>
>>> 	1. rt2x00: Further L2 padding fixes.
>>> 	2. rt2x00: Remove SKBDESC_L2_PADDED flag.
>>> 	3. rt2x00: Reorganize L2 padding inserting function.
>>> 	4. rt2x00: Only remove L2 padding in received frames if there is payload.
>>>
>>> ---
>>> Gertjan.
>>>       
>> Thanks Gertjan for the patches, I know that padding is a bit of
>> nightware. I've testing your tree and here are the results:
>>
>> 1. On TX : It fails for control frames with hdrlen=10 since it will
>> produce l2pad = 2 in your case, where it should be 0. In all other
>> cases, it works!
>>
>>     
>
> Hmm. That's strange, as patch 3 of the series should have handled this. Basically this patch
> checks if a frame actually has payload, and refrains from inserting l2pad and payload alignment
> in that case.
>
> How did you detect this failure?
>   
I sent every possible frame type in monitor and compared with what ath9k 
in monitor mode received.
> What did you observe on the "wire", i.e. how was the frame malformed because of this?
>   
For instance, i sent :
c400c400c40000000000 (10 bytes)

and I received :
c400ca00c400000000000000 (12 bytes)

You can see the 2 bytes added at the end (bytes 2-3 are modified by the 
HW since it contains the duration_id)
> Also, would it be possible to get some of the skb details of a frame that fails?
>   
I'll redo the test. What details do you want?
>
>   
>> Solution : padding is only needed for data frames for rt28x devices, so
>> I think it's better to something like rt2xqueue_padpos
>> [http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=95ddf076c13062d1026025d97ba511f880a1792d]
>>
>>     
>
> I'm still not fond of using detailed ieee 802.11 frame format knowledge to do this. I prefer
> to use standard mac80211 functions with a bit of detection on the actual skb.
>   

If you want to use mac80211 functions, we can do something like that : 
/*
 * Determine the L2 padding size based on the 802.11 header length
 *
 * Exact formula is (4 - (header_length%4))%4 but since header_length is
 * always a multiple of 2, we could simplify to (header_length&3).
 *
 * L2PAD is only present for data frame and an easy way to check for that is
 * to compare header_length with 24 bytes.
 */
#define L2PAD_SIZE(__header) \
       ((__header)<24 ? 0 : ((4 - ((__header)%4))%4))

Regards,
Benoit

Re: [rt2x00-users] [PATCH v3 0/4] Further L2 padding fixes.

[Gertjan van Wingerde]

On Wed, Dec 2, 2009 at 10:54 AM, Benoit PAPILLAULT
<benoit.papillault@free.fr> wrote:
> Gertjan van Wingerde a écrit :
>>
>> On 12/01/09 00:46, Benoit PAPILLAULT wrote:
>>
>>>
>>> Gertjan van Wingerde a écrit :
>>>
>>>>
>>>> The L2 padding fixes patch has grown a bit and now consists of 4
>>>> separate
>>>> patches to clean the L2 padding code up and to fix a number of bugs at
>>>> the
>>>> same time.
>>>>
>>>>        1. rt2x00: Further L2 padding fixes.
>>>>        2. rt2x00: Remove SKBDESC_L2_PADDED flag.
>>>>        3. rt2x00: Reorganize L2 padding inserting function.
>>>>        4. rt2x00: Only remove L2 padding in received frames if there is
>>>> payload.
>>>>
>>>> ---
>>>> Gertjan.
>>>>
>>>
>>> Thanks Gertjan for the patches, I know that padding is a bit of
>>> nightware. I've testing your tree and here are the results:
>>>
>>> 1. On TX : It fails for control frames with hdrlen=10 since it will
>>> produce l2pad = 2 in your case, where it should be 0. In all other
>>> cases, it works!
>>>
>>>
>>
>> Hmm. That's strange, as patch 3 of the series should have handled this.
>> Basically this patch
>> checks if a frame actually has payload, and refrains from inserting l2pad
>> and payload alignment
>> in that case.
>>
>> How did you detect this failure?
>>
>
> I sent every possible frame type in monitor and compared with what ath9k in
> monitor mode received.
>>
>> What did you observe on the "wire", i.e. how was the frame malformed
>> because of this?
>>
>
> For instance, i sent :
> c400c400c40000000000 (10 bytes)
>
> and I received :
> c400ca00c400000000000000 (12 bytes)
>
> You can see the 2 bytes added at the end (bytes 2-3 are modified by the HW
> since it contains the duration_id)

OK. Thanks for the example. I think I already know where the problem is. The
rt2x00queue_insert_l2pad function doesn't trim down the skb properly in case the
frame has no payload. I'll provide a patch, but that may take until
Friday (when I get back
home).

>>
>> Also, would it be possible to get some of the skb details of a frame that
>> fails?
>>
>
> I'll redo the test. What details do you want?

Never mind. I think I already know where the problem is.

>>
>>
>>>
>>> Solution : padding is only needed for data frames for rt28x devices, so
>>> I think it's better to something like rt2xqueue_padpos
>>>
>>> [http://git.popipo.fr/?p=rt2x00.git;a=commitdiff;h=95ddf076c13062d1026025d97ba511f880a1792d]
>>>
>>>
>>
>> I'm still not fond of using detailed ieee 802.11 frame format knowledge to
>> do this. I prefer
>> to use standard mac80211 functions with a bit of detection on the actual
>> skb.
>>
>
> If you want to use mac80211 functions, we can do something like that : /*
> * Determine the L2 padding size based on the 802.11 header length
> *
> * Exact formula is (4 - (header_length%4))%4 but since header_length is
> * always a multiple of 2, we could simplify to (header_length&3).

Current formula is (-header_length) & 3, which works even for
header_lengths that are not
a multiple of 2.

> *
> * L2PAD is only present for data frame and an easy way to check for that is
> * to compare header_length with 24 bytes.
> */
> #define L2PAD_SIZE(__header) \
>      ((__header)<24 ? 0 : ((4 - ((__header)%4))%4))
>

That depends on what the purpose of the L2PAD_SIZE macro is going to
be. At the moment
the intention is to have L2PAD_SIZE compute the number of l2pad bytes
necessary, if a
payload is present. Detection on whether actually a payload is present
and whether the
outcome of this macro should be used should be at the call-sites of this macro.

---
Gertjan.

Re: [rt2x00-users] [PATCH v3 0/4] Further L2 padding fixes.

[Ivo van Doorn]

> > * L2PAD is only present for data frame and an easy way to check for that is
> > * to compare header_length with 24 bytes.
> > */
> > #define L2PAD_SIZE(__header) \
> >      ((__header)<24 ? 0 : ((4 - ((__header)%4))%4))
> >
> 
> That depends on what the purpose of the L2PAD_SIZE macro is going to
> be. At the moment
> the intention is to have L2PAD_SIZE compute the number of l2pad bytes
> necessary, if a
> payload is present. Detection on whether actually a payload is present
> and whether the
> outcome of this macro should be used should be at the call-sites of this macro.

I personally prefer the current version, I don't see a valid reason for
L2PAD_SIZE to depend on the header size. The caller should check if
the payload is present and L2 padding is required.

Ivo

Re: [rt2x00-users] [PATCH v3 0/4] Further L2 padding fixes.

[Benoit PAPILLAULT]

Ivo van Doorn a écrit :
>>> * L2PAD is only present for data frame and an easy way to check for that is
>>> * to compare header_length with 24 bytes.
>>> */
>>> #define L2PAD_SIZE(__header) \
>>>      ((__header)<24 ? 0 : ((4 - ((__header)%4))%4))
>>>
>> That depends on what the purpose of the L2PAD_SIZE macro is going to
>> be. At the moment
>> the intention is to have L2PAD_SIZE compute the number of l2pad bytes
>> necessary, if a
>> payload is present. Detection on whether actually a payload is present
>> and whether the
>> outcome of this macro should be used should be at the call-sites of this macro.
> 
> I personally prefer the current version, I don't see a valid reason for
> L2PAD_SIZE to depend on the header size. The caller should check if
> the payload is present and L2 padding is required.
> 
> Ivo

Let's move forward and fix bugs later, if any. Just for my curiosity,
who is commiting posted patches in wireless-testing?

Regards,
Benoit

Re: [rt2x00-users] [PATCH v3 0/4] Further L2 padding fixes.

[Ivo Van Doorn]

On Thu, Dec 3, 2009 at 10:36 PM, Benoit PAPILLAULT
<benoit.papillault@free.fr> wrote:
> Ivo van Doorn a écrit :
>>>> * L2PAD is only present for data frame and an easy way to check for that is
>>>> * to compare header_length with 24 bytes.
>>>> */
>>>> #define L2PAD_SIZE(__header) \
>>>>      ((__header)<24 ? 0 : ((4 - ((__header)%4))%4))
>>>>
>>> That depends on what the purpose of the L2PAD_SIZE macro is going to
>>> be. At the moment
>>> the intention is to have L2PAD_SIZE compute the number of l2pad bytes
>>> necessary, if a
>>> payload is present. Detection on whether actually a payload is present
>>> and whether the
>>> outcome of this macro should be used should be at the call-sites of this macro.
>>
>> I personally prefer the current version, I don't see a valid reason for
>> L2PAD_SIZE to depend on the header size. The caller should check if
>> the payload is present and L2 padding is required.
>>
>> Ivo
>
> Let's move forward and fix bugs later, if any. Just for my curiosity,
> who is commiting posted patches in wireless-testing?

John Linville maintains wireless-testing, he is picking up the patches from
the linux-wireless mailinglist and merges them into his tree (and pushes them
upstream).

Ivo