mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v5 net 0/4] Fix short frame transmission in enetc
@ 2026-10-21 14:53 vladimir.oltean
  2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx,
	linux-kernel

This is a belated follow-up to Zefir Kurtisi's report:
https://lore.kernel.org/netdev/20260220132930.2521155-1-zefir.kurtisi@gmail.com/

which is that the ENETC misbehaves when transmitting packets smaller
than 16 octets sans FCS: it sends them but does not update the completed
index in the transmit BD ring. I did find a sentence in the reference
manual explicitly stating these short frames are not supported.

Separately, the series stops relying on the mEMAC to pad short frames
to ETH_ZLEN, because the padding it inserts is not always zero, and we
want to avoid any risk of leaking SoC-internal memory onto the wire.

- Patch 1 makes the software TSO path reserve enough TX BDs for VLAN
  extension BDs. Patch 2 adds one more BD per segment to that same
  reservation, so it builds on top of this fix.
- Patch 2 pads short frames to ETH_ZLEN in software, including short
  software TSO segments, using a shared read-only zero buffer.
- Patch 3 makes sure the head buffer of an skb is at least 16 octets
  long, which fixes the reported stall.
- Patch 4 covers the XDP_TX and ndo_xdp_xmit() paths: it pads short
  frames to ETH_ZLEN in the same way, and drops frames whose head
  buffer is shorter than 16 octets.

Review of previous versions found a number of pre-existing issues in
the code surrounding these changes, and v4 grew to 8 patches to address
them. Review of v4 found even more. To be able to make progress, v5 is
reduced to the patches strictly needed for the stated purpose. The
other fixes will be submitted separately.

v1 at:
https://lore.kernel.org/netdev/20260401172246.1075883-1-vladimir.oltean@nxp.com/

v2 at:
https://lore.kernel.org/netdev/20260406204122.167237-1-vladimir.oltean@nxp.com/

v3 at:
https://lore.kernel.org/netdev/20260915222735.1016937-5-vladimir.oltean@nxp.com/

v4 at:
https://lore.kernel.org/netdev/20261015105536.1692348-7-vladimir.oltean@nxp.com/

Change log in individual patches. At a high level:
- dropped the following v4 patches, which are unrelated to short frames
  and will be resubmitted separately:
  - 1/8: consistently track dropped frames in enetc_xdp_xmit()
  - 2/8: serialize access to the same TX ring from NAPI XDP_TX and
    ndo_xdp_xmit()
  - 3/8: fix bogus TX ring consumer index after reinitialization
  - 7/8: fix leak of pending XDP_TX frames when going down
  - 8/8: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()
- new patch 1/4: reserve VLAN extension BDs for software TSO
- patch 2/4: account for the pad BD in enetc_tso_count_descs() and in
  the per-frame chained BD limit; explain why hardware LSO is not
  affected
- patches 3/4 and 4/4 are unchanged

Vladimir Oltean (4):
  net: enetc: reserve VLAN extension BDs for software TSO
  net: enetc: do not rely on mEMAC hardware frame padding
  net: enetc: ensure head buffer has at least 16 octets
  net: enetc: pad short XDP frames in software

 drivers/net/ethernet/freescale/enetc/enetc.c | 114 +++++++++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h |   8 ++
 2 files changed, 113 insertions(+), 9 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO
  2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean
@ 2026-10-21 14:53 ` vladimir.oltean
  2026-10-25 15:21   ` netdev-bot+sashiko
  2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx,
	linux-kernel

enetc_start_xmit() uses tso_count_descs() to check whether the TX ring
has enough free BDs for a software TSO frame. That estimates one header
BD and one data BD per segment, plus one BD per frag. However, when the
skb carries a VLAN tag, enetc_map_tx_tso_hdr() also adds an extension
BD to every segment, which is not accounted for.

With a highly segmented VLAN TSO skb and a nearly full ring, the driver
can then write past next_to_clean and overwrite BDs still owned by the
hardware.

Add enetc_tso_count_descs() which accounts for the extension BD of each
segment when VLAN insertion is needed.

Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v4->v5: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index c6571b5cc842..f6b352a3a49c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -685,6 +685,17 @@ static int enetc_lso_count_descs(const struct sk_buff *skb)
 	return skb_shinfo(skb)->nr_frags + 4;
 }
 
+static int enetc_tso_count_descs(const struct sk_buff *skb)
+{
+	int count = tso_count_descs(skb);
+
+	/* Each segment also needs an extension BD for VLAN insertion */
+	if (skb_vlan_tag_present(skb))
+		count += skb_shinfo(skb)->gso_segs;
+
+	return count;
+}
+
 static int enetc_lso_get_hdr_len(const struct sk_buff *skb)
 {
 	int hdr_len, tlen;
@@ -1029,7 +1040,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
 
 			count = enetc_lso_hw_offload(tx_ring, skb);
 		} else {
-			if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
+			if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) {
 				netif_stop_subqueue(ndev, tx_ring->index);
 				return NETDEV_TX_BUSY;
 			}
-- 
2.43.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean
  2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean
