devm_alloc_etherdev() and devm_register_netdev() can be used to simplify
code.
Now the error handling path of the probe and the remove function are
useless and can be removed completely.
Signed-off-by: Christophe JAILLET <[email protected]>
---
drivers/net/ethernet/ezchip/nps_enet.c | 42 ++++++--------------------
1 file changed, 9 insertions(+), 33 deletions(-)
diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c
index 6389c6b5005c..21e230150104 100644
--- a/drivers/net/ethernet/ezchip/nps_enet.c
+++ b/drivers/net/ethernet/ezchip/nps_enet.c
@@ -579,7 +579,7 @@ static s32 nps_enet_probe(struct platform_device *pdev)
if (!dev->of_node)
return -ENODEV;
- ndev = alloc_etherdev(sizeof(struct nps_enet_priv));
+ ndev = devm_alloc_etherdev(dev, sizeof(struct nps_enet_priv));
if (!ndev)
return -ENOMEM;
@@ -594,10 +594,8 @@ static s32 nps_enet_probe(struct platform_device *pdev)
ndev->flags &= ~IFF_MULTICAST;
priv->regs_base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(priv->regs_base)) {
- err = PTR_ERR(priv->regs_base);
- goto out_netdev;
- }
+ if (IS_ERR(priv->regs_base))
+ return PTR_ERR(priv->regs_base);
dev_dbg(dev, "Registers base address is 0x%p\n", priv->regs_base);
/* set kernel MAC address to dev */
@@ -607,41 +605,20 @@ static s32 nps_enet_probe(struct platform_device *pdev)
/* Get IRQ number */
priv->irq = platform_get_irq(pdev, 0);
- if (priv->irq < 0) {
- err = -ENODEV;
- goto out_netdev;
- }
+ if (priv->irq < 0)
+ return -ENODEV;
netif_napi_add_weight(ndev, &priv->napi, nps_enet_poll,
NPS_ENET_NAPI_POLL_WEIGHT);
/* Register the driver. Should be the last thing in probe */
- err = register_netdev(ndev);
- if (err) {
- dev_err(dev, "Failed to register ndev for %s, err = 0x%08x\n",
- ndev->name, (s32)err);
- goto out_netif_api;
- }
+ err = devm_register_netdev(dev, ndev);
+ if (err)
+ return dev_err_probe(dev, err, "Failed to register ndev for %s\n",
+ ndev->name);
dev_info(dev, "(rx/tx=%d)\n", priv->irq);
return 0;
-
-out_netif_api:
-out_netdev:
- free_netdev(ndev);
-
- return err;
-}
-
-static s32 nps_enet_remove(struct platform_device *pdev)
-{
- struct net_device *ndev = platform_get_drvdata(pdev);
- struct nps_enet_priv *priv = netdev_priv(ndev);
-
- unregister_netdev(ndev);
- free_netdev(ndev);
-
- return 0;
}
static const struct of_device_id nps_enet_dt_ids[] = {
@@ -652,7 +629,6 @@ MODULE_DEVICE_TABLE(of, nps_enet_dt_ids);
static struct platform_driver nps_enet_driver = {
.probe = nps_enet_probe,
- .remove = nps_enet_remove,
.driver = {
.name = DRV_NAME,
.of_match_table = nps_enet_dt_ids,
--
2.34.1
On Wed, 4 Jan 2023 22:05:33 +0100 Christophe JAILLET wrote:
> devm_alloc_etherdev() and devm_register_netdev() can be used to simplify
> code.
>
> Now the error handling path of the probe and the remove function are
> useless and can be removed completely.
Right, but this is very likely a dead driver. Why invest in refactoring?
Le 05/01/2023 à 05:54, Jakub Kicinski a écrit :
> On Wed, 4 Jan 2023 22:05:33 +0100 Christophe JAILLET wrote:
>> devm_alloc_etherdev() and devm_register_netdev() can be used to simplify
>> code.
>>
>> Now the error handling path of the probe and the remove function are
>> useless and can be removed completely.
>
> Right, but this is very likely a dead driver. Why invest in refactoring?
>
Hi Jakub,
this driver was just randomly picked as an example.
My main point is in the cover letter. I look for feed-back to know if
patches like that are welcomed. Only the first, Only the second, Both or
None.
I put it here, slightly rephrased:
These patches (at least 1 and 2) can be seen as an RFC for net
MAINTAINERS, to see if there is any interest in:
- axing useless netif_napi_del() calls, when free_netdev() is called
just after. (patch 1)
- simplifying code with axing the error handling path of the probe
and the remove function in favor of using devm_ functions (patch 2)
or
if it doesn't worth it and would only waste MAINTAINERS' time to review
what is in fact only code clean-ups.
The rational for patch 1 is based on Jakub's comment [1].
free_netdev() already cleans up NAPIs (see [2]).
CJ
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://elixir.bootlin.com/linux/v6.2-rc1/source/net/core/dev.c#L10710
On Thu, Jan 05, 2023 at 07:27:00AM +0100, Christophe JAILLET wrote:
> Le 05/01/2023 ? 05:54, Jakub Kicinski a ?crit?:
> > On Wed, 4 Jan 2023 22:05:33 +0100 Christophe JAILLET wrote:
> > > devm_alloc_etherdev() and devm_register_netdev() can be used to simplify
> > > code.
> > >
> > > Now the error handling path of the probe and the remove function are
> > > useless and can be removed completely.
> >
> > Right, but this is very likely a dead driver. Why invest in refactoring?
> >
>
> Hi Jakub,
>
> this driver was just randomly picked as an example.
>
> My main point is in the cover letter. I look for feed-back to know if
> patches like that are welcomed. Only the first, Only the second, Both or
> None.
>
>
> I put it here, slightly rephrased:
>
>
> These patches (at least 1 and 2) can be seen as an RFC for net MAINTAINERS,
> to see if there is any interest in:
> - axing useless netif_napi_del() calls, when free_netdev() is called just
> after. (patch 1)
> - simplifying code with axing the error handling path of the probe and the
> remove function in favor of using devm_ functions (patch 2)
I would say no. In many occasions, the devm_* calls were marked as harmful.
Latest talk about devm_kzalloc(): https://lpc.events/event/16/contributions/1227/
Thanks
>
> or
>
> if it doesn't worth it and would only waste MAINTAINERS' time to review what
> is in fact only code clean-ups.
>
>
> The rational for patch 1 is based on Jakub's comment [1].
> free_netdev() already cleans up NAPIs (see [2]).
>
> CJ
>
> [1]: https://lore.kernel.org/all/[email protected]/
> [2]: https://elixir.bootlin.com/linux/v6.2-rc1/source/net/core/dev.c#L10710
On Thu, 5 Jan 2023 07:27:00 +0100 Christophe JAILLET wrote:
> My main point is in the cover letter. I look for feed-back to know if
> patches like that are welcomed. Only the first, Only the second, Both or
> None.
Sorry, missed that.
> These patches (at least 1 and 2) can be seen as an RFC for net
> MAINTAINERS, to see if there is any interest in:
> - axing useless netif_napi_del() calls, when free_netdev() is called
> just after. (patch 1)
I think it'd be too much noise. I'd vote no.
> - simplifying code with axing the error handling path of the probe
> and the remove function in favor of using devm_ functions (patch 2)
I believe DaveM was historically opposed to those helpers in general.
I think we should avoid pure conversions, unless they are part of
development of new features or fix bugs.