[rt2x00-users] [PATCH] rt2x00: Fix rt2800 USB TX Path DMA issue

[Ivo Van Doorn]

Hi Jay,

> Am Freitag 05 November 2010 schrieb Jay Hung:
>> rt2800usb chips need to add 1~3 bytes zero padding after each 802.11 header & payload,
>> and at the end need to add 4 bytes zero padding whether doing TX bulk aggregation or not,
>> so dma_map_single also need to include these areas when doing USB TX bulk out.
>>
>> TXINFO_W0_USB_DMA_TX_PKT_LEN in TXINFO must include 1-3 bytes padding after 802.11 header & payload
>> but do not include 4 bytes end zero padding.
>>
>> In rt2800usb_get_tx_data_len do not consider multiple of the USB packet size case, sometimes this will
>> cause USB DMA problem.
>
> Just one minor comment: could you please add a comment to the appropriate code
> sections as well that describe the desired behavior?
>
> I'll let the others comment about the functional changes in the patch since
> I'm not familiar at all with the USB drivers.

I've been testing the patch yesterday and today, and it indeed seems
to resolve all TX
DMA failures. However I do have some questions.

>> Signed-off-by: Jay Hung <jay_hung at ralinktech.com>
>> ---
>>  drivers/net/wireless/rt2x00/rt2800usb.c |   25 +++++++++++++++----------
>>  1 files changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
>> index 3dff56e..1a3b2cc 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
>> @@ -267,7 +267,7 @@ static void rt2800usb_write_tx_desc(struct queue_entry *entry,
>>        */
>>       rt2x00_desc_read(txi, 0, &word);
>>       rt2x00_set_field32(&word, TXINFO_W0_USB_DMA_TX_PKT_LEN,
>> -                        entry->skb->len - TXINFO_DESC_SIZE);
>> +                        entry->skb->len - TXINFO_DESC_SIZE - 4);

As helmut indicated, please add a comment here why the length is being reduced
with 4 bytes.

>> +static void rt2800usb_write_tx_data(struct queue_entry *entry,
>> +                                     struct txentry_desc *txdesc)
>> +{
>> +     u8 padding_len;
>> +
>> +     rt2800_write_tx_data(entry, txdesc);
>> +
>> +     padding_len = roundup(entry->skb->len + 4, 4) - entry->skb->len;
>> +     if (padding_len)
>> +             memset(skb_put(entry->skb, padding_len), 0, padding_len);
>> +}

In rt2800usb_get_tx_data_len we previously took
the usb_maxpacket into account. I thought that this was copied from
the original drivers from Ralink. So should it still be kept in mind,
or can it be
completely discarded (Please add some documentation here as well, about what
is exactly happening here).

>>  /*
>>   * TX data initialization
>>   */
>> @@ -292,14 +304,7 @@ static int rt2800usb_get_tx_data_len(struct queue_entry *entry)
>>  {
>>       int length;
>>
>> -     /*
>> -      * The length _must_ include 4 bytes padding,
>> -      * it should always be multiple of 4,
>> -      * but it must _not_ be a multiple of the USB packet size.
>> -      */
>> -     length = roundup(entry->skb->len + 4, 4);
>> -     length += (4 * !(length % entry->queue->usb_maxpacket));
>> -
>> +     length = entry->skb->len;
>>       return length;
>>  }

I think this can be simplified to:
  return entry->skb->len;
as we no longer need the length variable.

Thanks.

Ivo