@ 2026-10-21 14:53 ` vladimir.oltean
  2026-10-22  8:33   ` David Laight
  2026-10-25 15:21   ` netdev-bot+sashiko
  2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean
  2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean
  3 siblings, 2 replies; 15+ messages in thread
From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx,
	linux-kernel

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; padding that
would produce more segments than originally intended. What needs to be
done is to pad individual segments instead.

When a GSO segment's hdr_len + data_len is below ETH_ZLEN, append a
zero-padding data BD and advertise the padded length in the header BD's
frm_len.

The padding bytes are always the same zeros, so a single read-only
DMA-coherent buffer (tx_pad, ETH_ZLEN bytes, zero-initialized by
dma_alloc_coherent() and never written) is shared by all padding BDs of
the ring. Like the header BD, a padding BD carries no per-BD DMA address,
so cleanup does not unmap it. The pad BD becomes the segment's last BD,
so the F flag and the skb pointer move onto it. Segment checksums are
unaffected as they are computed over data_len only, before padding.

The worst-case descriptor reservation grows by one BD per segment.
The pad BD also counts towards the per-frame chained BD limit (15 on
LS1028A, including the head and extension BD), so a padded segment may
use one data BD less.

Only LS1028A (NETCv1) needs this. On i.MX95 (NETC v4.1), it was observed
that padding bytes are always zero, and the reference manual also
documents in "106.3.5.1.2.1.3.7 FCS generation and append on transmit"
that HTA Transmit pads short frames (including LSO segment frames) with
zeros before appending the FCS. On LS1028A it does not say this (padding
seems an exclusively mEMAC task).

Nonetheless, for simplicity's sake, we keep the unconditional eth_skb_pad()
call in the common transmit path, but we leave the hardware LSO transmit
path unpadded in software. The latter is only present on NETC v4.1 and
later (thus already contains zeroes).

Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v4->v5:
- account for the pad BD in the new enetc_tso_count_descs() helper
  from "net: enetc: reserve VLAN extension BDs for software TSO",
  instead of open-coding it in enetc_start_xmit()
- count the pad BD towards the per-frame chained BD limit, so a padded
  segment may use one data BD less
- explain in the commit message why hardware LSO is not affected
v3->v4:
- split from "net: enetc: pad short frames in software", bring its own
  justification:
  https://lore.kernel.org/netdev/20260921112946.edo5uw6cl7jx6oez@skbuf/
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 61 ++++++++++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h |  6 ++
 2 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index f6b352a3a49c..a9ea124f7835 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -524,7 +524,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 				struct enetc_tx_swbd *tx_swbd,
 				union enetc_tx_bd *txbd, int *i, int hdr_len,
-				int data_len)
+				int data_len, int frm_len)
 {
 	union enetc_tx_bd txbd_tmp;
 	u8 flags = 0, e_flags = 0;
@@ -541,7 +541,7 @@ static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 	txbd_tmp.buf_len = cpu_to_le16(hdr_len);
 
 	/* first BD needs frm_len and offload flags set */
-	txbd_tmp.frm_len = cpu_to_le16(hdr_len + data_len);
+	txbd_tmp.frm_len = cpu_to_le16(frm_len);
 	txbd_tmp.flags = flags;
 
 	/* For the TSO header we do not set the dma address since we do not
@@ -613,6 +613,22 @@ static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 	return 0;
 }
 
+static void enetc_append_tx_pad(struct enetc_bdr *tx_ring, int i, int pad_len)
+{
+	struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
+	union enetc_tx_bd *txbd = ENETC_TXBD(*tx_ring, i);
+
+	enetc_clear_tx_bd(txbd);
+	txbd->addr = cpu_to_le64(tx_ring->tx_pad_dma);
+	txbd->buf_len = cpu_to_le16(pad_len);
+	txbd->flags = ENETC_TXBD_FLAGS_F;
+
+	tx_swbd->len = pad_len;
+	tx_swbd->is_eof = true;
+	tx_swbd->do_twostep_tstamp = false;
+	tx_swbd->check_wb = false;
+}
+
 static __wsum enetc_tso_hdr_csum(struct tso_t *tso, struct sk_buff *skb,
 				 char *hdr, int hdr_len, int *l4_hdr_len)
 {
@@ -687,7 +703,8 @@ static int enetc_lso_count_descs(const struct sk_buff *skb)
 
 static int enetc_tso_count_descs(const struct sk_buff *skb)
 {
-	int count = tso_count_descs(skb);
+	/* +gso_segs to cover one pad BD per segment */
+	int count = tso_count_descs(skb) + skb_shinfo(skb)->gso_segs;
 
 	/* Each segment also needs an extension BD for VLAN insertion */
 	if (skb_vlan_tag_present(skb))
@@ -926,6 +943,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 	i = tx_ring->next_to_use;
 
 	while (total_len > 0) {
+		int max_data_bds;
+		int frm_len;
+		int pad_len;
 		char *hdr;
 
 		/* Get the BD */
@@ -937,6 +957,12 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 		data_len = min_t(int, skb_shinfo(skb)->gso_size, total_len);
 		total_len -= data_len;
 
+		/* Pad segments below ETH_ZLEN so the mEMAC does not pad them. */
+		frm_len = max_t(int, hdr_len + data_len, ETH_ZLEN);
+		pad_len = frm_len - (hdr_len + data_len);
+		/* The pad BD counts towards the max chained BD limit */
+		max_data_bds = priv->max_frags - !!pad_len;
+
 		/* prepare packet headers: MAC + IP + TCP */
 		hdr = tx_ring->tso_headers + i * TSO_HEADER_SIZE;
 		tso_build_hdr(skb, hdr, &tso, data_len, total_len == 0);
@@ -944,7 +970,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 		/* compute the csum over the L4 header */
 		csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos);
 		count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd,
-					      &i, hdr_len, data_len);
+					      &i, hdr_len, data_len, frm_len);
 		bd_data_num = 0;
 
 		while (data_len > 0) {
@@ -966,8 +992,10 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 			csum = csum_block_add(csum, csum2, pos);
 			pos += size;
 
+			/* When padding, the pad BD is the last one, not this */
 			err = enetc_map_tx_tso_data(tx_ring, skb, tx_swbd, txbd,
 						    tso.data, size,
+						    !pad_len &&
 						    size == data_len);
 			if (err) {
 				if (i == 0)
@@ -982,12 +1010,19 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 			bd_data_num++;
 			tso_build_data(skb, &tso, size);
 
-			if (unlikely(bd_data_num >= priv->max_frags && data_len))
+			if (unlikely(bd_data_num >= max_data_bds && data_len))
 				goto err_chained_bd;
 		}
 
 		enetc_tso_complete_csum(tx_ring, &tso, skb, hdr, pos, csum);
 
+		if (pad_len) {
+			enetc_bdr_idx_inc(tx_ring, &i);
+			enetc_append_tx_pad(tx_ring, i, pad_len);
+			tx_swbd = &tx_ring->tx_swbd[i];
+			count++;
+		}
+
 		if (total_len == 0)
 			tx_swbd->skb = skb;
 
@@ -1050,6 +1085,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
 			enetc_unlock_mdio();
 		}
 	} else {
+		if (eth_skb_pad(skb))
+			return NETDEV_TX_OK;
+
 		if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags))
 			if (unlikely(skb_linearize(skb)))
 				goto drop_packet_err;
@@ -2285,8 +2323,18 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res,
 		goto err_alloc_tso;
 	}
 
+	res->tx_pad = dma_alloc_coherent(dev, ETH_ZLEN, &res->tx_pad_dma,
+					 GFP_KERNEL);
+	if (!res->tx_pad) {
+		err = -ENOMEM;
+		goto err_alloc_pad;
+	}
+
 	return 0;
 
+err_alloc_pad:
+	dma_free_coherent(dev, bd_count * TSO_HEADER_SIZE, res->tso_headers,
+			  res->tso_headers_dma);
 err_alloc_tso:
 	enetc_dma_free_bdr(res);
 err_alloc_bdr:
@@ -2298,6 +2346,7 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res,
 
 static void enetc_free_tx_resource(const struct enetc_bdr_resource *res)
 {
+	dma_free_coherent(res->dev, ETH_ZLEN, res->tx_pad, res->tx_pad_dma);
 	dma_free_coherent(res->dev, res->bd_count * TSO_HEADER_SIZE,
 			  res->tso_headers, res->tso_headers_dma);
 	enetc_dma_free_bdr(res);
@@ -2425,6 +2474,8 @@ static void enetc_assign_tx_resource(struct enetc_bdr *tx_ring,
 	tx_ring->tx_swbd = res ? res->tx_swbd : NULL;
 	tx_ring->tso_headers = res ? res->tso_headers : NULL;
 	tx_ring->tso_headers_dma = res ? res->tso_headers_dma : 0;
+	tx_ring->tx_pad = res ? res->tx_pad : NULL;
+	tx_ring->tx_pad_dma = res ? res->tx_pad_dma : 0;
 }
 
 static void enetc_assign_rx_resource(struct enetc_bdr *rx_ring,
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index d9e91832a9c1..db3b3bad0f59 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -145,6 +145,8 @@ struct enetc_bdr_resource {
 	};
 	char *tso_headers;
 	dma_addr_t tso_headers_dma;
+	void *tx_pad;
+	dma_addr_t tx_pad_dma;
 };
 
 struct enetc_bdr {
@@ -182,6 +184,10 @@ struct enetc_bdr {
 	/* DMA buffer for TSO headers */
 	char *tso_headers;
 	dma_addr_t tso_headers_dma;
+
+	/* Shared read-only zero buffer to pad short frames to ETH_ZLEN */
+	void *tx_pad;
+	dma_addr_t tx_pad_dma;
 } ____cacheline_aligned_in_smp;
 
 static inline void enetc_bdr_idx_inc(struct enetc_bdr *bdr, int *i)
-- 
2.43.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets
  2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean
  2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean
  2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean
@ 2026-10-21 14:53 ` vladimir.oltean
  2026-10-25 15:21   ` netdev-bot+sashiko
  2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean
  3 siblings, 1 reply; 15+ messages in thread
From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx,
	linux-kernel

The ENETC does not support BUF_LEN or FRM_LEN in TX buffer descriptors
less than 16. This is written in the reference manual of all SoCs
supported by the driver: LS1028A, i.MX943, i.MX95 etc.

  Frames must not have a FRM_LEN that is less than 16 bytes. Frames of
  0-15 bytes are not supported.
  (...)
  The first descriptor in a chain must not have a BUFF_LEN that is less
  than 16 bytes.

I don't think proper attention was paid to this during development, we
found the text at the end of a bug investigation. Therefore, the driver
does not enforce this.

But the frame length and geometry is out of the driver's control, and
the network stack can actually send packets that ENETC doesn't like.

Zefir Kurtisi found a case where transmitting L2 WNM keep-alive frames
through ENETC would soft-lockup the host through an IRQ storm. He later
distilled this into a small enetc-killer.c user space program which
sends a packet with MAC DA, MAC SA and EtherType IPv4 (14 octets in
length) through an AF_PACKET raw socket. Note that this particular case
was already closed by the previous commit, but an identical situation
still happens when the first buffer of a multi-buffer frame is < 16
bytes.

The IRQ storm is actually a curious effect of a chain of events.

The hardware behaviour, when an invalid BD is put in its TX ring, is
that it would transmit the packet as normal, update counters, raise
completion interrupt as normal, but it would just not advance the
consumer index of the ring (TBaCIR) to signify that the BD has been
consumed and is available for software to free. The ring will also get
its TBaSR[BUSY] bit persistently set to 1 afterwards.

It deserves an explanation why the behaviour above would lead to an
IRQ storm, since ENETC interrupts are message-based (MSI-X), and an
unhandled interrupt would typically just be lost rather than retrigger
itself as a wired interrupt would.

NAPI processing in ENETC has 3 steps:

I. the enetc_msix() hardirq handler disables RBaIER, TBaIER and sets
   softirq processing to the 'pending' state.

II. the enetc_poll() softirq handler for the IRQ vector walks through
    the TX rings affine to that vector, checks which ones have a TBCIR
    updated since last time - enetc_bd_ready_count() - processes those
    completed frames, and clears pending interrupts in these updated TX
    rings by writing to TBaIDR. (I've excluded RX processing due to it
    being irrelevant).

III. After the softirq handler does its round of checking all RX and TX
     rings for updates, it re-enables all interrupts in RBaIER and
     TBaIER that were previously disabled by the hardirq handler, and
     exits.

Because the TX ring with the short frame is skipped at step II (TBCIR
wasn't updated as part of HW malfunction), its pending IRQ is not
cleared in TBaIDR by enetc_clean_tx_ring().

But because enetc_msix() disables TBaIER at step I and re-enables it at
step III, another MSI will be fired upon re-enabling it. This is what
completes the cycle and the driver goes back to step I.

So the driver misinterprets the mixed signals it's getting from the
hardware, and ends up causing a software-amplified IRQ storm.

Because the driver now pads linear skbs to ETH_ZLEN for other reasons
too, we just need to ensure the head buffer has at least 16 bytes.
This can be done using pskb_may_pull(), which moves that number of bytes
from the paged area into the head. This is case 5 below.

  skb_headlen (H) | skb->len (L) | linear? | handled by
  ----------------+--------------+---------+---------------------------------------
   H == L         | L < 60       |  yes    | eth_skb_pad() (pads head to 60)
   H == L         | L >= 60      |  yes    | already valid, no-op
   H < 16         | L < 60       |  no     | eth_skb_pad() (linearizes + pads to 60)
   H < 16         | L >= 60      |  no     | pskb_may_pull() (pulls frags into head)
   H >= 16        | L >= 60      |  no     | already valid, no-op

Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
Reported-by: Zefir Kurtisi <zefir.kurtisi@westermo.com>
Closes: https://lore.kernel.org/netdev/b3d9136c-2803-4203-b1ea-1f9e62de80a1@gmail.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v4->v5: none
v3->v4:
- rename from "net: enetc: pad short frames in software". The padding
  to ETH_ZLEN is now done by "net: enetc: do not rely on mEMAC hardware
  frame padding", so this patch only deals with the 16 octet minimum
  head buffer length
- move the check from enetc_xmit() to enetc_start_xmit(), after
  eth_skb_pad()
- use pskb_may_pull() instead of skb_linearize()
- drop the code comment above the head buffer length check, it is
  redundant with the commit message
- rework the commit message, add a table of the handled cases
- drop Reviewed-by from Wei Fang and Tested-by from Zefir Kurtisi due to
  the change of approach
v1->v3: part of "pad short frames in software"
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 6 ++++++
 drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index a9ea124f7835..58a88f15fa5d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1088,6 +1088,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
 		if (eth_skb_pad(skb))
 			return NETDEV_TX_OK;
 
+		if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
+			     !pskb_may_pull(skb, ENETC_MIN_BUFF_SIZE))) {
+			dev_kfree_skb_any(skb);
+			return NETDEV_TX_OK;
+		}
+
 		if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags))
 			if (unlikely(skb_linearize(skb)))
 				goto drop_packet_err;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index db3b3bad0f59..6fc7001c025c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -22,6 +22,8 @@
 #define ENETC_MAX_MTU		(ENETC_MAC_MAXFRM_SIZE - \
 				(ETH_FCS_LEN + ETH_HLEN + VLAN_HLEN))
 
+#define ENETC_MIN_BUFF_SIZE	16
+
 #define ENETC_CBD_DATA_MEM_ALIGN 64
 
 #define ENETC_MADDR_HASH_TBL_SZ	64
-- 
2.43.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software
  2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean
                   ` (2 preceding siblings ...)
  2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean
