Re: [PATCH net v2] net: cpsw: fix netdev cleanup on probe failure

[Siddharth Vadapalli <s-vadapalli@ti.com>]

On 10/19/26 11:34 AM, Karl Mehltretter wrote:
> The legacy CPSW driver registers its netdevs before requesting IRQs. If
> a later IRQ request fails in dual-EMAC mode, the probe error path
> unregisters only the primary netdev. Devres then calls free_netdev() on
> the still-registered secondary netdev, triggering the reg_state BUG_ON.
> 
> Once registered, either interface can queue rx_mode_work through
> dev_set_rx_mode(). The error path does not drain the work before devres
> releases the netdev and its private data, allowing the worker to access
> freed memory.
> 
> Use the same per-port cleanup for probe failure and remove. Unregister
> each netdev whose reg_state is NETREG_REGISTERED, then drain its
> rx_mode_work. The state check is needed because cpsw_probe_dual_emac()
> stores the secondary pointer before register_netdev() succeeds.
> 
> The workqueue lifetime issue was found during an LLM-assisted review of
> the 6.12.112-rc1 stable series. Inspecting the same path then exposed
> the dual-EMAC cleanup bug.
> 
> An instrumented KASAN ARM kernel and a QEMU CPSW probe stub reproduced
> both failures by forcing the TX IRQ request to return -EBUSY. Not tested
> on real hardware.
> 
> Fixes: 070f9c658a59 ("net: ethernet: ti: cpsw: Push the request_irq function to the end of probe")
> Fixes: 0b8c878d1173 ("net: cpsw: Execute ndo_set_rx_mode callback in a work queue")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> v2:
>    - fold the two fixes into one patch (Jakub)
>    - share registered-port cleanup between probe unwind and remove (Jakub)
>    - check NETREG_REGISTERED instead of adding a local flag (Jakub)
>    - add the requested discovery context (netdev bot)
>    - rebase on net
> 
> v1: https://lore.kernel.org/netdev/cover.1791035789.git.kmehltretter@gmail.com/
> 
> Tested:
>    - ARM W=1 cpsw.o builds with TI_CPSW=y and TI_CPSW=m
>    - KASAN ARM runs with a QEMU CPSW probe stub. A test hook held
>      rx_mode_work pending while the TX IRQ request failed with -EBUSY.
>      Both failures reproduced without the combined fix; v2 completed
>      single- and dual-EMAC cleanup without KASAN or BUG reports in two
>      runs per mode.
> 
>   drivers/net/ethernet/ti/cpsw.c | 33 ++++++++++++++++++++-------------
>   1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index aa3531e844e87..01296c0582bf0 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1524,6 +1524,23 @@ static int cpsw_probe_dual_emac(struct cpsw_priv *priv)
>   	return ret;
>   }
>   
> +static void cpsw_unregister_ports(struct cpsw_common *cpsw)
> +{
> +	struct net_device *ndev;
> +	struct cpsw_priv *priv;
> +	int i;
> +
> +	for (i = 0; i < cpsw->data.slaves; i++) {
> +		ndev = cpsw->slaves[i].ndev;
> +		if (!ndev || ndev->reg_state != NETREG_REGISTERED)
> +			continue;
> +
> +		priv = netdev_priv(ndev);
> +		unregister_netdev(ndev);
> +		disable_work_sync(&priv->rx_mode_work);

Although the above code is a refactor, it appears to me that the correct 
sequence should be:
	disable_work_sync(&priv->rx_mode_work);
	unregister_netdev(ndev);
RX Mode work (cpsw_ndo_set_rx_mode_work) does have a 'netif_running' check 
so it might still be fine if the sequence isn't followed.

Please correct me if I am mistaken regarding the above. Rest of the patch 
looks good to me.

Regards,
Siddharth.

> +	}
> +}
> +
>   static const struct of_device_id cpsw_of_mtable[] = {
>   	{ .compatible = "ti,cpsw"},
>   	{ .compatible = "ti,am335x-cpsw"},
> @@ -1764,7 +1781,7 @@ static int cpsw_probe(struct platform_device *pdev)
>   	return 0;
>   
>   clean_unregister_netdev_ret:
> -	unregister_netdev(ndev);
> +	cpsw_unregister_ports(cpsw);
>   clean_cpts:
>   	cpts_release(cpsw->cpts);
>   	cpdma_ctlr_destroy(cpsw->dma);
> @@ -1779,9 +1796,7 @@ static int cpsw_probe(struct platform_device *pdev)
>   static void cpsw_remove(struct platform_device *pdev)
>   {
>   	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
> -	struct net_device *ndev;
> -	struct cpsw_priv *priv;
> -	int i, ret;
> +	int ret;
>   
>   	ret = pm_runtime_resume_and_get(&pdev->dev);
>   	if (ret < 0) {
> @@ -1793,15 +1808,7 @@ static void cpsw_remove(struct platform_device *pdev)
>   		return;
>   	}
>   
> -	for (i = 0; i < cpsw->data.slaves; i++) {
> -		ndev = cpsw->slaves[i].ndev;
> -		if (!ndev)
> -			continue;
> -
> -		priv = netdev_priv(ndev);
> -		unregister_netdev(ndev);
> -		disable_work_sync(&priv->rx_mode_work);
> -	}
> +	cpsw_unregister_ports(cpsw);
>   
>   	cpts_release(cpsw->cpts);
>   	cpdma_ctlr_destroy(cpsw->dma);
> 
> base-commit: 7ec48f396a1d89532c0537bc38b408b8c31ee37f