[PATCH net-next] net: xilinx: axienet: Derive RX frame length from residue in dmaengine path

[PATCH net-next] net: xilinx: axienet: Derive RX frame length from residue in dmaengine path

[Srinivas Neeli]

The dmaengine RX path derived the received frame length from the descriptor
APP metadata. That only works when the optional AXI4-Stream status/control
interface is present, because the hardware populates the APP fields solely
when that interface is enabled. On designs without it the length read back
is invalid.

The AXI DMA engine already reports how many bytes it wrote into the buffer
through the standard dmaengine residue mechanism. Compute the RX frame
length as the posted buffer length minus result->residue, which is
independent of the status/control interface and correct across all designs,
including multi-descriptor frames where the residue is summed over the
chain.

Drop the descriptor metadata lookup, which was only used for this purpose.
Detect a failed transfer from dmaengine_result.result instead of the
metadata pointer return value, and remove the now unused LEN_APP macro.

The transmit path is unaffected. It still passes APP metadata for checksum
offload and derives its length from the skb.

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
---
This patch was originally posted as part of the MCDMA series.
Since the required MCDMA changes have already been merged into the
Linux branch, this patch no longer has any unresolved dependencies.
As it only modifies the AXI Ethernet driver, it is being submitted
separately to net-next.

https://lore.kernel.org/lkml/20260818174151.1608971-4-srinivas.neeli@amd.com
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 782f903d318f..e7490650482b 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -53,7 +53,6 @@
 #define TX_BD_NUM_MAX			4096
 #define RX_BD_NUM_MAX			4096
 #define DMA_NUM_APP_WORDS		5
-#define LEN_APP				4
 #define RX_BUF_NUM_DEFAULT		128
 
 /* Must be shorter than length of ethtool_drvinfo.driver field to fit */
@@ -1159,29 +1158,26 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 static void axienet_dma_rx_cb(void *data, const struct dmaengine_result *result)
 {
 	struct skbuf_dma_descriptor *skbuf_dma;
-	size_t meta_len, meta_max_len, rx_len;
 	struct axienet_local *lp = data;
 	struct sk_buff *skb;
-	u32 *app_metadata;
+	size_t rx_len;
 	int i;
 
 	skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_tail++);
 	skb = skbuf_dma->skb;
-	app_metadata = dmaengine_desc_get_metadata_ptr(skbuf_dma->desc, &meta_len,
-						       &meta_max_len);
 	dma_unmap_single(lp->dev, skbuf_dma->dma_address, lp->max_frm_size,
 			 DMA_FROM_DEVICE);
 
-	if (IS_ERR(app_metadata)) {
+	if (result->result != DMA_TRANS_NOERROR) {
 		if (net_ratelimit())
-			netdev_err(lp->ndev, "Failed to get RX metadata pointer\n");
+			netdev_err(lp->ndev, "RX DMA transfer failed\n");
 		dev_kfree_skb_any(skb);
 		lp->ndev->stats.rx_dropped++;
 		goto rx_submit;
 	}
 
-	/* TODO: Derive app word index programmatically */
-	rx_len = (app_metadata[LEN_APP] & 0xFFFF);
+	/* Actual length = posted buffer length - residue. */
+	rx_len = lp->max_frm_size - result->residue;
 	skb_put(skb, rx_len);
 	skb->protocol = eth_type_trans(skb, lp->ndev);
 	skb->ip_summed = CHECKSUM_NONE;
-- 
2.43.0

Re: [PATCH net-next] net: xilinx: axienet: Derive RX frame length from residue in dmaengine path

[netdev-bot+sashiko]

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

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

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] In direct (non-SG) AXI DMA mode, the new residue-based length
  makes axienet_dma_rx_cb() deliver RX buffers that the hardware never…
- [High] On MCDMA, which the cover note names as the target of this
  change, the residue-based path accepts successful completions for…
- [High] The commit message says the residue approach is correct
  'including multi-descriptor frames where the residue is summed over…
- [High] The residue-based length cannot tell a complete frame from a
  truncated chunk of an oversized one.
- [Medium] axienet_dma_rx_cb() computes rx_len = lp->max_frm_size -
  result->residue and feeds it straight into skb_put() and…
