On Thu, Aug 12, 2021 at 1:05 PM Marc Zyngier wrote: > > On Thu, 12 Aug 2021 11:18:35 +0100, > Matteo Croce wrote: > > > > On Thu, 12 Aug 2021 10:48:03 +0200 > > Eric Dumazet wrote: > > > > > > > > > > > On 8/11/21 4:16 PM, Marc Zyngier wrote: > > > > On Wed, 11 Aug 2021 13:53:59 +0100, > > > > Eric Dumazet wrote: > > > > > > > >> Are you sure you do not need to adjust stmmac_set_bfsize(), > > > >> stmmac_rx_buf1_len() and stmmac_rx_buf2_len() ? > > > >> > > > >> Presumably DEFAULT_BUFSIZE also want to be increased by NET_SKB_PAD > > > >> > > > >> Patch for stmmac_rx_buf1_len() : > > > >> > > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index > > > >> 7b8404a21544cf29668e8a14240c3971e6bce0c3..041a74e7efca3436bfe3e17f972dd156173957a9 > > > >> 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -4508,12 > > > >> +4508,12 @@ static unsigned int stmmac_rx_buf1_len(struct > > > >> stmmac_priv *priv, /* First descriptor, not last descriptor and > > > >> not split header */ if (status & rx_not_ls) > > > >> - return priv->dma_buf_sz; > > > >> + return priv->dma_buf_sz - NET_SKB_PAD - > > > >> NET_IP_ALIGN; > > > >> plen = stmmac_get_rx_frame_len(priv, p, coe); > > > >> > > > >> /* First descriptor and last descriptor and not split > > > >> header */ > > > >> - return min_t(unsigned int, priv->dma_buf_sz, plen); > > > >> + return min_t(unsigned int, priv->dma_buf_sz - NET_SKB_PAD > > > >> - NET_IP_ALIGN, plen); } > > > >> > > > >> static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv, > > > > > > > > Feels like a major deficiency of the original patch. Happy to test a > > > > more complete patch if/when you have one. > > > > > > I wont have time in the immediate future. > > > > > > Matteo, if you do not work on a fix, I suggest we revert > > > a955318fe67ec0d962760b5ee58e74bffaf649b8 stmmac: align RX buffers > > > > > > before a more polished version can be submitted. > > > > > > > Better to use stmmac_rx_offset() so to have the correct length when > > using XDP. Also, when XDP is enabled, the offset was > > XDP_PACKET_HEADROOM (i.e. 256 bytes) even before the change, so it > > could be already broken. Mark, can you try on the Jetson TX2 by > > attaching an XDP program and see if it works without my change? > > Sorry, you'll have to hold my hand here, as I know exactly nothing > about XDP.... > Attach the attached object with: ip link set eth0 xdp object passall.o This is an empty XDP program, its source: __attribute__((section("prog"), used)) int xdp_main(struct xdp_md *ctx) { return XDP_PASS; } Every packet will pass untouched, but the offset will be shifted from 0 to 256 bytes, which could trigger the problem anyway: > > A possible fix, which takes in account also the XDP headroom for > > stmmac_rx_buf1_len() only could be (only compile tested, I don't have > > the hardware now): > > However, this doesn't fix my issue. I still get all sort of > corruption. Probably stmmac_rx_buf2_len() also need adjusting (it has > a similar logic as its buf1 counterpart...) > > Unless you can fix it very quickly, and given that we're towards the > end of the cycle, I'd be more comfortable if we reverted this patch. > Can it be that the HW can't do DMA on an address which is not word aligned? What if you replace NET_SKB_PAD with, let's say, 8? Regards, -- per aspera ad upstream