Hi Zefir, [sorry if the message indentation is wrong ... webmail]
From: Zefir Kurtisi <zefir.kurt...@neratec.com> Sent: Friday, August 19, 2016 8:11 PM To: Claudiu Manoil Subject: Re: [PATCH] gianfar: fix size of fragmented frames Hi Claudiu, On 08/19/2016 06:46 PM, Claudiu Manoil wrote: >> -----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. > In fact, this works and I tested it by setting GFAR_RXB_SIZE to 256. Receiving a 1000 byte frame then results in 4 fragments, 3*256 plus the last one sized 1000. The driver then combines 256+256+256+232 (=1000-768) back to a 1000 bytes frame. I don't see why it should not, because skb->len is exactly the size of the fragments added before the last one, so subtracting them from the total frame size should give the size of the last fragment, no? [Claudiu] Thanks for the details. I didn't have much time to look into it (I still don't), and you can never be too careful with this kind of changes. But I agree with your assessment. I'm surprised this didn't come out earlier, like a warning from the stack, since the truesize can be easily exceeded in this case. >> + >> + 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. > Do you question the first part? In cases where the last fragment consists of only the FCS, adding a zero size fragment would falsify skb->truesize. The second expression is fail-safe only - it should never happen given the hardware specs - but if it did, not checking for a sane frame size might cause serious trouble (not sure what skb_add_rx_frag() does with an insanely high size value). If you prefer, I will leave it out in v2 of the patch. [Claudiu] Nice catch with the size > 0 part too. The second part is debatable indeed, it may case confusion implying that size may exceed that limit, which is false, since the point of RX S/G is that a buffer doesn't exceed GFAR_RXB_SIZE. So I'd drop the second check. Other than that, Acked-by: <claudiu.man...@nxp.com> Thanks, Claudiu