- [Low] The patch has no Fixes: tag and targets net-next, but its commit
  message describes a user-visible failure of existing code: 'On designs…
- [Low] The commit message describes the old failure only as an 'invalid'
  length.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but RX can stop
  permanently after refill failures.
- [Medium] This problem wasn't introduced by this patch, but
  axienet_rx_submit_desc() advances lp->rx_ring_head before…
- [Medium] This problem wasn't introduced by this patch, but
  axienet_stop() leaks the outstanding RX skbs and their streaming DMA…

--

net: xilinx: axienet: Derive RX frame length from residue in dmaengine path

This patch changes how the axienet dmaengine RX callback gets the frame
length. It now uses the posted buffer length minus the dmaengine residue
instead of the APP metadata words. Failed transfers are detected from
dmaengine_result.result, and the unused LEN_APP macro is removed.

> The dmaengine RX path derived the received frame length from the descriptor
> APP metadata. That only works when the optional AXI4-Stream status/control
> interface is present, because the hardware populates the APP fields solely
> when that interface is enabled. On designs without it the length read back
> is invalid.

[Severity: Low]
This describes a failure users can see in existing code. The patch has no
Fixes: tag and is aimed at net-next. Should it carry a Fixes: tag?

The APP word length read that this patch replaces:

    rx_len = (app_metadata[LEN_APP] & 0xFFFF);

