Re: [PATCH net-next v3 3/6] net: xilinx: axienet: Add 10G/25G (XXV) ethernet support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing mac_link_up callback for XXV MAC causes runtime-switchable speeds and flow control to never be programmed.
- [High] Legacy DMA descriptors and SKBs are leaked if mac_init() fails in axienet_device_reset().
- [High] Failure of axienet_xxv_mac_init() is ignored in the DMA error handler, proceeding with uninitialized hardware state.
--

commit 0e020a3d14a024a203937c2f29cbb4fde9c6c31b
Author: Suraj Gupta <suraj.gupta2@amd.com>

net: xilinx: axienet: Add 10G/25G (XXV) ethernet support

This commit adds support for the Xilinx XXV Ethernet IP, introducing a new
axienet_10g25g_config structure and associated callbacks. It also propagates
axienet_device_reset() failures in axienet_open().

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index a4963da53b306..222c82a1de914 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
[ ... ]
> @@ -2818,6 +2819,9 @@ static void axienet_dma_err_handler(struct work_struct *work)
>  
>  	axienet_dma_start(lp);
>  
> +	/* This error handler runs only for the legacy embedded-DMA (1G) path,
> +	 * whose mac_init() cannot fail, so its return value is not checked.
> +	 */
>  	lp->axienet_config->mac_init(ndev);

[Severity: High]
If the XXV MAC falls back to legacy DMA due to a missing device tree property,
couldn't this error handler run for the XXV MAC as well?

If axienet_xxv_mac_init() fails here (e.g., from a timeout), does the driver
proceed to configure hardware options and re-enable packet reception on an
uninitialized or failed MAC?

[ ... ]
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c b/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c
> new file mode 100644
> index 0000000000000..46b48a773c15a
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c
[ ... ]
> +static int axienet_xxv_mac_init(struct net_device *ndev)
> +{
> +	struct axienet_local *lp = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = axienet_xxv_poll_link_ready(ndev);
> +	if (ret)
> +		return ret;

[Severity: High]
If this function returns an error, are the legacy DMA descriptors leaked?

Looking at axienet_device_reset(), which calls this function when using
legacy DMA:

axienet_device_reset()
    ...
    if (!lp->use_dmaengine) {
        ret = axienet_dma_bd_init(ndev);
        if (ret)
            return ret;
    }
    ...
    ret = lp->axienet_config->mac_init(ndev);
    if (ret)
        return ret;

When mac_init() fails, it returns early without calling
axienet_dma_bd_release(ndev). Can this cause a memory leak of the TX/RX
rings and their associated SKBs?

[ ... ]
> +const struct axienet_config axienet_10g25g_config = {
> +	.sw_padding = true,
> +	.internal_pcs = true,
> +	.regs_n = XXV_REGS_N,
> +	.clk_init = axienet_10g25g_clk_init,
> +	.setoptions = axienet_xxv_setoptions,
> +	.probe_init = axienet_xxv_probe_init,
> +	.gt_reset = axienet_xxv_gt_reset,
> +	.mac_init = axienet_xxv_mac_init,
> +	.get_regs = axienet_xxv_get_regs,
> +	.phylink_set_caps = axienet_xxv_phylink_set_capabilities,
> +	.pcs_ops = &axienet_xxv_pcs_ops,
> +};

[Severity: High]
Since axienet_xxv_phylink_set_capabilities() advertises runtime-switchable
speeds (10G and 25G), shouldn't there be a mac_link_up callback here to
program the negotiated speed and pause settings to the hardware?

Without a mac_link_up callback, when phylink resolves to a non-default speed
or enables pause frames, will the hardware remain unconfigured, potentially
causing the datapath to silently fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831150816.1020883-1-suraj.gupta2@amd.com?part=3