Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[David Laight <david.laight.linux@gmail.com>]

On Thu, 22 Oct 2026 15:35:05 +0300
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Thu, Oct 22, 2026 at 11:22:18AM +0100, David Laight wrote:
> > On Thu, 22 Oct 2026 12:41:49 +0300
> > Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >   
> > > On Thu, Oct 22, 2026 at 09:33:59AM +0100, David Laight wrote:  
> > > > On Wed, 21 Oct 2026 17:53:12 +0300
> > > > vladimir.oltean@nxp.com wrote:
> > > >     
> > > > > The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers.
> > > > > This works, however sending short frames from user space and looking at
> > > > > the padding bytes inserted by the MAC at the link partner, we notice on
> > > > > LS1028A that the first octet of padding is sometimes not 0.
> > > > > 
> > > > > This is fine per 802.3, which does not require that the padding is zero.
> > > > > 
> > > > > However, there is an unproven suspicion that the padding bytes may
> > > > > come from internal memories, and unfortunately I couldn't get sufficient
> > > > > priority for this octet's origin to be investigated by hardware
> > > > > engineers.
> > > > > 
> > > > > To be defensive and not leak any sensitive information from the SoC,
> > > > > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't
> > > > > have to do anything.
> > > > > 
> > > > > There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO
> > > > > things are trivial; we need to call eth_skb_pad(). For GSO, things are
> > > > > not simple, because user space may set gso_size to a very small value,
> > > > > which could make the GSO super-skb smaller than ETH_ZLEN;    
> > > > 
> > > > That isn't sensible.
> > > > The minimum frame is 64 bytes (including FCS), subtract that, the MAC
> > > > header (14 bytes), minimum IP header (20), minimum TCP (20) and that
> > > > leaves 6 bytes for user data.
> > > > I doubt it is sensible to do GSO for anything under the minimum mtu
> > > > (128 bytes rings a bell somewhere).    
> > > 
> > > My fault for not being clear and inviting this kind of objection.
> > > 
> > > Since we do GSO in software, we advertise NETIF_F_GSO_UDP_L4 too, and
> > > there, Sashiko came on v3 with the observation that for UDP GSO, user
> > > space can set gso_size to a small value using setsockopt(UDP_SEGMENT):
> > > https://lore.kernel.org/netdev/178960175550.22033.10648880872141935672@kernel.org/  
> > 
> > I'd fix that in the setsockopt code.
> > But that might be a bug anyway.  
> 
> Why a bug?
> 
> > UDP segmentation has to use IP fragmentation and that is usually done because
> > the interface/path mtu is too short.  
> 
> In my testing, UDP GSO does not generate IP fragments, but rather, each
> UDP segment is its own datagram.

Hmmm....
I wouldn't have thought that was generally useful.
Losing data from random points in a connection is going to be hard
to recover from.
I guess that it why it is per-socket rather than system wide.
I mean, you can run telnet over udp and just hope no packets get lost.

> Are you thinking of UDP Fragmentation Offload instead? As per
> https://docs.kernel.org/networking/segmentation-offloads.html#udp-fragmentation-offload
> this is deprecated and AFAIU not what this is about.

That is the sort of UDP segmentation I thought made sense.
OTOH I think the main user of it was NFS, but even then 8k datagrams
were actually so unreliable it was often much better to tell UDP to fragment
the 8k blocks into 1k chunks (IIRC that was a mount option).

> 
> The docs do also explicitly say that UDP Segmentation Offload does not
> generate IP fragments.
> 
> > But there is a minimum mtu for IPv4 of (something like) 128 bytes, and
> > it may be invalid to fragment into smaller pieces.  
> 
> I have no idea what you are referring to, here, with the minimum MTU of
> 128 bytes.

The correct number is probably 68 (min max size of an IPv4 fragment) rather
than 576 (for a reassembled message) - I think both are from the start of
the IP headers.
That means there is no sane reason to fragment anything so small that
ethernet frames get padded.

> 
> > > Sensible or not, it is possible, and I did generate a test program using
> > > an LLM to confirm this.
> > > 
> > > Regardless of that, the last segment of a GSO super-skb would need
> > > padding anyway, because even with a 'normal' TCP MSS, the skb->len
> > > modulo gso_size could still result in a last frame < ETH_ZLEN. Padding
> > > just the GSO super-skb, as I did in v3, would also not fix this.  
> > 
> > True, but that is a different check to the one you did.  
> 
> So what is your suggestion for the driver to do?

Clearly you need to allow a BD for padding if the final fragment is short.
That is one extra BD for the entire frame, that same as a non-GSO message.
You could leave the hardware padding 'stupidly short' gso fragments.
I assume the driver is building the ethernet/IP/UDP/TDP header so that
will be one buffer and not hit the 'fragmented after 14 bytes' issue.

> 
> > > > But does this code allow for the VLAN tag that you forgot about in the
> > > > other patch?    
> > > 
> > > Not sure I understand your concern here? Is it that with VLAN TX
> > > insertion offload, we pad "too much" (i.e. to ETH_ZLEN, then the NIC
> > > pushes an extra VLAN_HLEN)? I'm not sure that's a problem..  
> > 
> > That is a problem, VLAN packets should only be padded to 64 bytes
> > including the VLAN header and CRC.
> > However such overlong packets are likely to be tolerated because
> > an ethernet switch can add a VLAN header before forwarding a packet
> > and it can't know whether a short packet is padded.  
> 
> Well, if it's a theoretical problem but it's not a practical problem,
> I'd prefer leaving it as is.
> 
> Since eth_skb_pad() itself is unaware of VLAN TX offloads, I'd be
> opening myself up to an endless rabbit hole ("why didn't you fix this
> in eth_skb_pad()? It's a generic problem." followed by
> "changing eth_skb_pad() broke 20 drivers!")...
> 
> Unless it's very clear why I'd be doing this, I'd very much prefer not to.

VLAN headers came in after I stopped writing ethernet drivers!
I think systems have to tolerate extra padding when a VLAN header is present.

David