[PATCH net-next 0/3] ezchip: Simplify some code

[PATCH net-next 0/3] ezchip: Simplify some code

[Christophe JAILLET]

Theses patches (at least 1 and 2) can be seen as an RFC for net MAINTAINERS
get see if they see 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 not worth it and MAINTAINERS' time can be focused on more
interesting topics than checking 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/20221221174043.1191996a@kernel.org/
[2]: https://elixir.bootlin.com/linux/v6.2-rc1/source/net/core/dev.c#L10710


Christophe JAILLET (3):
  ezchip: Remove some redundant clean-up functions
  ezchip: Switch to some devm_ function to simplify code
  ezchip: Further clean-up

 drivers/net/ethernet/ezchip/nps_enet.c | 47 ++++++--------------------
 1 file changed, 10 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH net-next 1/3] ezchip: Remove some redundant clean-up functions

[Christophe JAILLET]

free_netdev() already calls netif_napi_del(), no need to call it
explicitly.
It's harmless, but useless.

Remove the call in the error handling path of the probe and in the remove
function.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/net/ethernet/ezchip/nps_enet.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c
index f1eb660aaee2..6389c6b5005c 100644
--- a/drivers/net/ethernet/ezchip/nps_enet.c
+++ b/drivers/net/ethernet/ezchip/nps_enet.c
@@ -627,7 +627,6 @@ static s32 nps_enet_probe(struct platform_device *pdev)
 	return 0;
 
 out_netif_api:
-	netif_napi_del(&priv->napi);
 out_netdev:
 	free_netdev(ndev);
 
@@ -640,7 +639,6 @@ static s32 nps_enet_remove(struct platform_device *pdev)
 	struct nps_enet_priv *priv = netdev_priv(ndev);
 
 	unregister_netdev(ndev);
-	netif_napi_del(&priv->napi);
 	free_netdev(ndev);
 
 	return 0;
-- 
2.34.1

Re: [PATCH net-next 1/3] ezchip: Remove some redundant clean-up functions

[Jakub Kicinski]

On Wed,  4 Jan 2023 22:05:32 +0100 Christophe JAILLET wrote:
> @@ -640,7 +639,6 @@ static s32 nps_enet_remove(struct platform_device *pdev)
>  	struct nps_enet_priv *priv = netdev_priv(ndev);
>  
>  	unregister_netdev(ndev);
> -	netif_napi_del(&priv->napi);
>  	free_netdev(ndev);

This adds an unused variable warning, which is fixed by the next patch.
Could you remove the @priv variable here already?

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

[Christophe JAILLET]

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 <christophe.jaillet@wanadoo.fr>
---
 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

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

[Jakub Kicinski]

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?

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

[Christophe JAILLET]

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

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

[Leon Romanovsky]

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

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

[Jakub Kicinski]

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 net-next 3/3] ezchip: Further clean-up

[Christophe JAILLET]

Now that is only call to platform_get_drvdata() is removed, calling
platform_set_drvdata() looks useless. Remove it.

While at it, remove a useless 'err' initialization and change its type to a
more common 'int'

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/net/ethernet/ezchip/nps_enet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c
index 21e230150104..139510d9669f 100644
--- a/drivers/net/ethernet/ezchip/nps_enet.c
+++ b/drivers/net/ethernet/ezchip/nps_enet.c
@@ -574,7 +574,7 @@ static s32 nps_enet_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct net_device *ndev;
 	struct nps_enet_priv *priv;
-	s32 err = 0;
+	int err;
 
 	if (!dev->of_node)
 		return -ENODEV;
@@ -583,7 +583,6 @@ static s32 nps_enet_probe(struct platform_device *pdev)
 	if (!ndev)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, ndev);
 	SET_NETDEV_DEV(ndev, dev);
 	priv = netdev_priv(ndev);
 
-- 
2.34.1