[net-next,2/3] ezchip: Switch to some devm_ function to simplify code

Commit Message

Christophe JAILLET Jan. 4, 2023, 9:05 p.m. UTC

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 <[e-mail]>
---
 drivers/net/ethernet/ezchip/nps_enet.c | 42 ++++++--------------------
 1 file changed, 9 insertions(+), 33 deletions(-)

Comments

Jakub Kicinski Jan. 5, 2023, 4:54 a.m. UTC | #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?

Christophe JAILLET Jan. 5, 2023, 6:27 a.m. UTC | #2

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/[e-mail]/
[2]: https://elixir.bootlin.com/linux/v6.2-rc1/source/net/core/dev.c#L10710

Leon Romanovsky Jan. 5, 2023, 8:01 a.m. UTC | #3

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/[e-mail]/
> [2]: https://elixir.bootlin.com/linux/v6.2-rc1/source/net/core/dev.c#L10710

Jakub Kicinski Jan. 5, 2023, 6:31 p.m. UTC | #4

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.

Patch

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,