@ 2026-10-21 14:53 ` vladimir.oltean
  2026-10-25 15:21   ` netdev-bot+sashiko
  3 siblings, 1 reply; 15+ messages in thread
From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx,
	linux-kernel

Similar to the skb path, the XDP transmit path may put frames shorter
than ETH_ZLEN in the TX ring, and may put frames whose first buffer is
shorter than 16 bytes. The former relies on mEMAC hardware padding,
which we no longer want to rely upon, and the latter is not supported
by ENETC at all.

There are two callers of enetc_xdp_tx():

- ndo_xdp_xmit(), which can transmit short frames for example when
  redirecting from a veth interface pair through a devmap.

- XDP_TX from enetc_clean_rx_ring_xdp(). With RX VLAN extraction
  enabled (the default), a minimum-sized VLAN-tagged frame is 56 bytes
  long when it is sent back, and once the driver honours
  bpf_xdp_adjust_tail() and bpf_xdp_adjust_head() for XDP_TX (to be
  addressed separately), the BPF program can make the frame arbitrarily
  short.

Handle both in enetc_xdp_tx(). When the frame is shorter than ETH_ZLEN,
append a BD pointing to the shared read-only tx_pad zero buffer, the
same as for short TSO segments, and advertise the padded frame length
in the BDs. The pad BD becomes the frame's last BD, so the F flag, the
is_eof marker and the xdp_frame pointer (for XDP_REDIRECT) move onto
it. The pad BD has no DMA mapping of its own, and it is not an XDP_TX
buffer, so TX completion neither unmaps nor recycles it. It is also not
accounted in xdp_tx_in_flight, which counts RX buffers.

The 16 byte limitation for the first buffer cannot be addressed by the
pad BD, so frames with a head buffer shorter than that are dropped.
Both callers already treat a false return code from enetc_xdp_tx() as
a drop.

