>-----Original Message-----
>From: Zefir Kurtisi [mailto:zefir.kurt...@neratec.com]
>Sent: Friday, August 19, 2016 12:16 PM
>To: netdev@vger.kernel.org
>Cc: claudiu.man...@freescale.com
>Subject: [PATCH] gianfar: fix size of fragmented frames
>
>The eTSEC RxBD 'Data Length' field is context depening:
>for the last fragment it contains the full frame size,
>while fragments contain the fragment size, which equals
>the value written to register MRBLR.
>
According to RM the last fragment has the whole packet length indeed,
and this should apply to fragmented frames too:
" Data length, written by the eTSEC.
Data length is the number of octets written by the eTSEC into this BD's data
buffer if L is cleared
(the value is equal to MRBLR), or, if L is set, the length of the frame
including CRC, FCB (if
RCTRL[PRSDEP > 00), preamble (if MACCFG2[PreAmRxEn]=1), time stamp (if
RCTRL[TS] = 1)
and any padding (RCTRL[PAL]). "
>This differentiation is missing in the gianfar driver,
>which causes data corruption as soon as the hardware
>starts to fragment receiving frames. As a result, the
>size of fragmented frames is increased by
>(nr_frags - 1) * MRBLR
>
>We first noticed this issue working with DSA, where a
>1540 octet frame is fragmented by the hardware and
>reconstructed by the driver as 3076 octet frame.
>
>This patch fixes the problem by adjusting the size of
>the last fragment.
>
>Signed-off-by: Zefir Kurtisi <zefir.kurt...@neratec.com>
>---
> drivers/net/ethernet/freescale/gianfar.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/gianfar.c
>b/drivers/net/ethernet/freescale/gianfar.c
>index d20935d..4187280 100644
>--- a/drivers/net/ethernet/freescale/gianfar.c
>+++ b/drivers/net/ethernet/freescale/gianfar.c
>@@ -2922,17 +2922,24 @@ static bool gfar_add_rx_frag(struct gfar_rx_buff *rxb,
>u32 lstatus,
> {
> unsigned int size = lstatus & BD_LENGTH_MASK;
> struct page *page = rxb->page;
>+ bool last = !!(lstatus & BD_LFLAG(RXBD_LAST));
>
> /* Remove the FCS from the packet length */
>- if (likely(lstatus & BD_LFLAG(RXBD_LAST)))
>+ if (last)
> size -= ETH_FCS_LEN;
>
>- if (likely(first))
>+ if (likely(first)) {
> skb_put(skb, size);
>- else
>- skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>- rxb->page_offset + RXBUF_ALIGNMENT,
>- size, GFAR_RXB_TRUESIZE);
>+ } else {
>+ /* the last fragments' length contains the full frame length */
>+ if (last)
>+ size -= skb->len;
While I agree with your finding, I don't think this works for packets having
more
than 2 buffers (head + 1 fragment).
How's this supposed to work for a skb with 2 fragments, for instance?
I think this needs more thoughtful consideration.
>+
>+ if (size > 0 && size <= GFAR_RXB_SIZE)
Why do you need this check?
The h/w ensures that the buffers won't exceed GFAR_RXB_SIZE
(which is MRBL), size 0 is also not possible.
>+ skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>+ rxb->page_offset + RXBUF_ALIGNMENT,
>+ size, GFAR_RXB_TRUESIZE);
>+ }
>
> /* try reuse page */
> if (unlikely(page_count(page) != 1))
>--
>2.7.4
