Re: [PATCH net v3 2/2] net: ethernet: ti: am65-cpsw: avoid devm_alloc_etherdev, fix module removal

[Roger Quadros <rogerq@kernel.org>]

Hi Nicolas,

On 04/10/2024 07:10, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre@baylibre.com>
> 
> Usage of devm_alloc_etherdev_mqs() conflicts with
> am65_cpsw_nuss_cleanup_ndev() as the same struct net_device instances
> get unregistered twice. Switch to alloc_etherdev_mqs() and make sure

Do we know why the same net device gets unregistered twice?

> am65_cpsw_nuss_cleanup_ndev() unregisters and frees those net_device
> instances properly.
> 
> With this, it is finally possible to rmmod the driver without oopsing
> the kernel.
> 
> Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
> Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index f6bc8a4dc6..e95457c988 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2744,10 +2744,9 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>  		return 0;
>  
>  	/* alloc netdev */
> -	port->ndev = devm_alloc_etherdev_mqs(common->dev,
> -					     sizeof(struct am65_cpsw_ndev_priv),
> -					     AM65_CPSW_MAX_QUEUES,
> -					     AM65_CPSW_MAX_QUEUES);
> +	port->ndev = alloc_etherdev_mqs(sizeof(struct am65_cpsw_ndev_priv),
> +					AM65_CPSW_MAX_QUEUES,
> +					AM65_CPSW_MAX_QUEUES);

Can we solve this issue without doing this change as
there are many error cases relying on devm managed freeing of netdev.


>  	if (!port->ndev) {
>  		dev_err(dev, "error allocating slave net_device %u\n",
>  			port->port_id);
> @@ -2868,8 +2867,12 @@ static void am65_cpsw_nuss_cleanup_ndev(struct am65_cpsw_common *common)
>  
>  	for (i = 0; i < common->port_num; i++) {
>  		port = &common->ports[i];
> -		if (port->ndev && port->ndev->reg_state == NETREG_REGISTERED)
> +		if (!port->ndev)
> +			continue;
> +		if (port->ndev->reg_state == NETREG_REGISTERED)
>  			unregister_netdev(port->ndev);
> +		free_netdev(port->ndev);
> +		port->ndev = NULL;

I still can't see what we are doing wrong in existing code.

>  	}
>  }
>  
> @@ -3613,16 +3616,17 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
>  
>  	ret = am65_cpsw_nuss_init_ndevs(common);
>  	if (ret)
> -		goto err_free_phylink;
> +		goto err_ndevs_clear;
>  
>  	ret = am65_cpsw_nuss_register_ndevs(common);
>  	if (ret)
> -		goto err_free_phylink;
> +		goto err_ndevs_clear;
>  
>  	pm_runtime_put(dev);
>  	return 0;
>  
> -err_free_phylink:
> +err_ndevs_clear:
> +	am65_cpsw_nuss_cleanup_ndev(common);
>  	am65_cpsw_nuss_phylink_cleanup(common);
>  	am65_cpts_release(common->cpts);
>  err_of_clear:

-- 
cheers,
-roger