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

[Paolo Abeni]

On 9/27/24 04:53, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre@xxxxxxxxxxxx>
>
> 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
> 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.
>
> Signed-off-by: Nicolas Pitre <npitre@xxxxxxxxxxxx>

This patch and the previous one looks like fixes for the 'net' tree: you 
should include the 'net' target into the subj prefix and include a 
suitable fixes tag in the tag area, see:

https://elixir.bootlin.com/linux/v6.12-rc1/source/Documentation/process/maintainer-netdev.rst#L67

> ---
>   drivers/net/ethernet/ti/am65-cpsw-nuss.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index f6bc8a4dc6..4cb1c187c6 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);
>   	if (!port->ndev) {
>   		dev_err(dev, "error allocating slave net_device %u\n",
>   			port->port_id);
> @@ -2858,7 +2857,7 @@ static int am65_cpsw_nuss_init_ndevs(struct am65_cpsw_common *common)
>   			return ret;
>   	}
>   
> -	return ret;
> +	return 0;

This chunk looks unrelated from the actual fix, please do not include in 
the next revision.

>   }
>   
>   static void am65_cpsw_nuss_cleanup_ndev(struct am65_cpsw_common *common)
> @@ -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;
>   	}
>   }
>   
> @@ -3624,6 +3627,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
>   
>   err_free_phylink:
>   	am65_cpsw_nuss_phylink_cleanup(common);
> +	am65_cpsw_nuss_cleanup_ndev(common);

The cleanup functions are called in the reverse order in 
am65_cpsw_nuss_remove(). Skimming over the code the actual order between 
these 2 does not matter (please double check this statement), but it 
would be better to be consistent.

Cheers,

Paolo

>   	am65_cpts_release(common->cpts);
>   err_of_clear:
>   	if (common->mdio_dev)