Re: [PATCH v3 net 4/7] net: enetc: pad short frames in software

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

On Wed, 16 Sep 2026 01:27:31 +0300
vladimir.oltean@nxp.com wrote:

> 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 is out of the driver's control, and the network
> stack can actually send packets with skb->len smaller than that. The
> result is unpleasant, as will be explained below, so for simplicity
> sake, we just pad anything shorter than ETH_ZLEN.
> 
> 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.
> 
> 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.
> 
> 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/
> Tested-by: Zefir Kurtisi <zefir.kurtisi@westermo.com>
> Reviewed-by: Wei Fang <wei.fang@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v3: none
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 13 +++++++++++++
>  drivers/net/ethernet/freescale/enetc/enetc.h |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 0216f7d08e19..bbad942041f5 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1077,6 +1077,19 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	u8 udp, msgtype, twostep;
>  	u16 offset1, offset2;
>  
> +	/* Hardware does not support transmit buffer descriptors with a total
> +	 * length of less than 16 bytes, or a first buffer size of less than
> +	 * 16 bytes.
> +	 */
> +	if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
> +		     skb_linearize(skb))) {

Isn't it only necessary to pull a few bytes into the linear region?

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

That could be inside the (skb_headlen(skb) < ENETC_MIN_BUFF_SIZE) test.

David

> +
>  	/* Mark tx timestamp type on enetc_cb->flag if requires */
>  	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
>  	    (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK))
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index d1e9d9130057..a4e76060e94b 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