[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a3a81b7a-71a8-451a-a16d-53d9c54d6e80@redhat.com>
Date: Thu, 3 Oct 2024 10:34:19 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Nicolas Pitre <nico@...xnic.net>, "David S. Miller" <davem@...emloft.net>
Cc: Nicolas Pitre <npitre@...libre.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] net: ethernet: ti: am65-cpsw: avoid
devm_alloc_etherdev, fix module removal
On 9/27/24 04:53, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre@...libre.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
> 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@...libre.com>
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)
Powered by blists - more mailing lists