[rt2x00-users] [RFC] rt2800usb: remove usb pad after transmission

[Helmut Schaa]

Hi Jakub,

On Sat, Dec 17, 2011 at 11:47 PM, Jakub Kicinski <kubakici at wp.pl> wrote:
> rt2800usb appends certain pad to frames before they are transferred
> to HW. This pad should be removed once the frame is transmitted
> and before skb is handed back to mac80211.
>
> I send this as RFC because I'm not sure whether you agree with my
> approach. IMHO saving pad length in queue entry will be a cleaner
> solution than calculating actual length of frame from descriptor
> fields.

I agree that this should be fixed since mac80211 might retransmit the
frame for example. And that will then contain the padding in the
payload ...

However, see my inline comments.

> Signed-off-by: Jakub Kicinski <kubakici at wp.pl>
> ---
>  drivers/net/wireless/rt2x00/rt2800usb.c |   17 +++++++++++++++--
>  drivers/net/wireless/rt2x00/rt2800usb.h |   11 +++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index 3778763..e8bd429 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -424,6 +424,7 @@ static void rt2800usb_write_tx_desc(struct queue_entry *entry,
>  static void rt2800usb_write_tx_data(struct queue_entry *entry,
>                                        struct txentry_desc *txdesc)
>  {
> +       struct rt2800usb_tx_queue_entry_priv *entry_priv = entry->priv_data;
>        unsigned int len;
>        int err;
>
> @@ -437,6 +438,7 @@ static void rt2800usb_write_tx_data(struct queue_entry *entry,
>         *                 |<------------- tx_pkt_len ------------->|
>         */
>        len = roundup(entry->skb->len, 4) + 4;
> +       entry_priv->usb_pad = len - entry->skb->len;
>        err = skb_padto(entry->skb, len);
>        if (unlikely(err)) {
>                WARNING(entry->queue->rt2x00dev, "TX SKB padding error, out of memory\n");
> @@ -509,6 +511,13 @@ static bool rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
>        return true;
>  }
>
> +static void rt2800usb_remove_usb_pad(struct queue_entry *entry)
> +{
> +       struct rt2800usb_tx_queue_entry_priv *entry_priv = entry->priv_data;
> +
> +       skb_trim(entry->skb, entry->skb->len - entry_priv->usb_pad);

Might make sense to add a check for (entry_priv->usb_pad <
entry->skb->len) here.

> +}
> +
>  static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
>  {
>        struct data_queue *queue;
> @@ -541,9 +550,13 @@ static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
>                        entry = NULL;
>                }
>
> -               if (entry)
> +               if (entry) {
> +                       if (qid != QID_BEACON)
> +                               rt2800usb_remove_usb_pad(entry);
> +
>                        rt2800_txdone_entry(entry, reg,
>                                            rt2800usb_get_txwi(entry));
> +               }
>        }
>  }
>
> @@ -834,7 +847,7 @@ static const struct data_queue_desc rt2800usb_queue_tx = {
>        .entry_num              = 64,
>        .data_size              = AGGREGATION_SIZE,
>        .desc_size              = TXINFO_DESC_SIZE + TXWI_DESC_SIZE,
> -       .priv_size              = sizeof(struct queue_entry_priv_usb),
> +       .priv_size              = sizeof(struct rt2800usb_tx_queue_entry_priv),
>  };
>
>  static const struct data_queue_desc rt2800usb_queue_bcn = {
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.h b/drivers/net/wireless/rt2x00/rt2800usb.h
> index 671ea35..75eb5fb 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.h
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.h
> @@ -37,6 +37,17 @@
>  #define FIRMWARE_RT2870                        "rt2870.bin"
>  #define FIRMWARE_IMAGE_BASE            0x3000
>
> +/*
> + * rt2800usb TX queue specific entry
> + *
> + * usb_priv: Per entry USB specific information
> + * usb_pad: Length of pad appended to frame
> + */
> +struct rt2800usb_tx_queue_entry_priv {
> +       struct queue_entry_priv_usb usb_priv;
> +       u8 usb_pad;

Any special reason for using an u8?

Isnt't entry->priv_data also used in the generic rt2x00 USB parts (rt2x00usb.c)
but casted to queue_entry_priv_usb. So this needs a comment why it doesn't
break the generic USB routines ...

Helmut