Doing this in the driver rather than padding the xdp_frame in place
also covers multi-buffer frames shorter than ETH_ZLEN, and avoids
writing to the tailroom of buffers that, in the XDP_TX case, are
recycled into the RX ring.

Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX")
Fixes: 9d2b68cc108d ("net: enetc: add support for XDP_REDIRECT")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v4->v5: none
v3->v4:
- patch is new. It replaces "net: enetc: pad short XDP frames coming
  from devmap", which padded the xdp_frame in place using a new generic
  xdp_frame_pad() helper, and only covered ndo_xdp_xmit(). The XDP_TX
  path is now covered too, using the tx_pad buffer instead.
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 36 +++++++++++++++++---
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 58a88f15fa5d..275441c2ffa3 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1759,8 +1759,11 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
 {
 	struct enetc_tx_swbd *tmp_tx_swbd = xdp_tx_arr;
 	int i, k, frm_len = tmp_tx_swbd->len;
+	struct enetc_tx_swbd *last_swbd;
+	int pad_len;
 
-	if (unlikely(enetc_bd_unused(tx_ring) < ENETC_TXBDS_NEEDED(num_tx_swbd)))
+	/* Hardware does not support a first buffer size of less than 16 bytes */
+	if (unlikely(xdp_tx_arr[0].len < ENETC_MIN_BUFF_SIZE))
 		return false;
 
 	while (unlikely(!tmp_tx_swbd->is_eof)) {
@@ -1768,15 +1771,24 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
 		frm_len += tmp_tx_swbd->len;
 	}
 
+	/* Pad frames below ETH_ZLEN so the mEMAC does not pad them. */
+	pad_len = max_t(int, ETH_ZLEN - frm_len, 0);
+
+	if (unlikely(enetc_bd_unused(tx_ring) <
+		     ENETC_TXBDS_NEEDED(num_tx_swbd + !!pad_len)))
+		return false;
+
 	i = tx_ring->next_to_use;
 
 	for (k = 0; k < num_tx_swbd; k++) {
 		struct enetc_tx_swbd *xdp_tx_swbd = &xdp_tx_arr[k];
 
-		enetc_xdp_map_tx_buff(tx_ring, i, xdp_tx_swbd, frm_len);
+		enetc_xdp_map_tx_buff(tx_ring, i, xdp_tx_swbd,
+				      frm_len + pad_len);
+		last_swbd = &tx_ring->tx_swbd[i];
 
-		/* last BD needs 'F' bit set */
-		if (xdp_tx_swbd->is_eof) {
+		/* last BD needs 'F' bit set, unless a pad BD follows */
+		if (xdp_tx_swbd->is_eof && !pad_len) {
 			union enetc_tx_bd *txbd = ENETC_TXBD(*tx_ring, i);
 
 			txbd->flags = ENETC_TXBD_FLAGS_F;
@@ -1785,6 +1797,22 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
 		enetc_bdr_idx_inc(tx_ring, &i);
 	}
 
+	/* The pad BD becomes the last one, so it takes over the xdp_frame
+	 * (if any) from the last data BD. The pad BD has no DMA mapping of
+	 * its own, and it is not an XDP_TX buffer to be recycled.
+	 */
+	if (unlikely(pad_len)) {
+		struct enetc_tx_swbd *pad_swbd = &tx_ring->tx_swbd[i];
+
+		enetc_append_tx_pad(tx_ring, i, pad_len);
+		pad_swbd->is_xdp_redirect = last_swbd->is_xdp_redirect;
+		pad_swbd->xdp_frame = last_swbd->xdp_frame;
+		last_swbd->xdp_frame = NULL;
+		last_swbd->is_eof = false;
+
+		enetc_bdr_idx_inc(tx_ring, &i);
+	}
+
 	tx_ring->next_to_use = i;
 
 	return true;
-- 
2.43.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean
@ 2026-10-22  8:33   ` David Laight
  2026-10-22  9:41     ` Vladimir Oltean
  2026-10-25 15:21   ` netdev-bot+sashiko
  1 sibling, 1 reply; 15+ messages in thread
From: David Laight @ 2026-10-22  8:33 UTC (permalink / raw)
  To: vladimir.oltean
  Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel

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).

But does this code allow for the VLAN tag that you forgot about in the
other patch?

I also noticed that the code seems to dma map the fragments for each
gso segment separately, just mapping each skb fragment would be better.
(But that is for another patch series.) 

David

> padding that
> would produce more segments than originally intended. What needs to be
> done is to pad individual segments instead.
> 
> When a GSO segment's hdr_len + data_len is below ETH_ZLEN, append a
> zero-padding data BD and advertise the padded length in the header BD's
> frm_len.
> 
> The padding bytes are always the same zeros, so a single read-only
> DMA-coherent buffer (tx_pad, ETH_ZLEN bytes, zero-initialized by
> dma_alloc_coherent() and never written) is shared by all padding BDs of
> the ring. Like the header BD, a padding BD carries no per-BD DMA address,
> so cleanup does not unmap it. The pad BD becomes the segment's last BD,
> so the F flag and the skb pointer move onto it. Segment checksums are
> unaffected as they are computed over data_len only, before padding.
> 
> The worst-case descriptor reservation grows by one BD per segment.
> The pad BD also counts towards the per-frame chained BD limit (15 on
> LS1028A, including the head and extension BD), so a padded segment may
> use one data BD less.
> 
> Only LS1028A (NETCv1) needs this. On i.MX95 (NETC v4.1), it was observed
> that padding bytes are always zero, and the reference manual also
> documents in "106.3.5.1.2.1.3.7 FCS generation and append on transmit"
> that HTA Transmit pads short frames (including LSO segment frames) with
> zeros before appending the FCS. On LS1028A it does not say this (padding
> seems an exclusively mEMAC task).
> 
> Nonetheless, for simplicity's sake, we keep the unconditional eth_skb_pad()
> call in the common transmit path, but we leave the hardware LSO transmit
> path unpadded in software. The latter is only present on NETC v4.1 and
> later (thus already contains zeroes).
> 
> Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO")
> Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v4->v5:
> - account for the pad BD in the new enetc_tso_count_descs() helper
>   from "net: enetc: reserve VLAN extension BDs for software TSO",
>   instead of open-coding it in enetc_start_xmit()
> - count the pad BD towards the per-frame chained BD limit, so a padded
>   segment may use one data BD less
> - explain in the commit message why hardware LSO is not affected
> v3->v4:
> - split from "net: enetc: pad short frames in software", bring its own
>   justification:
>   https://lore.kernel.org/netdev/20260921112946.edo5uw6cl7jx6oez@skbuf/
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 61 ++++++++++++++++++--
>  drivers/net/ethernet/freescale/enetc/enetc.h |  6 ++
>  2 files changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index f6b352a3a49c..a9ea124f7835 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -524,7 +524,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
>  static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb,
>  				struct enetc_tx_swbd *tx_swbd,
>  				union enetc_tx_bd *txbd, int *i, int hdr_len,
> -				int data_len)
> +				int data_len, int frm_len)
>  {
>  	union enetc_tx_bd txbd_tmp;
>  	u8 flags = 0, e_flags = 0;
> @@ -541,7 +541,7 @@ static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb,
>  	txbd_tmp.buf_len = cpu_to_le16(hdr_len);
>  
>  	/* first BD needs frm_len and offload flags set */
> -	txbd_tmp.frm_len = cpu_to_le16(hdr_len + data_len);
> +	txbd_tmp.frm_len = cpu_to_le16(frm_len);
>  	txbd_tmp.flags = flags;
>  
>  	/* For the TSO header we do not set the dma address since we do not
> @@ -613,6 +613,22 @@ static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static void enetc_append_tx_pad(struct enetc_bdr *tx_ring, int i, int pad_len)
> +{
> +	struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
> +	union enetc_tx_bd *txbd = ENETC_TXBD(*tx_ring, i);
> +
> +	enetc_clear_tx_bd(txbd);
> +	txbd->addr = cpu_to_le64(tx_ring->tx_pad_dma);
> +	txbd->buf_len = cpu_to_le16(pad_len);
> +	txbd->flags = ENETC_TXBD_FLAGS_F;
> +
> +	tx_swbd->len = pad_len;
> +	tx_swbd->is_eof = true;
> +	tx_swbd->do_twostep_tstamp = false;
> +	tx_swbd->check_wb = false;
> +}
> +
>  static __wsum enetc_tso_hdr_csum(struct tso_t *tso, struct sk_buff *skb,
>  				 char *hdr, int hdr_len, int *l4_hdr_len)
>  {
> @@ -687,7 +703,8 @@ static int enetc_lso_count_descs(const struct sk_buff *skb)
>  
>  static int enetc_tso_count_descs(const struct sk_buff *skb)
>  {
> -	int count = tso_count_descs(skb);
> +	/* +gso_segs to cover one pad BD per segment */
> +	int count = tso_count_descs(skb) + skb_shinfo(skb)->gso_segs;
>  
>  	/* Each segment also needs an extension BD for VLAN insertion */
>  	if (skb_vlan_tag_present(skb))
> @@ -926,6 +943,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  	i = tx_ring->next_to_use;
>  
>  	while (total_len > 0) {
> +		int max_data_bds;
> +		int frm_len;
> +		int pad_len;
>  		char *hdr;
>  
>  		/* Get the BD */
> @@ -937,6 +957,12 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  		data_len = min_t(int, skb_shinfo(skb)->gso_size, total_len);
>  		total_len -= data_len;
>  
> +		/* Pad segments below ETH_ZLEN so the mEMAC does not pad them. */
> +		frm_len = max_t(int, hdr_len + data_len, ETH_ZLEN);
> +		pad_len = frm_len - (hdr_len + data_len);
> +		/* The pad BD counts towards the max chained BD limit */
> +		max_data_bds = priv->max_frags - !!pad_len;
> +
>  		/* prepare packet headers: MAC + IP + TCP */
>  		hdr = tx_ring->tso_headers + i * TSO_HEADER_SIZE;
>  		tso_build_hdr(skb, hdr, &tso, data_len, total_len == 0);
> @@ -944,7 +970,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  		/* compute the csum over the L4 header */
>  		csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos);
>  		count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd,
> -					      &i, hdr_len, data_len);
> +					      &i, hdr_len, data_len, frm_len);
>  		bd_data_num = 0;
>  
>  		while (data_len > 0) {
> @@ -966,8 +992,10 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  			csum = csum_block_add(csum, csum2, pos);
>  			pos += size;
>  
> +			/* When padding, the pad BD is the last one, not this */
>  			err = enetc_map_tx_tso_data(tx_ring, skb, tx_swbd, txbd,
>  						    tso.data, size,
> +						    !pad_len &&
>  						    size == data_len);
>  			if (err) {
>  				if (i == 0)
> @@ -982,12 +1010,19 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  			bd_data_num++;
>  			tso_build_data(skb, &tso, size);
>  
> -			if (unlikely(bd_data_num >= priv->max_frags && data_len))
> +			if (unlikely(bd_data_num >= max_data_bds && data_len))
>  				goto err_chained_bd;
>  		}
>  
>  		enetc_tso_complete_csum(tx_ring, &tso, skb, hdr, pos, csum);
>  
> +		if (pad_len) {
> +			enetc_bdr_idx_inc(tx_ring, &i);
> +			enetc_append_tx_pad(tx_ring, i, pad_len);
> +			tx_swbd = &tx_ring->tx_swbd[i];
> +			count++;
> +		}
> +
>  		if (total_len == 0)
>  			tx_swbd->skb = skb;
>  
> @@ -1050,6 +1085,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>  			enetc_unlock_mdio();
>  		}
>  	} else {
> +		if (eth_skb_pad(skb))
> +			return NETDEV_TX_OK;
> +
>  		if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags))
>  			if (unlikely(skb_linearize(skb)))
>  				goto drop_packet_err;
> @@ -2285,8 +2323,18 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res,
>  		goto err_alloc_tso;
>  	}
>  
> +	res->tx_pad = dma_alloc_coherent(dev, ETH_ZLEN, &res->tx_pad_dma,
> +					 GFP_KERNEL);
> +	if (!res->tx_pad) {
> +		err = -ENOMEM;
> +		goto err_alloc_pad;
> +	}
> +
>  	return 0;
>  
> +err_alloc_pad:
> +	dma_free_coherent(dev, bd_count * TSO_HEADER_SIZE, res->tso_headers,
> +			  res->tso_headers_dma);
>  err_alloc_tso:
>  	enetc_dma_free_bdr(res);
>  err_alloc_bdr:
> @@ -2298,6 +2346,7 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res,
>  
>  static void enetc_free_tx_resource(const struct enetc_bdr_resource *res)
>  {
> +	dma_free_coherent(res->dev, ETH_ZLEN, res->tx_pad, res->tx_pad_dma);
>  	dma_free_coherent(res->dev, res->bd_count * TSO_HEADER_SIZE,
>  			  res->tso_headers, res->tso_headers_dma);
>  	enetc_dma_free_bdr(res);
> @@ -2425,6 +2474,8 @@ static void enetc_assign_tx_resource(struct enetc_bdr *tx_ring,
>  	tx_ring->tx_swbd = res ? res->tx_swbd : NULL;
>  	tx_ring->tso_headers = res ? res->tso_headers : NULL;
>  	tx_ring->tso_headers_dma = res ? res->tso_headers_dma : 0;
> +	tx_ring->tx_pad = res ? res->tx_pad : NULL;
> +	tx_ring->tx_pad_dma = res ? res->tx_pad_dma : 0;
>  }
>  
>  static void enetc_assign_rx_resource(struct enetc_bdr *rx_ring,
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index d9e91832a9c1..db3b3bad0f59 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> @@ -145,6 +145,8 @@ struct enetc_bdr_resource {
>  	};
>  	char *tso_headers;
>  	dma_addr_t tso_headers_dma;
> +	void *tx_pad;
> +	dma_addr_t tx_pad_dma;
>  };
>  
>  struct enetc_bdr {
> @@ -182,6 +184,10 @@ struct enetc_bdr {
>  	/* DMA buffer for TSO headers */
>  	char *tso_headers;
>  	dma_addr_t tso_headers_dma;
> +
> +	/* Shared read-only zero buffer to pad short frames to ETH_ZLEN */
> +	void *tx_pad;
> +	dma_addr_t tx_pad_dma;
>  } ____cacheline_aligned_in_smp;
>  
>  static inline void enetc_bdr_idx_inc(struct enetc_bdr *bdr, int *i)


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-22  8:33   ` David Laight
@ 2026-10-22  9:41     ` Vladimir Oltean
  2026-10-22 10:22       ` David Laight
  0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Oltean @ 2026-10-22  9:41 UTC (permalink / raw)
  To: David Laight
  Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel

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/

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.

> 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..

Anyway, with VLAN TX offload, the VLAN tag is not part of the frm_len.
It's part of the extension BD that we forgot to account for, when
determining whether the TX BD ring has enough space for the frame under
transmission.

> I also noticed that the code seems to dma map the fragments for each
> gso segment separately, just mapping each skb fragment would be better.
> (But that is for another patch series.) 

Noted, I can look into this separately.

Thanks for the review!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-22  9:41     ` Vladimir Oltean
@ 2026-10-22 10:22       ` David Laight
  2026-10-22 12:35         ` Vladimir Oltean
  0 siblings, 1 reply; 15+ messages in thread
From: David Laight @ 2026-10-22 10:22 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel

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.
UDP segmentation has to use IP fragmentation and that is usually done because
the interface/path mtu is too short.
But there is a minimum mtu for IPv4 of (something like) 128 bytes, and
it may be invalid to fragment into smaller pieces.

> 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.

> > 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.

> Anyway, with VLAN TX offload, the VLAN tag is not part of the frm_len.
> It's part of the extension BD that we forgot to account for, when
> determining whether the TX BD ring has enough space for the frame under
> transmission.

That is what I was thinking of.
By brain thought it was a normal BD - so would be in the 15 limit.

David

> > I also noticed that the code seems to dma map the fragments for each
> > gso segment separately, just mapping each skb fragment would be better.
> > (But that is for another patch series.)   
> 
> Noted, I can look into this separately.
> 
> Thanks for the review!


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-22 10:22       ` David Laight
@ 2026-10-22 12:35         ` Vladimir Oltean
  2026-10-22 20:48           ` David Laight
  0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Oltean @ 2026-10-22 12:35 UTC (permalink / raw)
  To: David Laight
  Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel

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.

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.

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.

> > 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?

> > > 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-22 12:35         ` Vladimir Oltean
@ 2026-10-22 20:48           ` David Laight
  2026-10-23  9:35             ` Vladimir Oltean
  0 siblings, 1 reply; 15+ messages in thread
From: David Laight @ 2026-10-22 20:48 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel

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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-22 20:48           ` David Laight
@ 2026-10-23  9:35             ` Vladimir Oltean
  0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Oltean @ 2026-10-23  9:35 UTC (permalink / raw)
  To: David Laight
  Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
	Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel

Hi David,

On Thu, Oct 22, 2026 at 09:48:03PM +0100, David Laight wrote:
> > > 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.

After a very quick search, I think the main use case for UDP GSO was QUIC:
https://lpc.events/event/2/contributions/106/

In turn, QUIC has its own loss detection and congestion control, though
I don't exactly know how they do it.

An interesting statement I found in that paper is:

"The gso size parameter communicates the length of the
payload without network or transport layer headers. A
gso_size all the way down to 1 is a valid choice. A value of 0
effectively disables GSO."

> > 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.

Leaving the hardware to pad 'stupidly short' GSO segments is definitely
not an option. Given my context: the hardware adds some non-zero padding
bytes, and customers suspect the MAC is leaking device-internal memories,
this is exactly the kind of attack vector user space could use to
exfiltrate data (even if not practically useful, this is a valid API).
The request has been explicit to avoid this if we can't prove the origin
of the non-zero padding bytes.

Now, having that said, I noticed that Joe Damato had this exact problem
a few weeks ago in bnxt:
https://lore.kernel.org/netdev/20261009215426.2377946-1-joe@dama.to/

It looks like he did something there in ndo_features_check() to simply
clear NETIF_F_GSO_MASK for the 'stupidly short' gso_size, so that
they're segmented in the stack and appear as non-GSO during driver xmit.
But this is just a superficial analysis. I'll have to see how much
complexity it adds to do something similar in enetc, vs just adding a
padding BD for each GSO segment/datagram myself.

> 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.

Yes, the header buffer < ENETC_MIN_BUFF_SIZE is not the problem here,
just general MAC padding contents. This is why I split the patches the
way I did: first never allow the MAC to pad by itself, then address the
remaining buffer geometry issues that ENETC doesn't tolerate (this
causes a separate, unrelated problem, that the software padding partly
fixes and partly doesn't).

> > > > > 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
> 
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO
  2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean
@ 2026-10-25 15:21   ` netdev-bot+sashiko
  0 siblings, 0 replies; 15+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw)
  To: vladimir.oltean
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil,
	wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi,
	zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix stops the ring overrun, but VLAN-tagged TSO skbs that
  can never fit now stall the TX queue for good.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the BUSY path
  the patch changes can lose a wakeup.
- [High] This problem wasn't introduced by this patch, but
  enetc_map_tx_tso_buffs() copies the whole L2+L3+L4 header into a fixed…

--

net: enetc: reserve VLAN extension BDs for software TSO

This commit adds enetc_tso_count_descs(). It extends tso_count_descs()
with one extension BD per segment when the skb carries a VLAN tag.
enetc_start_xmit() now uses it for the free BD check on the software TSO
path.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 80f0082f6c633..ab4bfefda6c8f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -688,6 +688,17 @@ static int enetc_lso_count_descs(const struct sk_buff *skb)
>  	return skb_shinfo(skb)->nr_frags + 4;
>  }
>  
> +static int enetc_tso_count_descs(const struct sk_buff *skb)
> +{
> +	int count = tso_count_descs(skb);
> +
> +	/* Each segment also needs an extension BD for VLAN insertion */
> +	if (skb_vlan_tag_present(skb))
> +		count += skb_shinfo(skb)->gso_segs;

[Severity: High]

Can this estimate exceed what the TX ring can ever provide? If so, the
queue would stop permanently instead of overrunning.

For a VLAN skb this returns 3 * gso_segs + nr_frags. The TX ring is
fixed at ENETC_TX_RING_DEFAULT_SIZE (2048) and there is no
set_ringparam. So enetc_bd_unused() returns at most 2047, even on an
empty ring.

The driver does not limit the segment count anywhere. There is no
netif_set_tso_max_segs(), no gso_max_segs and no ndo_features_check.

Take a 64KB TSO skb with an MSS of about 96 or less (gso_segs of roughly
683 or more). enetc_start_xmit() takes this path on every attempt:

	if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) {
		netif_stop_subqueue(ndev, tx_ring->index);
		return NETDEV_TX_BUSY;
	}

The qdisc requeues the same skb. The only runtime wake is in
enetc_clean_tx_ring():

	if (unlikely(tx_frm_cnt && netif_carrier_ok(ndev) &&
		     __netif_subqueue_stopped(ndev, tx_ring->index) &&
		     ...
		netif_wake_subqueue(ndev, tx_ring->index);

Once the ring drains, tx_frm_cnt stays 0. There is also no
ndo_tx_timeout, so nothing restarts the queue.

This can be triggered by a TCP sender on a VLAN on top of enetc using
the software TSO path. A remote peer can force a small MSS through its
SYN (down to tcp_min_snd_mss), and a local user can use TCP_MAXSEG.

Before this patch, most of these skbs overran the ring. Now they hang
the queue instead.

The same stall already existed for non-VLAN skbs above about 1016
segments. This change moves VLAN skbs with about 683 to 1015 segments
into it.

The estimate is a worst case, so some VLAN skbs that really fit are also
rejected forever. For example, gso_segs=680 with 17 segment-aligned
frags needs 2040 BDs, but the estimate is 2057.

The padding patch in this series ("net: enetc: do not rely on mEMAC
hardware frame padding") adds another gso_segs to the estimate. That
lowers the threshold to about 508 segments with VLAN and about 677
without.

Would it make sense to cap the segment count to what the ring can hold?
For example:

	netif_set_tso_max_segs(ndev, (bd_count - 1 - MAX_SKB_FRAGS) /
			       per_seg_bds);

Another option is an ndo_features_check that clears GSO when the count
exceeds bd_count - 1, so the core falls back to software GSO.

> +
> +	return count;
> +}
> +

[ ... ]

> @@ -1032,7 +1043,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>  
>  			count = enetc_lso_hw_offload(tx_ring, skb);
>  		} else {
> -			if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
> +			if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) {
>  				netif_stop_subqueue(ndev, tx_ring->index);
>  				return NETDEV_TX_BUSY;
>  			}

[Severity: High]

This is a pre-existing issue, not one introduced by this patch. Can
this stop path lose a wakeup?

enetc_bd_unused() reads tx_ring->next_to_clean without a lock. The
queue is then stopped with no smp_mb() and no re-check of free space.

On another CPU, enetc_clean_tx_ring() updates next_to_clean. It wakes
the queue only if it already sees the queue stopped and tx_frm_cnt is
non-zero:

CPU0 (enetc_start_xmit)              CPU1 (enetc_clean_tx_ring)
enetc_bd_unused() reads stale
next_to_clean, not enough room
                                     cleans the last in-flight frames
                                     tx_ring->next_to_clean = i;
                                     __netif_subqueue_stopped() is false,
                                     no wake
netif_stop_subqueue()
return NETDEV_TX_BUSY

At that point the ring has plenty of room, but no BDs are in flight. No
completion will call netif_wake_subqueue().

The only other wake is netif_tx_start_all_queues() on open, and there
is no ndo_tx_timeout.

The same pattern is present at the LSO, non-GSO and post-xmit stop
sites. This patch raises the BD requirement for VLAN TSO skbs, so this
path now runs more often, including when only a few short frames are in
flight.

Would netif_txq_maybe_stop() / netif_txq_completed_wake() fit here?
Alternatively, an smp_mb() plus a re-check after stopping could be
paired with an smp_mb() in the cleaner before it tests the stopped bit.

[Severity: High]

This isn't a bug introduced by this patch, but it is in the same
software TSO path. Can enetc_map_tx_tso_buffs() write past its
tx_ring->tso_headers slot when the headers are longer than
TSO_HEADER_SIZE?

	hdr = tx_ring->tso_headers + i * TSO_HEADER_SIZE;
	tso_build_hdr(skb, hdr, &tso, data_len, total_len == 0);

tso_build_hdr() copies hdr_len bytes, where hdr_len comes from
tso_start() and covers the full L2+L3+L4 header. Nothing compares it
against TSO_HEADER_SIZE (256).

enetc_lso_map_hdr() does the same:

	hdr = tx_ring->tso_headers + *i * TSO_HEADER_SIZE;
	memcpy(hdr, skb->data, lso->hdr_len);

enetc_alloc_tx_resource() allocates exactly bd_count * TSO_HEADER_SIZE
bytes for this buffer. An oversized header therefore spills into the
next slot. For the last ring index, it writes past the end of the
dma_alloc_coherent() allocation.

One possible trigger is an IPv6 TCP socket with a sticky IPV6_RTHDR
type-4 SRH, which doesn't need CAP_NET_RAW. With 11 or more segments,
the header is 14 + 40 + 184 + 32 = 270 bytes.

The IPv6 setsockopt and net/core/tso.c side hasn't been fully traced in
this tree, so the trigger may still need confirming.

Should skbs with hdr_len > TSO_HEADER_SIZE fall back to software
segmentation or be dropped? Clearing GSO for them in ndo_features_check
would be another option.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding
  2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean
  2026-10-22  8:33   ` David Laight
@ 2026-10-25 15:21   ` netdev-bot+sashiko
  1 sibling, 0 replies; 15+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw)
  To: vladimir.oltean
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil,
	wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi,
	zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In enetc_start_xmit(), the new early exit `if
  (eth_skb_pad(skb)) return NETDEV_TX_OK;` runs after the…
- [Medium] enetc_tso_count_descs() now reserves tso_count_descs(skb) +
  gso_segs, i.e. 3*gso_segs + nr_frags, or 4*gso_segs + nr_frags with a…
- [Low] This patch leaves the XDP transmit paths unpadded.

--

net: enetc: do not rely on mEMAC hardware frame padding

This commit pads short frames to ETH_ZLEN in software on the enetc
transmit path, instead of relying on the mEMAC, because the padding the
LS1028A MAC inserts is sometimes not zero. Non-GSO skbs are padded with
eth_skb_pad(). Software TSO segments shorter than ETH_ZLEN get an extra
BD that points to a shared, zeroed DMA-coherent buffer, and the
worst-case TSO descriptor reservation grows by one BD per segment.

> 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.

[Severity: Low]
Does this cover the XDP transmit paths? enetc_xdp_tx() is reached from
XDP_TX in enetc_clean_rx_ring_xdp() and from enetc_xdp_xmit(). It still
uses the raw frame length and adds no padding:

	while (unlikely(!tmp_tx_swbd->is_eof)) {
		tmp_tx_swbd++;
		frm_len += tmp_tx_swbd->len;
	}

ENETC_PM0_CMD_TXP is still set in enetc_configure_port_mac(). That means
the mEMAC would still pad short XDP_TX and XDP_REDIRECT frames on LS1028A
after this commit.

The last patch in the series, "net: enetc: pad short XDP frames in
software", seems to fix this by appending a tx_pad BD in enetc_xdp_tx().
So this only affects the intermediate state of the series.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index ab4bfefda6c8f..f0f9ba94b4df6 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -690,7 +706,8 @@ static int enetc_lso_count_descs(const struct sk_buff *skb)
>  
>  static int enetc_tso_count_descs(const struct sk_buff *skb)
>  {
> -	int count = tso_count_descs(skb);
> +	/* +gso_segs to cover one pad BD per segment */
> +	int count = tso_count_descs(skb) + skb_shinfo(skb)->gso_segs;

[Severity: Medium]
Can reserving a pad BD for every segment push the reservation past the
ring size for skbs that used to transmit fine?

Every segment only needs a pad BD when hdr_len + gso_size < ETH_ZLEN.
Otherwise at most the last, shorter segment needs one. With this change
the reservation becomes 3 * gso_segs + nr_frags, or 4 * gso_segs +
nr_frags with a VLAN tag.

The TX ring is fixed at ENETC_TX_RING_DEFAULT_SIZE (2048), so
enetc_bd_unused() returns at most 2047. The driver does not lower
gso_max_segs or tso_max_segs. A software TSO skb with more than about 677
segments (about 507 with VLAN) can now never fit. Before this patch the
limit was about 1015 segments (677 with VLAN).

For example, a TCP flow with an MSS of about 88 bytes and a 64KB GSO skb
has about 744 segments. enetc_start_xmit() then does this even on an
empty ring:

	if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) {
		netif_stop_subqueue(ndev, tx_ring->index);
		return NETDEV_TX_BUSY;
	}

enetc_clean_tx_ring() only wakes the queue after it completes a frame,
and the driver has no ndo_tx_timeout. Wouldn't the requeued skb then
block this TX queue until ifdown/ifup?

The same stall already happened above about 1015 segments; this change
lowers the threshold.

Could pad BDs be reserved only when padding can happen (gso_segs pad BDs
if hdr_len + gso_size < ETH_ZLEN, otherwise one)? Alternatively,
netif_set_tso_max_segs() could cap the segment count so the worst case
fits in the ring.

>  
>  	/* Each segment also needs an extension BD for VLAN insertion */
>  	if (skb_vlan_tag_present(skb))

[ ... ]

> @@ -1053,6 +1088,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>  			enetc_unlock_mdio();
>  		}
>  	} else {
> +		if (eth_skb_pad(skb))
> +			return NETDEV_TX_OK;
> +

[Severity: Medium]
Can this early return leak the ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS bit?

For one-step PTP Sync skbs, enetc_start_xmit() has already taken the bit
by the time it gets here:

	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
		if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS,
					  &priv->flags)) {
			skb_queue_tail(&priv->tx_skbs, skb);
			return NETDEV_TX_OK;
		}
	}

