On Fri, 20 Aug 2021 19:14:22 +0100,
Matteo Croce [off-list ref] wrote:

On Fri, Aug 20, 2021 at 8:09 PM Marc Zyngier [off-list ref] wrote:

quoted

On Fri, 20 Aug 2021 18:56:33 +0100,
Matteo Croce [off-list ref] wrote:

quoted

On Fri, Aug 20, 2021 at 7:51 PM Marc Zyngier [off-list ref] wrote:

quoted

On Fri, 20 Aug 2021 18:35:45 +0100,
Matteo Croce [off-list ref] wrote:

quoted

quoted

quoted

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 (local)

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 (local)

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