Re: [PATCH net-next] stmmac: align RX buffers

From: Marc Zyngier <maz@kernel.org>
To: Matteo Croce <mcroce@linux.microsoft.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	netdev@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Drew Fustini <drew@beagleboard.org>,
	Emil Renner Berthing <kernel@esmil.dk>,
	Jon Hunter <jonathanh@nvidia.com>, Will Deacon <will@kernel.org>
Subject: Re: [PATCH net-next] stmmac: align RX buffers
Date: Fri, 20 Aug 2021 19:09:04 +0100	[thread overview]
Message-ID: <87eeaoyon3.wl-maz@kernel.org> (raw)
In-Reply-To: <CAFnufp2qFuhMDae20u_dV+aOPfB+zpcEK8D-=8ACE6r4kDn2rw@mail.gmail.com>

On Fri, 20 Aug 2021 18:56:33 +0100,
Matteo Croce <mcroce@linux.microsoft.com> wrote:
> 
> On Fri, Aug 20, 2021 at 7:51 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Fri, 20 Aug 2021 18:35:45 +0100,
> > Matteo Croce <mcroce@linux.microsoft.com> wrote:
> > >
> > > > > I think it's wrong. The original offset was 0, and to align it to the
> > > > > boundary we need to add just NET_IP_ALIGN, which is two.
> > > > > NET_SKB_PAD is a much bigger value, (I think 64), which is used to
> > > > > reserve space to prepend an header, e.g. with tunnels.
> > > >
> > > > How about the other adjustments that Eric mentioned regarding the size
> > > > of the buffer? Aren't they required?
> > > >
> > >
> > > I guess that if stmmac_rx_buf1_len() needed such adjustment, it would
> > > be already broken when XDP is in use.
> > > When you use XDP, stmmac_rx_offset() adds a pretty big headroom of 256
> > > byte, which would easily trigger an overflow if not accounted.
> > > Did you try attaching a simple XDP program on a stock 5.13 kernel?
> >
> > Yes, as mentioned in [1], to which you replied...
> >
> >         M.
> >
> > [1] https://lore.kernel.org/r/87wnohqty1.wl-maz@kernel.org
> >
> 
> Great.
> So I doubt that the adjustment is needed.
> Does it work with all the frame size?

I have no idea. Honestly, you are the one who should be able to answer
these questions, given that you should have worked out how the buffer
allocations work in this particular driver.

This whole "let's try another random set of values until something
sticks" is not how things ought to be done, and doesn't fill me with
the utmost confidence that 5.14 (which apparently may well be cut in
*two days*) is going to have a solid stmmac driver.

I re-re-request that this patch gets reverted until you figure out
what is wrong with the initial patch.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.