An L2 PTP Sync is 14 + 44 = 58 bytes, so it now always goes through
eth_skb_pad(). If the skb is cloned or lacks tailroom, the call chain
eth_skb_pad()->skb_put_padto()->__skb_pad() calls
pskb_expand_head(GFP_ATOMIC). If that fails, the skb is freed and an
error is returned.

The bit is only released in enetc_tx_onestep_tstamp():

	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);

That work is scheduled from enetc_clean_tx_ring() when the one-step skb
completes on the TX ring. A dropped skb never reaches the ring, so the
work never runs.

After that, wouldn't every later one-step Sync fail
test_and_set_bit_lock() and be added to priv->tx_skbs, with nothing left
to dequeue it? One-step PTP transmit would stop for good and tx_skbs
would grow without bound. The bit also survives ifdown/ifup, because
enetc_tx_onestep_tstamp_init() only reinitializes the work and the queue.

The worker hits the same path when it calls enetc_start_xmit() on a
dequeued skb. The existing drop_packet_err and NETDEV_TX_BUSY exits have
the same problem. So does the pskb_may_pull() drop added by the next
patch, "net: enetc: ensure head buffer has at least 16 octets".

Could the bit be released (and tx_skbs drained) on every exit that does
not post the skb to the ring? Or could the padding happen before the
bit-lock is taken?

