Re: [BISECTED] [PATCH v2 8/8] rt2x00: Properly request tx headroom for alignment operations.

[Gertjan van Wingerde <gwingerde@gmail.com>]

On 12/22/09 06:25, Pavel Roskin wrote:
> Quoting Gertjan van Wingerde <gwingerde@gmail.com>:
> 
>>> Perhaps non-zero headroom is not handled correctly?
>>>
>>
>> Hmmm, perhaps the problem is that the headroom is not a multiple of 4.
>> Can you check what happens when you set the extra_tx_headroom fixed  
>> to e.g. 20?
> 
> I tried 64 and 2, and neither worked.  rt2x00dev->ops->extra_tx_headroom
> is 0 for me.  When the rt2x00dev->hw->extra_tx_headroom is 2, I observed
> invalid packets being emitted (using another interface).  The packets
> have two bytes inserted in the beginning of the frame:
> 
> 0000  00 00 1c 00 6f 48 00 00  fc de ee 13 00 00 00 00   ....oH.. ........
> 0010  10 02 85 09 a0 00 b1 b7  00 00 00 00 00 00 40 00   ........ ......@.
> radiotap, 28 bytes ---------------------->
> 2 extra bytes                              <--->
> correct 802.11 header                            <----
> 0020  00 00 ff ff ff ff ff ff  00 d0 41 af ad 2c ff ff   ........ ..A..,..
> 0030  ff ff cf 02 c0 02 00 05  77 62 65 6e 64 01 08 02   ........ wbend...
> 0040  04 0b 16 0c 12 18 24 32  04 30 48 e7 f8 07 9d      ......$2 .0H.... 
> 
> Here's a patch that is working for me, but I would not vouch for its
> correctness.  Signing off just in case it happens to be correct ;-)
> 
> 
> rt2x00: use correct headroom for transmission
> 
> Use rt2x00dev->ops->extra_tx_headroom, not
> rt2x00dev->hw->extra_tx_headroom in the tx code, as the later includes
> headroom only meant for the monitor mode.
> 
> Signed-off-by: Pavel Roskin <proski@gnu.org>

Doh, that's it. Here we should indeed only take the real extra TX headroom required by
the driver into account. Seems like I goofed up in a preparatory patch that centralized
setting of rt2x00dev->hw->extra_tx_headroom.

Can you confirm that everything works okay with the original code and this patch applied?

In any way:
Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

John, with this you can reinstate the original patch as well. Let me know if you want
me to resubmit that one with this one included in it.


> ---
>  drivers/net/wireless/rt2x00/rt2x00queue.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
> index 3d8fb68..0b4801a 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
> @@ -104,7 +104,7 @@ void rt2x00queue_map_txskb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
>  	 * is also mapped to the DMA so it can be used for transfering
>  	 * additional descriptor information to the hardware.
>  	 */
> -	skb_push(skb, rt2x00dev->hw->extra_tx_headroom);
> +	skb_push(skb, rt2x00dev->ops->extra_tx_headroom);
>  
>  	skbdesc->skb_dma =
>  	    dma_map_single(rt2x00dev->dev, skb->data, skb->len, DMA_TO_DEVICE);
> @@ -112,7 +112,7 @@ void rt2x00queue_map_txskb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
>  	/*
>  	 * Restore data pointer to original location again.
>  	 */
> -	skb_pull(skb, rt2x00dev->hw->extra_tx_headroom);
> +	skb_pull(skb, rt2x00dev->ops->extra_tx_headroom);
>  
>  	skbdesc->flags |= SKBDESC_DMA_MAPPED_TX;
>  }
> @@ -134,7 +134,7 @@ void rt2x00queue_unmap_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
>  		 * by the driver, but it was actually mapped to DMA.
>  		 */
>  		dma_unmap_single(rt2x00dev->dev, skbdesc->skb_dma,
> -				 skb->len + rt2x00dev->hw->extra_tx_headroom,
> +				 skb->len + rt2x00dev->ops->extra_tx_headroom,
>  				 DMA_TO_DEVICE);
>  		skbdesc->flags &= ~SKBDESC_DMA_MAPPED_TX;
>  	}
>