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

["Gupta, Suraj" <suraj.gupta2@amd.com>]

On 9/1/2026 8:39 PM, sashiko-bot@kernel.org wrote:
> 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?
> 



We don't support legacy DMA for XXV ethernet & fail probe if dmaengine 
properties are missing. So this is a false issue.
> [ ... ]
>> 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?
> 


Same here, XXV ethernet driver supports dmaengine flow only. Probe will 
fail if dmanegine properties are missing in XXV DT. So this condition 
won't occur and is false positive issue.

> [ ... ]
>> +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?
> 


As mentioned in cover-letter, due to phylink limitations, we aren't able 
to advertise both 10G and 25G base-r interfaces and are open to 
suggestions to handle it.

Regards,
Suraj