>  		if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags))
>  			if (unlikely(skb_linearize(skb)))
>  				goto drop_packet_err;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets
  2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean
@ 2026-10-25 15:21   ` netdev-bot+sashiko
  0 siblings, 0 replies; 15+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw)
  To: vladimir.oltean
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil,
	wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi,
	zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] At this commit, the 16-octet minimum for the head BUFF_LEN and
  for FRM_LEN is enforced only in the non-GSO skb branch of…
- [Low] The commit message says the pskb_may_pull() handling "is case 5
  below".

Pre-existing issues:
- [Medium] In enetc_start_xmit(), when an skb has
  ENETC_F_TX_ONESTEP_SYNC_TSTAMP set, the caller takes the…

--

net: enetc: ensure head buffer has at least 16 octets

On the non-GSO path, enetc_start_xmit() now uses pskb_may_pull() to move
bytes from the paged area into the head when the head buffer is shorter
than 16 octets. The ENETC does not accept a first TX BD with BUFF_LEN
below 16. Such a descriptor stalls the TX ring consumer index and leads
to an IRQ storm.

Closes: https://lore.kernel.org/netdev/b3d9136c-2803-4203-b1ea-1f9e62de80a1@gmail.com/

> Because the driver now pads linear skbs to ETH_ZLEN for other reasons
> too, we just need to ensure the head buffer has at least 16 bytes.
> This can be done using pskb_may_pull(), which moves that number of bytes
> from the paged area into the head. This is case 5 below.
>
>   skb_headlen (H) | skb->len (L) | linear? | handled by
>   ----------------+--------------+---------+---------------------------------------
>    H == L         | L < 60       |  yes    | eth_skb_pad() (pads head to 60)
>    H == L         | L >= 60      |  yes    | already valid, no-op
>    H < 16         | L < 60       |  no     | eth_skb_pad() (linearizes + pads to 60)
>    H < 16         | L >= 60      |  no     | pskb_may_pull() (pulls frags into head)
>    H >= 16        | L >= 60      |  no     | already valid, no-op

[Severity: Low]
This isn't a bug, but should "case 5" be "case 4"? The pskb_may_pull()
case (H < 16, L >= 60) is the fourth row. The fifth row (H >= 16,
L >= 60) is the no-op case.

The table also has no row for the non-linear H >= 16, L < 60 case.
eth_skb_pad() handles that one too, because
skb_put_padto()->__skb_pad() linearizes a non-linear skb before padding.

Could that row be added so the case analysis is complete?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index f0f9ba94b4df6..f5a47d53d2bea 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1091,6 +1091,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>  		if (eth_skb_pad(skb))
>  			return NETDEV_TX_OK;
>  
> +		if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
> +			     !pskb_may_pull(skb, ENETC_MIN_BUFF_SIZE))) {

[Severity: Low]
This covers the non-GSO skb path. Can the XDP transmit path still build
a first BD with BUFF_LEN < 16?

enetc_xdp_xmit()
  enetc_xdp_frame_to_xdp_tx_swbd()   <- head len is xdp_frame->len
  enetc_xdp_tx()
    enetc_xdp_map_tx_buff()

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_xdp_map_tx_buff() {
    ...
	txbd->buf_len = cpu_to_le16(tx_swbd->len);
	txbd->frm_len = cpu_to_le16(frm_len);
    ...
}

Neither length is checked against ENETC_MIN_BUFF_SIZE at this commit.
Two kinds of frame could still produce the invalid BD:

  - a redirected frame that bpf_xdp_adjust_tail() shrank to ETH_HLEN
  - a multi-buffer xdp_frame with a 14-15 byte head

Either one looks like it could cause the TBaCIR stall and IRQ storm
described in the commit message.

The later patch in this series, "net: enetc: pad short XDP frames in
software", appears to close this. It makes enetc_xdp_tx() drop frames
whose first buffer is shorter than ENETC_MIN_BUFF_SIZE, and it adds a
pad BD so that FRM_LEN reaches ETH_ZLEN. enetc_xdp_xmit() and the XDP_TX
path both go through enetc_xdp_tx(), so the gap only exists at this
intermediate commit.

> +			dev_kfree_skb_any(skb);
> +			return NETDEV_TX_OK;
> +		}

[Severity: Medium]
This is a pre-existing issue that was not introduced by this patch, but
the new drop path inherits it. If the skb has
ENETC_F_TX_ONESTEP_SYNC_TSTAMP set, enetc_start_xmit() has already taken
the bit at the top of the function:

	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
		if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS,
					  &priv->flags)) {
			skb_queue_tail(&priv->tx_skbs, skb);
			return NETDEV_TX_OK;
		}
	}

If pskb_may_pull() fails here, the skb is freed. The bit is not cleared
and priv->tx_onestep_tstamp is not scheduled.

Only enetc_tx_onestep_tstamp() releases the bit. That work is scheduled
only from enetc_clean_tx_ring(), when a one-step skb completes in the TX
ring, and a dropped skb never gets there.

After such a drop, would every later one-step Sync skb stay in
priv->tx_skbs forever, with the queue growing without bound?

The existing drop_packet_err label already leaks the bit the same way.
It is reached on skb_linearize() failure, or when DMA mapping fails and
count is zero. A common drop path that releases the bit (or schedules
the work) for one-step skbs would fix both cases, separately from this
patch.

>  
>  		if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags))
>  			if (unlikely(skb_linearize(skb)))
>  				goto drop_packet_err;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software
  2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean
@ 2026-10-25 15:21   ` netdev-bot+sashiko
  0 siblings, 0 replies; 15+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw)
  To: vladimir.oltean
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil,
	wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi,
	zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new XDP pad BD in enetc_xdp_tx() is only partly initialized,
  so after a failed XDP reconfiguration it can inherit a stale…
