Re: [PATCH net-next v5 04/13] net: lan966x: add FDMA LLP register write helper

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving the LLP register configuration to the allocation function breaks the MTU change error recovery path, leaving the RX channel reactivated without a valid LLP.

Pre-existing issues:
- [Critical] The return value of `fdma_dcbs_init()` is ignored in `lan966x_fdma_rx_alloc()`, leading to partial initialization and potential memory corruption.
- [Critical] Setting the PTP flag for the TX buffer after handing the DCB to hardware creates a race condition with the TX completion handler.
- [High] The hardware and software DMA states become desynchronized during the MTU reload error path due to taking a stale snapshot of the FDMA state before stopping the traffic.
- [High] Missing `dma_wmb()` before updating the `nextptr` of the previous DCB in `__fdma_dcb_add()`.
--

commit 4390e4c62295adb12eb1d72e9557c5052040eb8a
Author: Daniel Machon <daniel.machon@microchip.com>

net: lan966x: add FDMA LLP register write helper

This patch introduces a shared helper for configuring the FDMA Link List
Pointer (LLP) register and moves the LLP configuration from the channel
start/activate functions to the allocation functions. This change
prevents the shared start/activate path from overwriting ATU-translated
addresses with DMA addresses in the upcoming PCIe FDMA path.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index f8ce735a7fc09..6c5761e886d47 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c

[ ... ]

> @@ -127,6 +134,9 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
>  	fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
>  		       FDMA_DCB_STATUS_INTR);

[Severity: Critical]
This isn't a bug introduced by this patch, but is it safe to ignore the
return value of fdma_dcbs_init() here?

If fdma_dcbs_init() fails (e.g., due to page allocation failure in the
data pointer callback), it returns an error code, leaving subsequent
DCBs with uninitialized or zeroed data pointers. Because the return
value is ignored, could the driver incorrectly proceed to activate the
DMA ring, causing the hardware to attempt to DMA incoming packets into
physical address 0?

> +	lan966x_fdma_llp_configure(lan966x, (u64)fdma->dma,
> +				   fdma->channel_id);
> +
>  	return 0;
>  }

[ ... ]

> @@ -136,14 +146,6 @@ static void lan966x_fdma_rx_start(struct lan966x_rx *rx)
>  	struct fdma *fdma = &rx->fdma;
>  	u32 mask;
>  
> -	/* When activating a channel, first is required to write the first DCB
> -	 * address and then to activate it
> -	 */
> -	lan_wr(lower_32_bits((u64)fdma->dma), lan966x,
> -	       FDMA_DCB_LLP(fdma->channel_id));
> -	lan_wr(upper_32_bits((u64)fdma->dma), lan966x,
> -	       FDMA_DCB_LLP1(fdma->channel_id));
> -

[Severity: High]
Does moving the LLP register configuration to the allocation function
break the MTU change error recovery path?

During an MTU reload failure, the driver restores the software state and
calls lan966x_fdma_rx_start(), which now fails to write the required
LLP register. Could this activate the hardware DMA channel without a
valid Link List Pointer, leading to DMA failures or hardware stalls?

[Severity: High]
This is a pre-existing issue, but does taking a stale snapshot of the
FDMA state before stopping the traffic cause the hardware and software
DMA states to become desynchronized during the MTU reload error path?

In lan966x_fdma_reload(), the software snapshot fdma_rx_old is taken
before NAPI and the hardware channel are disabled. If packets arrive in
this window, the hardware advances.

On allocation failure, restoring this stale snapshot rolls back the
software indices while the hardware's internal state remains advanced.
Could this permanently desynchronize the ring indices and cause ring
corruption?

>  	lan_wr(FDMA_CH_CFG_CH_DCB_DB_CNT_SET(fdma->n_dbs) |
>  	       FDMA_CH_CFG_CH_INTR_DB_EOF_ONLY_SET(1) |
>  	       FDMA_CH_CFG_CH_INJ_PORT_SET(0) |

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a missing dma_wmb() before
updating the nextptr of the previous DCB in __fdma_dcb_add()?

When appending a new DCB to a running TX ring in lan966x_fdma_xmit(),
the CPU updates the new DB's status and data pointers, and then updates
the nextptr of the previous DCB to link it. Without a dma_wmb() separating
these writes, could the CPU or compiler reorder them?

If the hardware polling the previous DCB's nextptr observes the new link
before the new DCB's contents are visible in memory, could this lead to
the hardware reading garbage descriptors?

[Severity: Critical]
This isn't a bug introduced by this patch, but does setting the PTP flag
for the TX buffer after handing the DCB to hardware create a race condition
with the TX completion handler?

In lan966x_fdma_xmit(), the DCB is submitted to the hardware via
__fdma_dcb_add() and lan966x_fdma_tx_start() before the software sets
next_dcb_buf->ptp = true.

If the hardware transmits the packet immediately and raises a completion
interrupt, could a concurrent NAPI poll process the completion, read the
stale ptp == false state, and free the SKB? Could the submitting CPU then
set the PTP flag on the already freed SKB, causing a use-after-free during
subsequent PTP timestamp processing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-lan966x-pci-fdma-v5-0-ca56197ae05b@microchip.com?part=4