came in with commit 6a91b846af85 ("net: axienet: Introduce dmaengine
support"). xilinx_dma has populated the SG residue that the new code
relies on since d8bae21a48dbe1.

If such designs were never considered supported, could the message
describe this as enabling new hardware configurations instead of fixing
invalid behaviour? In either case, the RX length questions below would
probably need to be settled before this is suitable for stable.

[Severity: Low]
Is "the length read back is invalid" accurate for the main target
configurations?

xilinx_dma only installs metadata_ops when xdev->has_axistream_connected
is set. That comes from the "xlnx,axistream-connected" DT property, which
is only parsed for AXIDMA. xilinx_mcdma_prep_slave_sg() never sets
metadata_ops.

In those cases dmaengine_desc_get_metadata_ptr() returns an ERR_PTR. The
old callback then dropped every RX frame with "Failed to get RX metadata
pointer". Before 8bbceba7dc5090 it dereferenced the ERR_PTR instead.

Could the message mention that symptom? It could also say that the RX
path no longer needs a metadata-capable DMA channel.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 782f903d318f3..e7490650482bb 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c

[ ... ]

> @@ -1159,29 +1158,26 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  static void axienet_dma_rx_cb(void *data, const struct dmaengine_result *result)
>  {

[ ... ]

>  	skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_tail++);
>  	skb = skbuf_dma->skb;
> -	app_metadata = dmaengine_desc_get_metadata_ptr(skbuf_dma->desc, &meta_len,
> -						       &meta_max_len);
>  	dma_unmap_single(lp->dev, skbuf_dma->dma_address, lp->max_frm_size,
>  			 DMA_FROM_DEVICE);
>  
> -	if (IS_ERR(app_metadata)) {
> +	if (result->result != DMA_TRANS_NOERROR) {

[Severity: High]
With the metadata check gone, this is the only validation left. Can MCDMA
report DMA_TRANS_NOERROR here for RX descriptors the hardware has not
finished?

xilinx_dma_complete_descriptor() only checks the BD completion bit for
AXI DMA:

    if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
        ... XILINX_DMA_BD_COMP_MASK ...
    }

So on MCDMA, every IOC interrupt retires the whole active_list.

Since commit 60786900347901, xilinx_mcdma_start_transfer() extends the
running chain on every refill. It sets the coalesce threshold from
desc_pendingcount:

    if (chan->desc_pendingcount <= XILINX_MCDMA_COALESCE_MAX) {
        reg &= ~XILINX_MCDMA_COALESCE_MASK;
        reg |= chan->desc_pendingcount << XILINX_MCDMA_COALESCE_SHIFT;

That count only covers the newly pending batch, which is 1 per refill.
The initial batch of 128 is above XILINX_MCDMA_COALESCE_MAX, so the
threshold is not reprogrammed at all. An IOC after a single packet would
then retire every outstanding RX descriptor.

The hardware never wrote the status of those descriptors, so
xilinx_dma_get_residue() returns the full control length. This callback
then computes rx_len = 0 and passes a zero-length skb to eth_type_trans()
and __netif_rx().

At the same time, xilinx_dma recycles those segments while the hardware
still owns the chain (xilinx_mcdma_clean_hw_desc() memsets the BD). The
hardware can then DMA into buffers that this callback has already
unmapped and handed to the stack. That ownership part predates this
patch, because the old IS_ERR path also freed the skb.

MCDMA is the configuration this change targets. Does the provider need to
validate completion per descriptor before the residue can be trusted
here?

>  		if (net_ratelimit())
> -			netdev_err(lp->ndev, "Failed to get RX metadata pointer\n");
> +			netdev_err(lp->ndev, "RX DMA transfer failed\n");

[ ... ]

> -	/* TODO: Derive app word index programmatically */
> -	rx_len = (app_metadata[LEN_APP] & 0xFFFF);
> +	/* Actual length = posted buffer length - residue. */
> +	rx_len = lp->max_frm_size - result->residue;
>  	skb_put(skb, rx_len);
>  	skb->protocol = eth_type_trans(skb, lp->ndev);

[Severity: High]
In direct (non-SG) AXI DMA mode, does this hand RX buffers to the stack
that the hardware never wrote?

axienet_init_dmaengine() queues RX_BUF_NUM_DEFAULT (128) descriptors and
calls dma_async_issue_pending() once. In direct mode,
xilinx_dma_start_transfer() programs only the first descriptor, but moves
all 128 to active_list:

    dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
                   hw->control & chan->xdev->max_buffer_len);
    ...
    list_splice_tail_init(&chan->pending_list, &chan->active_list);

When that one transfer completes, xilinx_dma_complete_descriptor()
retires every active descriptor. Without SG, nothing stops the loop:

    if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
        break;

Each descriptor gets the same residue from
xilinx_dma_get_residue_axidma_direct_s2mm(), which is hw->control minus
BTT. With equal-sized buffers, every callback computes rx_len equal to
the first packet's length.

The other skbs came from netdev_alloc_skb() and were never initialized.
skb_put(), eth_type_trans() and __netif_rx() would still pass them up as
packets. Before this patch, the metadata check dropped them via IS_ERR or
gave them rx_len 0.

xilinx_dma supports this mode explicitly (5c9142a8063f71,
60786900347901), and axienet does not reject it. Is "correct across all
designs" in the commit message accurate for this configuration?

[Severity: High]
The commit message says this is correct "including multi-descriptor
frames where the residue is summed over the chain". Does that hold when
one posted RX buffer is split across several hardware BDs?

axienet_rx_submit_desc() posts a single sg entry of lp->max_frm_size.
xilinx_dma_prep_slave_sg() splits it into segments of at most
max_buffer_len:

    copy = xilinx_dma_calc_copysize(chan, sg_dma_len(sg), sg_used);

That split happens whenever max_buffer_len (from xlnx,sg-length-width) is
smaller than max_frm_size. Examples are a width of 10 or less with a 1522
byte buffer, or 13 or less with a 9022 byte jumbo buffer.

S2MM closes a BD on TLAST and starts the next frame in the next BD. A
short frame that ends in BD0 can therefore be followed by the next frame
in BD1 of the same descriptor. xilinx_dma_get_residue() just adds up the
per-segment values:

    residue += (axidma_hw->control & chan->xdev->max_buffer_len) -
               (axidma_hw->status & chan->xdev->max_buffer_len);

Take a 1023/499 split and a 500 byte frame. rx_len here comes out as 999.
skb_put() then covers 499 bytes of the unwritten tail of BD0, which is
stale netdev_alloc_skb() memory, followed by the start of the next frame.
That next frame is itself truncated.

Could this leak uninitialized heap contents to AF_PACKET users, or onto
the wire when the port is bridged?

[Severity: High]
How does this tell a complete frame apart from a chunk of an oversized
frame?

For MTU > 1500, axienet_device_reset() enables jumbo receive but sizes
the RX buffer to the local MTU:

    if (ndev->mtu > XAE_MTU && ndev->mtu <= XAE_JUMBO_MTU) {
        lp->max_frm_size = ndev->mtu + VLAN_ETH_HLEN + XAE_TRL_SIZE;
        if (lp->max_frm_size <= lp->rxmem)
            lp->options |= XAE_OPTION_JUMBO;

No receive max-frame limit is programmed. The MAC therefore accepts
frames longer than one posted buffer, up to rxmem.

S2MM then carries such a frame into the BDs of the following descriptors.
Each of those completes with DMA_TRANS_NOERROR. The first has residue 0,
and the later ones hold the remainder.

Only result->result is checked, so each chunk is delivered as its own
frame. eth_type_trans() parses the start of each continuation chunk as a
new Ethernet header.

Can a sender on the link hide an inner frame in the tail of a jumbo frame
and have it processed as if it had arrived directly? That would bypass
MAC address filtering, as well as any VLAN or ACL decision an upstream
switch made on the outer frame.

Before this patch, the last chunk's app4 value (the full frame length)
reached skb_put() and panicked. Now the chunks are delivered silently.
Should full buffers, or chunks that do not start a frame, be dropped
here?

[Severity: Medium]
Should rx_len be checked against [ETH_HLEN, max_frm_size] before
skb_put() and eth_type_trans()?

This code also assumes the residue always covers the full posted length.
In direct S2MM mode with max_buffer_len < max_frm_size,
xilinx_dma_start_transfer() only programs the first segment, and the
residue covers only that segment:

    seg = list_first_entry(&desc->segments, ...);
    hw = &seg->hw;
    return hw->control - finished_len;

rx_len then works out to BTT + (max_frm_size - first_seg_len), which
includes the never-written tail of the netdev_alloc_skb() buffer.

When residue == max_frm_size, as in the MCDMA and direct mode cases
above, rx_len is 0. eth_type_trans() then fails to pull the MAC header
and reads h_proto from unwritten bytes.

A residue larger than max_frm_size would wrap and hit skb_over_panic().
That has not been shown to be reachable with correctly behaving
hardware.

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but can RX stop for good after
refill failures?

axienet_rx_submit_desc() returns without posting anything when
netdev_alloc_skb(), dma_map_single() or dmaengine_prep_slave_sg() fails:

    skb = netdev_alloc_skb(ndev, lp->max_frm_size);
    if (!skb)
        return;

After init, the only refill point is the loop at the end of
axienet_dma_rx_cb():

    for (i = 0; i < CIRC_SPACE(lp->rx_ring_head, lp->rx_ring_tail,
                               RX_BUF_NUM_DEFAULT); i++)
        axienet_rx_submit_desc(lp->ndev);

Suppose allocation keeps failing until every outstanding descriptor has
completed. Then no further callback runs and nothing retries, so RX stays
dead until the interface is brought down and up again.

[Severity: Medium]
This is a pre-existing issue, but axienet_rx_submit_desc() advances the
ring head before submitting, and it ignores the returned cookie:

    lp->rx_ring_head++;
    dmaengine_submit(dma_rx_desc);

xilinx_dma_tx_submit() can fail without queuing the descriptor:

    if (chan->err) {
        err = xilinx_dma_chan_reset(chan);
        if (err < 0)
            return err;
    }

axienet_dma_rx_cb() matches completions only by incrementing
rx_ring_tail. Would a later successful completion then be paired with the
wrong skb and DMA mapping?

[Severity: Medium]
This isn't a bug introduced by this patch, but does axienet_stop() leak
the outstanding RX skbs and their DMA mappings in dmaengine mode?

dmaengine_terminate_sync(lp->rx_chan) ends up in
xilinx_dma_free_descriptors(). That frees the descriptor lists without
calling the client callbacks. axienet_stop() then frees only the
wrappers:

    for (i = 0; i < RX_BUF_NUM_DEFAULT; i++)
        kfree(lp->rx_skb_ring[i]);
    kfree(lp->rx_skb_ring);

It never calls dma_unmap_single() or dev_kfree_skb() for the skbs posted
between rx_ring_tail and rx_ring_head. Each ifdown can leak up to 128
max_frm_size skbs and their mappings.

This patch does not change that, because the old IS_ERR path also
refilled the ring.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929081316.803997-1-srinivas.neeli%40amd.com