RE: [PATCH v2 2/7] net: fec: manage the netdev lifetime with devres

[Wei Fang]

> Subject: [PATCH v2 2/7] net: fec: manage the netdev lifetime with devres

Please add target tree to the subject. Since this is a fix, the target tree should be net.

> 
> fec_drv_remove() frees the netdev before devres releases the managed
> IRQs whose handlers use it as their data pointer. A late interrupt can
> therefore access the freed netdev.
> 
> Allocate the netdev with devres so that the later IRQ registrations are
> released first during teardown.
> 
> Fixes: 0d9b2ab1c376 ("fec: Use devm_request_irq()")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee..23e794a31ce8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5219,8 +5219,9 @@ fec_probe(struct platform_device *pdev)
>  	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
> 
>  	/* Init network device */
> -	ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private) +
> -				  FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
> +	ndev = devm_alloc_etherdev_mqs(&pdev->dev,
> +				       sizeof(struct fec_enet_private) +
> +				       FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
>  	if (!ndev)
>  		return -ENOMEM;
> 
> @@ -5480,8 +5481,6 @@ fec_probe(struct platform_device *pdev)
>  failed_phy:
>  	dev_id--;
>  failed_ioremap:

failed_ioremap is no longer needed, please remove it.

> -	free_netdev(ndev);
> -
>  	return ret;
>  }
> 
> @@ -5522,7 +5521,6 @@ fec_drv_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
> 
>  	fec_enet_deinit(ndev);
> -	free_netdev(ndev);
>  }
> 
>  static int fec_suspend(struct device *dev)
> --
> 2.34.1

This patch just prevents the netdev from being freed in fec_drv_remove(),
but fec_enet_interrupt() could still be called after the removal, and
fec_enet_collect_events() will be called to access the registers, however,
the ipg clk has been disabled, the registers are not accessible at that point,
that is a problem.

I think the hardware interrupts should be disabled on the removal path
and disable_irq() should be called to disable the irqs.

BTW, do not repost a new version within 24 hours.
https://elixir.bootlin.com/linux/v7.3-rc4/source/Documentation/process/maintainer-netdev.rst#L15

There are other upstream rules in maintainer-netdev.rst, please
refer to them.