- [Low] The commit message understates what the new 16-byte head-buffer
  check in enetc_xdp_tx() fixes.

Pre-existing issues:
- [High] Concurrent, unsynchronized producers on the same XDP TX ring.

--

net: enetc: pad short XDP frames in software

This patch makes enetc_xdp_tx() drop XDP frames whose head buffer is
shorter than 16 bytes. When a frame is shorter than ETH_ZLEN, it appends
a BD pointing to the shared tx_pad zero buffer. The pad BD then takes
over the F flag, the is_eof marker and the xdp_frame pointer from the
last data BD.

> Similar to the skb path, the XDP transmit path may put frames shorter
> than ETH_ZLEN in the TX ring, and may put frames whose first buffer is
> shorter than 16 bytes. The former relies on mEMAC hardware padding,
> which we no longer want to rely upon, and the latter is not supported
> by ENETC at all.

[Severity: Low]
Could the commit message say what happens without the 16-byte check?

Before this patch, enetc_xdp_tx() only checked enetc_bd_unused(). A head
BD shorter than 16 bytes went straight into the TX ring.

The earlier patch in this series, "net: enetc: ensure head buffer has at
least 16 octets", describes the result. TBaCIR does not advance,
TBaSR[BUSY] stays set, and the NAPI/TBaIER re-enable sequence turns this
into an MSI-X IRQ storm that soft-locks the host.

