2019-02-11 08:37:55

by Stanislaw Gruszka

Subject: [RFC] mt76x02u: correct pad for fragments

Modifying skb->len & data_len doesn't look correct. Issue is not critical
we just pad 2 times, except first padding is not filled with zeros.

However I'm not sure if we should not add pad to all skb's in frags
list.

Additionally remove unlikely(pad) condition, we always pad for at least
four bytes what is needed by HW.

Signed-off-by: Stanislaw Gruszka <[email protected]>
---
.../wireless/mediatek/mt76/mt76x02_usb_core.c | 22 +++++++------------
1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
index dc2226c722dd..c7ca2d93720a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
@@ -49,21 +49,15 @@ int mt76x02u_skb_dma_info(struct sk_buff *skb, int port, u32 flags)
FIELD_PREP(MT_TXD_INFO_DPORT, port) | flags;
put_unaligned_le32(info, skb_push(skb, sizeof(info)));

- pad = round_up(skb->len, 4) + 4 - skb->len;
- skb_walk_frags(skb, iter) {
+ skb_walk_frags(skb, iter)
last = iter;
- if (!iter->next) {
- skb->data_len += pad;
- skb->len += pad;
- break;
- }
- }
-
- if (unlikely(pad)) {
- if (skb_pad(last, pad))
- return -ENOMEM;
- __skb_put(last, pad);
- }
+
+ /* Add zero pad of 4 - 7 bytes at the end of buffer */
+ pad = round_up(skb->len, 4) + 4 - skb->len;
+ if (skb_pad(last, pad))
+ return -ENOMEM;
+ __skb_put(last, pad);
+
return 0;
}

--
2.19.2

2019-02-11 09:47:16

by Lorenzo Bianconi

Subject: Re: [RFC] mt76x02u: correct pad for fragments

> Modifying skb->len & data_len doesn't look correct. Issue is not critical
> we just pad 2 times, except first padding is not filled with zeros.
>
> However I'm not sure if we should not add pad to all skb's in frags
> list.

Hi Stanislaw,

IIRC the first packet of a A-MSDU burst keeps track of the burst length
so if we add the a pad to last one we need to update the skb->len/data_len
of the first one. No need to pad all skbs in the frag list, just last one

Regards,
Lorenzo

> Additionally remove unlikely(pad) condition, we always pad for at least
> four bytes what is needed by HW.
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> .../wireless/mediatek/mt76/mt76x02_usb_core.c | 22 +++++++------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
> index dc2226c722dd..c7ca2d93720a 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
> @@ -49,21 +49,15 @@ int mt76x02u_skb_dma_info(struct sk_buff *skb, int port, u32 flags)
> FIELD_PREP(MT_TXD_INFO_DPORT, port) | flags;
> put_unaligned_le32(info, skb_push(skb, sizeof(info)));
>
> - pad = round_up(skb->len, 4) + 4 - skb->len;
> - skb_walk_frags(skb, iter) {
> + skb_walk_frags(skb, iter)
> last = iter;
> - if (!iter->next) {
> - skb->data_len += pad;
> - skb->len += pad;
> - break;
> - }
> - }
> -
> - if (unlikely(pad)) {
> - if (skb_pad(last, pad))
> - return -ENOMEM;
> - __skb_put(last, pad);
> - }
> +
> + /* Add zero pad of 4 - 7 bytes at the end of buffer */
> + pad = round_up(skb->len, 4) + 4 - skb->len;
> + if (skb_pad(last, pad))
> + return -ENOMEM;
> + __skb_put(last, pad);
> +
> return 0;
> }
>
> --
> 2.19.2
>