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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked return value of `fdma_dcbs_init` in `lan966x_fdma_rx_alloc` leads to missing pages, causing a guaranteed NULL pointer dereference during teardown or reload.
- [High] Hardware and software RX ring pointers become permanently desynchronized on the `lan966x_fdma_reload` failure path, breaking RX completely.
--

commit 3d957750cc640b40763313c06874bd2261200dd3
Author: Daniel Machon <daniel.machon@microchip.com>

net: lan966x: add FDMA LLP register write helper

Move the FDMA Link List Pointer (LLP) register writes into a new shared
helper and call it from the allocation functions instead of the activate
and start functions. This avoids 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 41d4ec7f2f571..b8344fd5e5ad2 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
[ ... ]
> @@ -128,6 +135,8 @@ 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);
>  
> +	lan966x_fdma_llp_configure(lan966x, fdma->dma, fdma->channel_id);
> +
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed the return value of
fdma_dcbs_init() is ignored here in lan966x_fdma_rx_alloc().

If page allocation fails under memory pressure inside
lan966x_fdma_rx_dataptr_cb(), fdma_dcbs_init() aborts early and
returns -ENOMEM. The remaining rx->page[i][j] entries are left as NULL,
but since the error is ignored, initialization succeeds with an
incomplete ring.

Does this lead to a NULL pointer dereference in
lan966x_fdma_rx_free_pages() when the interface is brought down or an
MTU reload fails? That function blindly loops over all fdma->n_dcbs and
passes each page to page_pool_put_full_page() which will crash when trying
to access the page's reference count.

[ ... ]
> @@ -876,6 +871,9 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
>  					   MEM_TYPE_PAGE_POOL, page_pool);
>  	}
>  
> +	lan966x_fdma_llp_configure(lan966x, lan966x->rx.fdma.dma,
> +				   lan966x->rx.fdma.channel_id);
> +
>  	lan966x_fdma_rx_start(&lan966x->rx);
>  
>  	lan966x_fdma_wakeup_netdev(lan966x);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
lan966x_fdma_llp_configure() here in the restore: path of
lan966x_fdma_reload() cause the hardware and software ring pointers to
become permanently desynchronized?

The restore: path successfully restores the old rx->fdma state to memory
(via memcpy), reverting software's dcb_index to wherever it was before
the reload attempt. However, configuring the LLP register here forces the
hardware back to the start of the ring (index 0).

When lan966x_fdma_rx_start() starts the engine, hardware will write newly
received frames sequentially from index 0, but the software NAPI poll
(lan966x_fdma_napi_poll) will still poll the restored dcb_index. Can this
cause all newly received packets to be missed and stall the ring entirely
once the hardware wraps around and encounters the already 'done'
descriptors from its first pass?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671@microchip.com?part=4