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

[netdev-bot+sashiko@kernel.org]

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