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:41:00 +0100 [thread overview]
Message-ID: <87bl5syn5v.wl-maz@kernel.org> (raw)
In-Reply-To: <CAFnufp22=M0NRvaAcXgfUVzA2HNFLwxO6Bwp+GZk56S3ycLbNQ@mail.gmail.com>
On Fri, 20 Aug 2021 19:14:22 +0100,
Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> On Fri, Aug 20, 2021 at 8:09 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > 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,
> >
>
> I would have done it, but I'll not have the hardware until next week at least,
> otherwise I'd have tried all these tests myself.
>
> I'm sure that NET_SKB_PAD doesn't need to be there, if just removing
> it fixes the problem, consider applying it and put a Fixes tag.
No, I don't think that's the right thing to do. A patch breaks a
driver, and the author of the patch is not in a position to fix it.
That's OK, these things happen, it's just bad timing.
But I don't understand this part of the kernel well enough to submit a
patch based on a sample of *one*, at the last minute, just because "it
works for me", and have the confidence that it doesn't break anything
else.
I have now posted a revert of the original patch[1]. I'll be happy to
work with you, with a less pressure, in order to have something that
works for everyone in the next cycle.
Thanks,
M.
[1] https://lore.kernel.org/netdev/20210820183002.457226-1-maz@kernel.org
--
Without deviation from the norm, progress is not possible.
next prev parent reply other threads:[~2021-08-20 18:41 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-14 2:25 [PATCH net-next] stmmac: align RX buffers Matteo Croce
2021-06-14 19:51 ` David Miller
2021-06-14 23:21 ` Matteo Croce
2021-06-15 17:28 ` David Miller
2021-06-15 17:30 ` patchwork-bot+netdevbpf
2021-08-10 19:07 ` Marc Zyngier
2021-08-11 10:28 ` Thierry Reding
2021-08-11 12:53 ` Eric Dumazet
2021-08-11 14:16 ` Marc Zyngier
2021-08-12 8:48 ` Eric Dumazet
2021-08-12 10:18 ` Matteo Croce
2021-08-12 11:05 ` Marc Zyngier
2021-08-12 11:18 ` Matteo Croce
2021-08-19 16:29 ` Marc Zyngier
2021-08-20 10:37 ` Matteo Croce
2021-08-20 16:26 ` Marc Zyngier
2021-08-20 16:38 ` Matteo Croce
2021-08-20 17:09 ` Marc Zyngier
2021-08-20 17:14 ` Matteo Croce
2021-08-20 17:24 ` Marc Zyngier
2021-08-20 17:35 ` Matteo Croce
2021-08-20 17:51 ` Marc Zyngier
2021-08-20 17:56 ` Matteo Croce
2021-08-20 18:05 ` Matteo Croce
2021-08-20 18:14 ` Marc Zyngier
2021-08-20 18:09 ` Marc Zyngier
2021-08-20 18:14 ` Matteo Croce
2021-08-20 18:41 ` Marc Zyngier [this message]
2021-08-16 15:12 ` Jakub Kicinski
2021-08-17 0:01 ` Matteo Croce
2021-08-19 15:26 ` Marc Zyngier
2021-08-11 10:41 ` Thierry Reding
2021-08-11 10:56 ` Joakim Zhang
2021-08-11 13:23 ` Marc Zyngier
2021-08-12 14:29 ` Thierry Reding
2021-08-12 15:26 ` Marc Zyngier
2021-08-13 14:44 ` Thierry Reding
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bl5syn5v.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=alexandre.torgue@foss.st.com \
--cc=davem@davemloft.net \
--cc=drew@beagleboard.org \
--cc=eric.dumazet@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=kernel@esmil.dk \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=mcroce@linux.microsoft.com \
--cc=netdev@vger.kernel.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=peppe.cavallaro@st.com \
--cc=thierry.reding@gmail.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).