Re: [PATCH 1/2] rt2x00: Disable RX aggregation for rt2800usb

Re: [PATCH 1/2] rt2x00: Disable RX aggregation for rt2800usb

[Ivo van Doorn]

On Monday 30 November 2009, Benoit Papillault wrote:
> From: Benoit PAPILLAULT <benoit.papillault@free.fr>
> 
> RX aggregation is a way to receive multiple 802.11 frames in one RX buffer.
> However, we don't know yet how to handle this case in rt2800usb_fill_rxdone
> and this has probably no impact on RX performance as well, so we disable it
> 
> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index af85d18..ed31763 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -295,9 +295,7 @@ static int rt2800usb_enable_radio(struct rt2x00_dev *rt2x00dev)
>  
>  	rt2800_register_read(rt2x00dev, USB_DMA_CFG, &reg);
>  	rt2x00_set_field32(&reg, USB_DMA_CFG_PHY_CLEAR, 0);
> -	/* Don't use bulk in aggregation when working with USB 1.1 */
> -	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN,
> -			   (rt2x00dev->rx->usb_maxpacket == 512));
> +	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN, 0);
>  	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_TIMEOUT, 128);
>  	/*
>  	 * Total room for RX frames in kilobytes, PBF might still exceed


Are you sure this works? I think Luis tried this 2 weeks ago and it didn't
disable the aggregation... (Luis, please confirm that this was indeed the change you made...)

Ivo

Re: [rt2x00-users] [PATCH 1/2] rt2x00: Disable RX aggregation for rt2800usb

[Luis Correia]

On Tue, Dec 1, 2009 at 19:17, Ivo van Doorn <ivdoorn@gmail.com> wrote:
> On Monday 30 November 2009, Benoit Papillault wrote:
>> From: Benoit PAPILLAULT <benoit.papillault@free.fr>
>>
>> RX aggregation is a way to receive multiple 802.11 frames in one RX buffer.
>> However, we don't know yet how to handle this case in rt2800usb_fill_rxdone
>> and this has probably no impact on RX performance as well, so we disable it
>>
>> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
>> ---
>>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +---
>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
>> index af85d18..ed31763 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
>> @@ -295,9 +295,7 @@ static int rt2800usb_enable_radio(struct rt2x00_dev *rt2x00dev)
>>
>>       rt2800_register_read(rt2x00dev, USB_DMA_CFG, &reg);
>>       rt2x00_set_field32(&reg, USB_DMA_CFG_PHY_CLEAR, 0);
>> -     /* Don't use bulk in aggregation when working with USB 1.1 */
>> -     rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN,
>> -                        (rt2x00dev->rx->usb_maxpacket == 512));
>> +     rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN, 0);
>>       rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_TIMEOUT, 128);
>>       /*
>>        * Total room for RX frames in kilobytes, PBF might still exceed
>
>
> Are you sure this works? I think Luis tried this 2 weeks ago and it didn't
> disable the aggregation... (Luis, please confirm that this was indeed the change you made...)

Yes, it was one of the changes I made.

However, I wouldn't say that it didn't disable aggregation, it just
didn't fix the problem I was pursuing (receiving 11n beacon frames).

So I guess that for now, we could just go with it disabled and try
later on to enable it when the driver is more useable.

Luis Correia

Re: [rt2x00-users] [PATCH 1/2] rt2x00: Disable RX aggregation for rt2800usb

[Benoit PAPILLAULT]

Luis Correia a écrit :
> On Tue, Dec 1, 2009 at 19:17, Ivo van Doorn <ivdoorn@gmail.com> wrote:
>> On Monday 30 November 2009, Benoit Papillault wrote:
>>> From: Benoit PAPILLAULT <benoit.papillault@free.fr>
>>>
>>> RX aggregation is a way to receive multiple 802.11 frames in one RX buffer.
>>> However, we don't know yet how to handle this case in rt2800usb_fill_rxdone
>>> and this has probably no impact on RX performance as well, so we disable it
>>>
>>> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>
>>> ---
>>>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +---
>>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
>>> index af85d18..ed31763 100644
>>> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
>>> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
>>> @@ -295,9 +295,7 @@ static int rt2800usb_enable_radio(struct rt2x00_dev *rt2x00dev)
>>>
>>>       rt2800_register_read(rt2x00dev, USB_DMA_CFG, &reg);
>>>       rt2x00_set_field32(&reg, USB_DMA_CFG_PHY_CLEAR, 0);
>>> -     /* Don't use bulk in aggregation when working with USB 1.1 */
>>> -     rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN,
>>> -                        (rt2x00dev->rx->usb_maxpacket == 512));
>>> +     rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN, 0);
>>>       rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_TIMEOUT, 128);
>>>       /*
>>>        * Total room for RX frames in kilobytes, PBF might still exceed
>>
>> Are you sure this works? I think Luis tried this 2 weeks ago and it didn't
>> disable the aggregation... (Luis, please confirm that this was indeed the change you made...)
> 
> Yes, it was one of the changes I made.
> 
> However, I wouldn't say that it didn't disable aggregation, it just
> didn't fix the problem I was pursuing (receiving 11n beacon frames).
> 
> So I guess that for now, we could just go with it disabled and try
> later on to enable it when the driver is more useable.
> 
> Luis Correia
> 

Without this patch, the driver is receiving several 802.11 frames inside
the same buffer and only one is sent to mac80211, so this patch is a
quick fix for that issue.

Regards,
Benoit

Re: [PATCH 2/2] rt2x00: Fix rt2800usb RX frame format and as such L2PAD

[Ivo van Doorn]

On Monday 30 November 2009, Benoit Papillault wrote:
> According to Ralink source code, the RX frame format is RXINFO + RXWI +
> 802.11 frame + RXD, including various padding. Before this patch, we
> were using RXD + RXWI + 802.11 frame, so RXD was not correct.
> 
> Doing this, we fix the L2PAD bit which is now correctly set on received
> frames.
> 
> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>

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

Re: [PATCH 2/2] rt2x00: Fix rt2800usb RX frame format and as such L2PAD

[Gertjan van Wingerde]

On 11/30/09 23:21, Benoit Papillault wrote:
> According to Ralink source code, the RX frame format is RXINFO + RXWI +
> 802.11 frame + RXD, including various padding. Before this patch, we
> were using RXD + RXWI + 802.11 frame, so RXD was not correct.
> 
> Doing this, we fix the L2PAD bit which is now correctly set on received
> frames.
> 
> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>

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

> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |   40 ++++++++++----
>  drivers/net/wireless/rt2x00/rt2800usb.h |   90 ++++++++++++++++++++++++-------
>  2 files changed, 98 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index ed31763..fc8d7a7 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -571,41 +571,57 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>  {
>  	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
>  	struct skb_frame_desc *skbdesc = get_skb_frame_desc(entry->skb);
> -	__le32 *rxd = (__le32 *)entry->skb->data;
> +	__le32 *rxi = (__le32 *)entry->skb->data;
>  	__le32 *rxwi;
> -	u32 rxd0;
> +	__le32 *rxd;
> +	u32 rxi0;
>  	u32 rxwi0;
>  	u32 rxwi1;
>  	u32 rxwi2;
>  	u32 rxwi3;
> +	u32 rxd0;
> +	int rx_pkt_len;
> +
> +	/*
> +	 * RX frame format is :
> +	 * | RXINFO | RXWI | header | L2 pad | payload | pad | RXD | USB pad |
> +	 *          |<------------ rx_pkt_len -------------->|
> +	 */
> +	rt2x00_desc_read(rxi, 0, &rxi0);
> +	rx_pkt_len = rt2x00_get_field32(rxi0, RXINFO_W0_USB_DMA_RX_PKT_LEN);
> +
> +	rxwi = (__le32 *)(entry->skb->data + RXINFO_DESC_SIZE);
> +
> +	/*
> +	 * FIXME : we need to check for rx_pkt_len validity
> +	 */
> +	rxd = (__le32 *)(entry->skb->data + RXINFO_DESC_SIZE + rx_pkt_len);
>  
>  	/*
>  	 * Copy descriptor to the skbdesc->desc buffer, making it safe from
>  	 * moving of frame data in rt2x00usb.
>  	 */
> -	memcpy(skbdesc->desc, rxd, skbdesc->desc_len);
> -	rxd = (__le32 *)skbdesc->desc;
> -	rxwi = &rxd[RXINFO_DESC_SIZE / sizeof(__le32)];
> +	memcpy(skbdesc->desc, rxi, skbdesc->desc_len);
>  
>  	/*
>  	 * It is now safe to read the descriptor on all architectures.
>  	 */
> -	rt2x00_desc_read(rxd, 0, &rxd0);
>  	rt2x00_desc_read(rxwi, 0, &rxwi0);
>  	rt2x00_desc_read(rxwi, 1, &rxwi1);
>  	rt2x00_desc_read(rxwi, 2, &rxwi2);
>  	rt2x00_desc_read(rxwi, 3, &rxwi3);
> +	rt2x00_desc_read(rxd, 0, &rxd0);
>  
> -	if (rt2x00_get_field32(rxd0, RXINFO_W0_CRC_ERROR))
> +	if (rt2x00_get_field32(rxd0, RXD_W0_CRC_ERROR))
>  		rxdesc->flags |= RX_FLAG_FAILED_FCS_CRC;
>  
>  	if (test_bit(CONFIG_SUPPORT_HW_CRYPTO, &rt2x00dev->flags)) {
>  		rxdesc->cipher = rt2x00_get_field32(rxwi0, RXWI_W0_UDF);
>  		rxdesc->cipher_status =
> -		    rt2x00_get_field32(rxd0, RXINFO_W0_CIPHER_ERROR);
> +		    rt2x00_get_field32(rxd0, RXD_W0_CIPHER_ERROR);
>  	}
>  
> -	if (rt2x00_get_field32(rxd0, RXINFO_W0_DECRYPTED)) {
> +	if (rt2x00_get_field32(rxd0, RXD_W0_DECRYPTED)) {
>  		/*
>  		 * Hardware has stripped IV/EIV data from 802.11 frame during
>  		 * decryption. Unfortunately the descriptor doesn't contain
> @@ -620,10 +636,10 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>  			rxdesc->flags |= RX_FLAG_MMIC_ERROR;
>  	}
>  
> -	if (rt2x00_get_field32(rxd0, RXINFO_W0_MY_BSS))
> +	if (rt2x00_get_field32(rxd0, RXD_W0_MY_BSS))
>  		rxdesc->dev_flags |= RXDONE_MY_BSS;
>  
> -	if (rt2x00_get_field32(rxd0, RXINFO_W0_L2PAD)) {
> +	if (rt2x00_get_field32(rxd0, RXD_W0_L2PAD)) {
>  		rxdesc->dev_flags |= RXDONE_L2PAD;
>  		skbdesc->flags |= SKBDESC_L2_PADDED;
>  	}
> @@ -661,7 +677,7 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>  	 * Remove RXWI descriptor from start of buffer.
>  	 */
>  	skb_pull(entry->skb, skbdesc->desc_len);
> -	skb_trim(entry->skb, rxdesc->size);
> +	skb_trim(entry->skb, rx_pkt_len - RXWI_DESC_SIZE);
>  }
>  
>  /*
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.h b/drivers/net/wireless/rt2x00/rt2800usb.h
> index 1e4340a..d1d8ae9 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.h
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.h
> @@ -79,6 +79,8 @@
>   */
>  #define TXINFO_DESC_SIZE		( 1 * sizeof(__le32) )
>  #define RXINFO_DESC_SIZE		( 1 * sizeof(__le32) )
> +#define RXWI_DESC_SIZE			( 4 * sizeof(__le32) )
> +#define RXD_DESC_SIZE			( 1 * sizeof(__le32) )
>  
>  /*
>   * TX Info structure
> @@ -101,6 +103,54 @@
>  #define TXINFO_W0_USB_DMA_TX_BURST	FIELD32(0x80000000)
>  
>  /*
> + * RX Info structure
> + */
> +
> +/*
> + * Word 0
> + */
> +
> +#define RXINFO_W0_USB_DMA_RX_PKT_LEN	FIELD32(0x0000ffff)
> +
> +/*
> + * RX WI structure
> + */
> +
> +/*
> + * Word0
> + */
> +#define RXWI_W0_WIRELESS_CLI_ID		FIELD32(0x000000ff)
> +#define RXWI_W0_KEY_INDEX		FIELD32(0x00000300)
> +#define RXWI_W0_BSSID			FIELD32(0x00001c00)
> +#define RXWI_W0_UDF			FIELD32(0x0000e000)
> +#define RXWI_W0_MPDU_TOTAL_BYTE_COUNT	FIELD32(0x0fff0000)
> +#define RXWI_W0_TID			FIELD32(0xf0000000)
> +
> +/*
> + * Word1
> + */
> +#define RXWI_W1_FRAG			FIELD32(0x0000000f)
> +#define RXWI_W1_SEQUENCE		FIELD32(0x0000fff0)
> +#define RXWI_W1_MCS			FIELD32(0x007f0000)
> +#define RXWI_W1_BW			FIELD32(0x00800000)
> +#define RXWI_W1_SHORT_GI		FIELD32(0x01000000)
> +#define RXWI_W1_STBC			FIELD32(0x06000000)
> +#define RXWI_W1_PHYMODE			FIELD32(0xc0000000)
> +
> +/*
> + * Word2
> + */
> +#define RXWI_W2_RSSI0			FIELD32(0x000000ff)
> +#define RXWI_W2_RSSI1			FIELD32(0x0000ff00)
> +#define RXWI_W2_RSSI2			FIELD32(0x00ff0000)
> +
> +/*
> + * Word3
> + */
> +#define RXWI_W3_SNR0			FIELD32(0x000000ff)
> +#define RXWI_W3_SNR1			FIELD32(0x0000ff00)
> +
> +/*
>   * RX descriptor format for RX Ring.
>   */
>  
> @@ -115,25 +165,25 @@
>   * AMSDU: rx with 802.3 header, not 802.11 header.
>   */
>  
> -#define RXINFO_W0_BA			FIELD32(0x00000001)
> -#define RXINFO_W0_DATA			FIELD32(0x00000002)
> -#define RXINFO_W0_NULLDATA		FIELD32(0x00000004)
> -#define RXINFO_W0_FRAG			FIELD32(0x00000008)
> -#define RXINFO_W0_UNICAST_TO_ME		FIELD32(0x00000010)
> -#define RXINFO_W0_MULTICAST		FIELD32(0x00000020)
> -#define RXINFO_W0_BROADCAST		FIELD32(0x00000040)
> -#define RXINFO_W0_MY_BSS		FIELD32(0x00000080)
> -#define RXINFO_W0_CRC_ERROR		FIELD32(0x00000100)
> -#define RXINFO_W0_CIPHER_ERROR		FIELD32(0x00000600)
> -#define RXINFO_W0_AMSDU			FIELD32(0x00000800)
> -#define RXINFO_W0_HTC			FIELD32(0x00001000)
> -#define RXINFO_W0_RSSI			FIELD32(0x00002000)
> -#define RXINFO_W0_L2PAD			FIELD32(0x00004000)
> -#define RXINFO_W0_AMPDU			FIELD32(0x00008000)
> -#define RXINFO_W0_DECRYPTED		FIELD32(0x00010000)
> -#define RXINFO_W0_PLCP_RSSI		FIELD32(0x00020000)
> -#define RXINFO_W0_CIPHER_ALG		FIELD32(0x00040000)
> -#define RXINFO_W0_LAST_AMSDU		FIELD32(0x00080000)
> -#define RXINFO_W0_PLCP_SIGNAL		FIELD32(0xfff00000)
> +#define RXD_W0_BA			FIELD32(0x00000001)
> +#define RXD_W0_DATA			FIELD32(0x00000002)
> +#define RXD_W0_NULLDATA			FIELD32(0x00000004)
> +#define RXD_W0_FRAG			FIELD32(0x00000008)
> +#define RXD_W0_UNICAST_TO_ME		FIELD32(0x00000010)
> +#define RXD_W0_MULTICAST		FIELD32(0x00000020)
> +#define RXD_W0_BROADCAST		FIELD32(0x00000040)
> +#define RXD_W0_MY_BSS			FIELD32(0x00000080)
> +#define RXD_W0_CRC_ERROR		FIELD32(0x00000100)
> +#define RXD_W0_CIPHER_ERROR		FIELD32(0x00000600)
> +#define RXD_W0_AMSDU			FIELD32(0x00000800)
> +#define RXD_W0_HTC			FIELD32(0x00001000)
> +#define RXD_W0_RSSI			FIELD32(0x00002000)
> +#define RXD_W0_L2PAD			FIELD32(0x00004000)
> +#define RXD_W0_AMPDU			FIELD32(0x00008000)
> +#define RXD_W0_DECRYPTED		FIELD32(0x00010000)
> +#define RXD_W0_PLCP_RSSI		FIELD32(0x00020000)
> +#define RXD_W0_CIPHER_ALG		FIELD32(0x00040000)
> +#define RXD_W0_LAST_AMSDU		FIELD32(0x00080000)
> +#define RXD_W0_PLCP_SIGNAL		FIELD32(0xfff00000)
>  
>  #endif /* RT2800USB_H */

Re: [PATCH 1/2] rt2x00: Disable RX aggregation for rt2800usb

[Gertjan van Wingerde]

On 11/30/09 23:21, Benoit Papillault wrote:
> From: Benoit PAPILLAULT <benoit.papillault@free.fr>
> 
> RX aggregation is a way to receive multiple 802.11 frames in one RX buffer.
> However, we don't know yet how to handle this case in rt2800usb_fill_rxdone
> and this has probably no impact on RX performance as well, so we disable it
> 
> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>

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

> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index af85d18..ed31763 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -295,9 +295,7 @@ static int rt2800usb_enable_radio(struct rt2x00_dev *rt2x00dev)
>  
>  	rt2800_register_read(rt2x00dev, USB_DMA_CFG, &reg);
>  	rt2x00_set_field32(&reg, USB_DMA_CFG_PHY_CLEAR, 0);
> -	/* Don't use bulk in aggregation when working with USB 1.1 */
> -	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN,
> -			   (rt2x00dev->rx->usb_maxpacket == 512));
> +	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN, 0);
>  	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_TIMEOUT, 128);
>  	/*
>  	 * Total room for RX frames in kilobytes, PBF might still exceed

Re: [PATCH 1/2] rt2x00: Disable RX aggregation for rt2800usb

[Ivo van Doorn]

On Monday 30 November 2009, Benoit Papillault wrote:
> From: Benoit PAPILLAULT <benoit.papillault@free.fr>
> 
> RX aggregation is a way to receive multiple 802.11 frames in one RX buffer.
> However, we don't know yet how to handle this case in rt2800usb_fill_rxdone
> and this has probably no impact on RX performance as well, so we disable it
> 
> Signed-off-by: Benoit Papillault <benoit.papillault@free.fr>

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

> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index af85d18..ed31763 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -295,9 +295,7 @@ static int rt2800usb_enable_radio(struct rt2x00_dev *rt2x00dev)
>  
>  	rt2800_register_read(rt2x00dev, USB_DMA_CFG, &reg);
>  	rt2x00_set_field32(&reg, USB_DMA_CFG_PHY_CLEAR, 0);
> -	/* Don't use bulk in aggregation when working with USB 1.1 */
> -	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN,
> -			   (rt2x00dev->rx->usb_maxpacket == 512));
> +	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN, 0);
>  	rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_TIMEOUT, 128);
>  	/*
>  	 * Total room for RX frames in kilobytes, PBF might still exceed