| Message ID | 20240927025301.1312590-2-nico@fluxnic.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: ethernet: ti: am65-cpsw: prevent WARN_ON upon module removal | expand |
On 9/27/24 04:53, 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 > 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@baylibre.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)
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; } 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); am65_cpts_release(common->cpts); err_of_clear: if (common->mdio_dev)