That looks reachable today through ndo_xdp_xmit(). For example, a veth
XDP program redirecting through a devmap can forward a 14 or 15 byte
frame. The frame can come from bpf_xdp_adjust_tail(), or the peer can
simply send one.

The Fixes tags are already there. What seems to be missing is a
description of the impact, a soft lockup on the XDP redirect path.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index f5a47d53d2bea..e3a08a82dc3c9 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -1771,15 +1774,24 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
>  		frm_len += tmp_tx_swbd->len;
>  	}
>  
> +	/* Pad frames below ETH_ZLEN so the mEMAC does not pad them. */
> +	pad_len = max_t(int, ETH_ZLEN - frm_len, 0);
> +
> +	if (unlikely(enetc_bd_unused(tx_ring) <
> +		     ENETC_TXBDS_NEEDED(num_tx_swbd + !!pad_len)))
> +		return false;
> +
>  	i = tx_ring->next_to_use;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can
two producers write to the same XDP TX ring at the same time?

enetc_xdp_xmit() picks the ring by the executing CPU:

	tx_ring = priv->xdp_tx_ring[smp_processor_id()];

The XDP_TX path in enetc_clean_rx_ring_xdp() picks it by RX ring index:

	tx_ring = priv->xdp_tx_ring[rx_ring->index];

The only thing tying a vector to a CPU is irq_set_affinity_hint() in
enetc_setup_irqs(). irqbalance, /proc/irq/*/smp_affinity or threaded
NAPI can run RX vector i's NAPI on CPU j. Meanwhile CPU i can call
ndo_xdp_xmit() into this device, for example from a devmap redirect.

enetc_lock_mdio() takes read_lock(&enetc_mdio_lock), and only when the
ERR050089 workaround is active. That does not serialize the two paths.

Both producers can then:

- pass the enetc_bd_unused() check;
- read the same next_to_use;
- overwrite the same swbds and BDs, now including the pad BD and the
  xdp_frame handoff;
- write next_to_use and the tail.

Could that lose frames or leak DMA mappings and xdp_frames? It could
also leave xdp_tx_in_flight undecremented for overwritten XDP_TX
buffers, which throttles RX refill, or hand torn BDs to the hardware.

Would per-ring producer serialization be needed here? That could be a
spinlock taken by both paths, or __netif_tx_lock() as other drivers use
for shared XDP queues.

>  
>  	for (k = 0; k < num_tx_swbd; k++) {
>  		struct enetc_tx_swbd *xdp_tx_swbd = &xdp_tx_arr[k];

[ ... ]

> @@ -1788,6 +1800,22 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring,
>  		enetc_bdr_idx_inc(tx_ring, &i);
>  	}
>  
> +	/* The pad BD becomes the last one, so it takes over the xdp_frame
> +	 * (if any) from the last data BD. The pad BD has no DMA mapping of
> +	 * its own, and it is not an XDP_TX buffer to be recycled.
> +	 */
> +	if (unlikely(pad_len)) {
> +		struct enetc_tx_swbd *pad_swbd = &tx_ring->tx_swbd[i];
> +
> +		enetc_append_tx_pad(tx_ring, i, pad_len);
> +		pad_swbd->is_xdp_redirect = last_swbd->is_xdp_redirect;
> +		pad_swbd->xdp_frame = last_swbd->xdp_frame;

[Severity: High]
Can pad_swbd keep a stale is_xdp_tx and page from an earlier use of
this slot?

enetc_append_tx_pad() sets only len, is_eof, do_twostep_tstamp and
check_wb. The code here adds is_xdp_redirect and xdp_frame. Nothing
writes is_xdp_tx, page, page_offset, dir, dma or is_dma_page.

The data BDs are fine, because enetc_xdp_map_tx_buff() memcpy()s the
whole swbd. That makes the pad BD the first XDP ring slot that depends
on already being zero.

The slot is not always zero. On an XDP program change,
enetc_reconfigure() calls enetc_stop() and then enetc_free_rxtx_rings().
For XDP_TX slots that were not cleaned,
enetc_free_tx_ring()->enetc_free_tx_frame() only does this:

	if (tx_swbd->dma)
		enetc_unmap_tx_buff(tx_ring, tx_swbd);

That clears dma but leaves is_xdp_tx = 1 and page set.

netif_set_real_num_tx_queues() can then fail in
enetc_reconfigure_xdp_cb(), for example with -ENOMEM when an XDP program
is removed. In that case the out_restart path restarts the old rings
with the same tx_swbd arrays:

out_restart:
	enetc_setup_bdrs(priv, extended);
	enetc_start(priv->ndev);

The old XDP program is restored, so XDP_TX keeps running. Every 56 byte
VLAN-stripped XDP_TX frame now gets a pad BD. If a pad BD lands on a
stale slot, enetc_clean_tx_ring() checks is_xdp_tx before dma:

		if (tx_swbd->is_xdp_tx)
			enetc_recycle_xdp_tx_buff(tx_ring, tx_swbd);

Would that put {dma = 0, stale page, stale page_offset} into the RX
ring? It would also call dma_sync_single_range_for_device() on address 0
and wrongly decrement xdp_tx_in_flight.

enetc_refill_rx_ring() would then program rxbd->w.addr with
0 + page_offset, so RX DMA would go to a bus address near 0. If the RX
ring is full, dma_unmap_page() would run on address 0 instead.

The incomplete scrubbing on the out_restart path is older than this
patch. The new pad BD is what exposes it on XDP rings.

Would a memset of the swbd in enetc_append_tx_pad() be enough? A more
robust option would be for enetc_free_tx_frame()/enetc_free_tx_ring() to
scrub the whole swbd, so slots are zero after teardown as well.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2026-10-25 15:21 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean
2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean
2026-10-25 15:21   ` netdev-bot+sashiko
2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean
2026-10-22  8:33   ` David Laight
2026-10-22  9:41     ` Vladimir Oltean
2026-10-22 10:22       ` David Laight
2026-10-22 12:35         ` Vladimir Oltean
2026-10-22 20:48           ` David Laight
2026-10-23  9:35             ` Vladimir Oltean
2026-10-25 15:21   ` netdev-bot+sashiko
2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean
2026-10-25 15:21   ` netdev-bot+sashiko
2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean
2026-10-25 15:21   ` netdev-bot+sashiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®