[PATCH net-next 0/6] gianfar cleanups

[PATCH net-next 0/6] gianfar cleanups

[Rosen Penev]

Mostly devm adjustments and bugfixes along the way.

Rosen Penev (6):
  net: gianfar: use devm_alloc_etherdev_mqs
  net: gianfar: remove free_gfar_dev
  net: gianfar: allocate queues with devm
  net: gianfar: use devm for register_netdev
  net: gianfar: use devm for request_irq
  net: gianfar: use devm for of_iomap

 drivers/net/ethernet/freescale/gianfar.c | 158 +++++------------------
 1 file changed, 35 insertions(+), 123 deletions(-)

-- 
2.46.2

[PATCH net-next 1/6] net: gianfar: use devm_alloc_etherdev_mqs

[Rosen Penev]

There seems to be a mistake here. There's a num_rx_qs variable that is not
being passed to the allocation function. The mq variant just calls mqs
with the last parameter of the former duplicated to the last parameter
of the latter. That's fine if they match. Not sure they do.

Also avoids manual free_netdev

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index ecb1703ea150..b0f65cdf4872 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -475,8 +475,6 @@ static void free_gfar_dev(struct gfar_private *priv)
 			kfree(priv->gfargrp[i].irqinfo[j]);
 			priv->gfargrp[i].irqinfo[j] = NULL;
 		}
-
-	free_netdev(priv->ndev);
 }
 
 static void disable_napi(struct gfar_private *priv)
@@ -681,7 +679,8 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 		return -EINVAL;
 	}
 
-	*pdev = alloc_etherdev_mq(sizeof(*priv), num_tx_qs);
+	*pdev = devm_alloc_etherdev_mqs(&ofdev->dev, sizeof(*priv), num_tx_qs,
+					num_rx_qs);
 	dev = *pdev;
 	if (NULL == dev)
 		return -ENOMEM;
-- 
2.46.2

Re: [PATCH net-next 1/6] net: gianfar: use devm_alloc_etherdev_mqs

[Maxime Chevallier]

On Tue,  1 Oct 2024 14:21:59 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> There seems to be a mistake here. There's a num_rx_qs variable that is not
> being passed to the allocation function. The mq variant just calls mqs
> with the last parameter of the former duplicated to the last parameter
> of the latter. That's fine if they match. Not sure they do.

As far as I can tell, both queue numbers used during the netdev alloc
are always the same, so this looks good to me.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

[PATCH net-next 2/6] net: gianfar: remove free_gfar_dev

[Rosen Penev]

Can be completely removed with devm.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index b0f65cdf4872..9f0824f0b2d1 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -466,17 +466,6 @@ static void unmap_group_regs(struct gfar_private *priv)
 			iounmap(priv->gfargrp[i].regs);
 }
 
-static void free_gfar_dev(struct gfar_private *priv)
-{
-	int i, j;
-
-	for (i = 0; i < priv->num_grps; i++)
-		for (j = 0; j < GFAR_NUM_IRQS; j++) {
-			kfree(priv->gfargrp[i].irqinfo[j]);
-			priv->gfargrp[i].irqinfo[j] = NULL;
-		}
-}
-
 static void disable_napi(struct gfar_private *priv)
 {
 	int i;
@@ -504,8 +493,8 @@ static int gfar_parse_group(struct device_node *np,
 	int i;
 
 	for (i = 0; i < GFAR_NUM_IRQS; i++) {
-		grp->irqinfo[i] = kzalloc(sizeof(struct gfar_irqinfo),
-					  GFP_KERNEL);
+		grp->irqinfo[i] = devm_kzalloc(
+			priv->dev, sizeof(struct gfar_irqinfo), GFP_KERNEL);
 		if (!grp->irqinfo[i])
 			return -ENOMEM;
 	}
@@ -818,7 +807,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 	gfar_free_rx_queues(priv);
 tx_alloc_failed:
 	gfar_free_tx_queues(priv);
-	free_gfar_dev(priv);
 	return err;
 }
 
@@ -3361,7 +3349,6 @@ static int gfar_probe(struct platform_device *ofdev)
 	gfar_free_tx_queues(priv);
 	of_node_put(priv->phy_node);
 	of_node_put(priv->tbi_node);
-	free_gfar_dev(priv);
 	return err;
 }
 
@@ -3381,7 +3368,6 @@ static void gfar_remove(struct platform_device *ofdev)
 	unmap_group_regs(priv);
 	gfar_free_rx_queues(priv);
 	gfar_free_tx_queues(priv);
-	free_gfar_dev(priv);
 }
 
 #ifdef CONFIG_PM
-- 
2.46.2

Re: [PATCH net-next 2/6] net: gianfar: remove free_gfar_dev

[Maxime Chevallier]

On Tue,  1 Oct 2024 14:22:00 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> Can be completely removed with devm.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

This looks correct to me,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

[PATCH net-next 3/6] net: gianfar: allocate queues with devm

[Rosen Penev]

There seems to be a mistake here where free_tx_queue is called on
failure. Just let devm deal with it.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 38 +++++-------------------
 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 9f0824f0b2d1..66818d63cced 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -412,8 +412,8 @@ static int gfar_alloc_tx_queues(struct gfar_private *priv)
 	int i;
 
 	for (i = 0; i < priv->num_tx_queues; i++) {
-		priv->tx_queue[i] = kzalloc(sizeof(struct gfar_priv_tx_q),
-					    GFP_KERNEL);
+		priv->tx_queue[i] = devm_kzalloc(
+			priv->dev, sizeof(struct gfar_priv_tx_q), GFP_KERNEL);
 		if (!priv->tx_queue[i])
 			return -ENOMEM;
 
@@ -430,8 +430,8 @@ static int gfar_alloc_rx_queues(struct gfar_private *priv)
 	int i;
 
 	for (i = 0; i < priv->num_rx_queues; i++) {
-		priv->rx_queue[i] = kzalloc(sizeof(struct gfar_priv_rx_q),
-					    GFP_KERNEL);
+		priv->rx_queue[i] = devm_kzalloc(
+			priv->dev, sizeof(struct gfar_priv_rx_q), GFP_KERNEL);
 		if (!priv->rx_queue[i])
 			return -ENOMEM;
 
@@ -441,22 +441,6 @@ static int gfar_alloc_rx_queues(struct gfar_private *priv)
 	return 0;
 }
 
-static void gfar_free_tx_queues(struct gfar_private *priv)
-{
-	int i;
-
-	for (i = 0; i < priv->num_tx_queues; i++)
-		kfree(priv->tx_queue[i]);
-}
-
-static void gfar_free_rx_queues(struct gfar_private *priv)
-{
-	int i;
-
-	for (i = 0; i < priv->num_rx_queues; i++)
-		kfree(priv->rx_queue[i]);
-}
-
 static void unmap_group_regs(struct gfar_private *priv)
 {
 	int i;
@@ -685,16 +669,16 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 
 	err = gfar_alloc_tx_queues(priv);
 	if (err)
-		goto tx_alloc_failed;
+		return err;
 
 	err = gfar_alloc_rx_queues(priv);
 	if (err)
-		goto rx_alloc_failed;
+		return err;
 
 	err = of_property_read_string(np, "model", &model);
 	if (err) {
 		pr_err("Device model property missing, aborting\n");
-		goto rx_alloc_failed;
+		return err;
 	}
 
 	/* Init Rx queue filer rule set linked list */
@@ -803,10 +787,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 
 err_grp_init:
 	unmap_group_regs(priv);
-rx_alloc_failed:
-	gfar_free_rx_queues(priv);
-tx_alloc_failed:
-	gfar_free_tx_queues(priv);
 	return err;
 }
 
@@ -3345,8 +3325,6 @@ static int gfar_probe(struct platform_device *ofdev)
 	if (of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
 	unmap_group_regs(priv);
-	gfar_free_rx_queues(priv);
-	gfar_free_tx_queues(priv);
 	of_node_put(priv->phy_node);
 	of_node_put(priv->tbi_node);
 	return err;
@@ -3366,8 +3344,6 @@ static void gfar_remove(struct platform_device *ofdev)
 		of_phy_deregister_fixed_link(np);
 
 	unmap_group_regs(priv);
-	gfar_free_rx_queues(priv);
-	gfar_free_tx_queues(priv);
 }
 
 #ifdef CONFIG_PM
-- 
2.46.2

Re: [PATCH net-next 3/6] net: gianfar: allocate queues with devm

[Maxime Chevallier]

On Tue,  1 Oct 2024 14:22:01 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> There seems to be a mistake here where free_tx_queue is called on
> failure. Just let devm deal with it.

Good catch, this looks good to me.

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

RE: [PATCH net-next 3/6] net: gianfar: allocate queues with devm

[Claudiu Manoil]

> -----Original Message-----
> From: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Sent: Wednesday, October 2, 2024 10:25 AM
[...]
> 
> On Tue,  1 Oct 2024 14:22:01 -0700
> Rosen Penev <rosenp@gmail.com> wrote:
> 
> > There seems to be a mistake here where free_tx_queue is called on
> > failure. Just let devm deal with it.
> 
> Good catch, this looks good to me.
> 

I like your enthusiasm, but there's nothing to catch here.
kfree() does nothing to NULL objects, however the 'constructor' allocates an
array of objects so free_tx_queues() has to iterate over all objects, to free those
allocated before failure.

I don't have a strong opinion regarding the usage of devm_*() API to allocate resources
at device probing time, it saves some lines of code. However I see this as bringing limited
benefits for simple cases like device probe()/remove(), especially when converting old drivers
like this one. And there's also the risk of falling into the trap of thinking that devm_*() takes
care of everything.

-Claudiu

[PATCH net-next 4/6] net: gianfar: use devm for register_netdev

[Rosen Penev]

Avoids manual unregister netdev.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 66818d63cced..07936dccc389 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -3272,7 +3272,7 @@ static int gfar_probe(struct platform_device *ofdev)
 	/* Carrier starts down, phylib will bring it up */
 	netif_carrier_off(dev);
 
-	err = register_netdev(dev);
+	err = devm_register_netdev(&ofdev->dev, dev);
 
 	if (err) {
 		pr_err("%s: Cannot register net device, aborting\n", dev->name);
@@ -3338,8 +3338,6 @@ static void gfar_remove(struct platform_device *ofdev)
 	of_node_put(priv->phy_node);
 	of_node_put(priv->tbi_node);
 
-	unregister_netdev(priv->ndev);
-
 	if (of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
 
-- 
2.46.2

Re: [PATCH net-next 4/6] net: gianfar: use devm for register_netdev

[Maxime Chevallier]

Hi,

On Tue,  1 Oct 2024 14:22:02 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> Avoids manual unregister netdev.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 66818d63cced..07936dccc389 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -3272,7 +3272,7 @@ static int gfar_probe(struct platform_device *ofdev)
>  	/* Carrier starts down, phylib will bring it up */
>  	netif_carrier_off(dev);
>  
> -	err = register_netdev(dev);
> +	err = devm_register_netdev(&ofdev->dev, dev);

I wonder if this is not a good opportunity to also move the
registration at the end of this function. Here, the netdev is
registered but some configuration is still being done afterwards, such
as WoL init and internal filter configuration.

There's the ever so slightly chance that traffic can start flowing
before these filters are configured, which could lead to unexpected
side effects. We usually register the netdev as a very last step, once
all initial configuration is done and the device is ready to be used.

As you're doing some cleanup on the registration code itself, it seems
like a good opportunity to change that.

Thanks,

Maxime

Re: [PATCH net-next 4/6] net: gianfar: use devm for register_netdev

[Rosen Penev]

On Wed, Oct 2, 2024 at 12:29 AM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> Hi,
>
> On Tue,  1 Oct 2024 14:22:02 -0700
> Rosen Penev <rosenp@gmail.com> wrote:
>
> > Avoids manual unregister netdev.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/freescale/gianfar.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > index 66818d63cced..07936dccc389 100644
> > --- a/drivers/net/ethernet/freescale/gianfar.c
> > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > @@ -3272,7 +3272,7 @@ static int gfar_probe(struct platform_device *ofdev)
> >       /* Carrier starts down, phylib will bring it up */
> >       netif_carrier_off(dev);
> >
> > -     err = register_netdev(dev);
> > +     err = devm_register_netdev(&ofdev->dev, dev);
>
> I wonder if this is not a good opportunity to also move the
> registration at the end of this function. Here, the netdev is
> registered but some configuration is still being done afterwards, such
> as WoL init and internal filter configuration.
>
> There's the ever so slightly chance that traffic can start flowing
> before these filters are configured, which could lead to unexpected
> side effects. We usually register the netdev as a very last step, once
> all initial configuration is done and the device is ready to be used.
>
> As you're doing some cleanup on the registration code itself, it seems
> like a good opportunity to change that.
There seem to be a bunch of netdev_info calls. I assume those need a
registered netdev.

Additionally, the irqs are allocated in _open instead of _probe. I
assume those would need to be moved.
>
> Thanks,
>
> Maxime
>
>

[PATCH net-next 5/6] net: gianfar: use devm for request_irq

[Rosen Penev]

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 67 +++++++-----------------
 1 file changed, 18 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 07936dccc389..78fdab3c6f77 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2769,13 +2769,6 @@ static void gfar_netpoll(struct net_device *dev)
 }
 #endif
 
-static void free_grp_irqs(struct gfar_priv_grp *grp)
-{
-	free_irq(gfar_irq(grp, TX)->irq, grp);
-	free_irq(gfar_irq(grp, RX)->irq, grp);
-	free_irq(gfar_irq(grp, ER)->irq, grp);
-}
-
 static int register_grp_irqs(struct gfar_priv_grp *grp)
 {
 	struct gfar_private *priv = grp->priv;
@@ -2789,80 +2782,58 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
 		/* Install our interrupt handlers for Error,
 		 * Transmit, and Receive
 		 */
-		err = request_irq(gfar_irq(grp, ER)->irq, gfar_error, 0,
-				  gfar_irq(grp, ER)->name, grp);
+		err = devm_request_irq(priv->dev, gfar_irq(grp, ER)->irq,
+				       gfar_error, 0, gfar_irq(grp, ER)->name,
+				       grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",
 				  gfar_irq(grp, ER)->irq);
 
-			goto err_irq_fail;
+			return err;
 		}
 		enable_irq_wake(gfar_irq(grp, ER)->irq);
 
-		err = request_irq(gfar_irq(grp, TX)->irq, gfar_transmit, 0,
-				  gfar_irq(grp, TX)->name, grp);
+		err = devm_request_irq(priv->dev, gfar_irq(grp, TX)->irq,
+				       gfar_transmit, 0,
+				       gfar_irq(grp, TX)->name, grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",
 				  gfar_irq(grp, TX)->irq);
-			goto tx_irq_fail;
+			return err;
 		}
-		err = request_irq(gfar_irq(grp, RX)->irq, gfar_receive, 0,
-				  gfar_irq(grp, RX)->name, grp);
+		err = devm_request_irq(priv->dev, gfar_irq(grp, RX)->irq,
+				       gfar_receive, 0, gfar_irq(grp, RX)->name,
+				       grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",
 				  gfar_irq(grp, RX)->irq);
-			goto rx_irq_fail;
+			return err;
 		}
 		enable_irq_wake(gfar_irq(grp, RX)->irq);
 
 	} else {
-		err = request_irq(gfar_irq(grp, TX)->irq, gfar_interrupt, 0,
-				  gfar_irq(grp, TX)->name, grp);
+		err = devm_request_irq(priv->dev, gfar_irq(grp, TX)->irq,
+				       gfar_interrupt, 0,
+				       gfar_irq(grp, TX)->name, grp);
 		if (err < 0) {
 			netif_err(priv, intr, dev, "Can't get IRQ %d\n",
 				  gfar_irq(grp, TX)->irq);
-			goto err_irq_fail;
+			return err;
 		}
 		enable_irq_wake(gfar_irq(grp, TX)->irq);
 	}
 
 	return 0;
-
-rx_irq_fail:
-	free_irq(gfar_irq(grp, TX)->irq, grp);
-tx_irq_fail:
-	free_irq(gfar_irq(grp, ER)->irq, grp);
-err_irq_fail:
-	return err;
-
-}
-
-static void gfar_free_irq(struct gfar_private *priv)
-{
-	int i;
-
-	/* Free the IRQs */
-	if (priv->device_flags & FSL_GIANFAR_DEV_HAS_MULTI_INTR) {
-		for (i = 0; i < priv->num_grps; i++)
-			free_grp_irqs(&priv->gfargrp[i]);
-	} else {
-		for (i = 0; i < priv->num_grps; i++)
-			free_irq(gfar_irq(&priv->gfargrp[i], TX)->irq,
-				 &priv->gfargrp[i]);
-	}
 }
 
 static int gfar_request_irq(struct gfar_private *priv)
 {
-	int err, i, j;
+	int err, i;
 
 	for (i = 0; i < priv->num_grps; i++) {
 		err = register_grp_irqs(&priv->gfargrp[i]);
-		if (err) {
-			for (j = 0; j < i; j++)
-				free_grp_irqs(&priv->gfargrp[j]);
+		if (err)
 			return err;
-		}
 	}
 
 	return 0;
@@ -2902,8 +2873,6 @@ static int gfar_close(struct net_device *dev)
 	/* Disconnect from the PHY */
 	phy_disconnect(dev->phydev);
 
-	gfar_free_irq(priv);
-
 	return 0;
 }
 
-- 
2.46.2

Re: [PATCH net-next 5/6] net: gianfar: use devm for request_irq

[Maxime Chevallier]

Hi Rosen,

On Tue,  1 Oct 2024 14:22:03 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/freescale/gianfar.c | 67 +++++++-----------------
>  1 file changed, 18 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 07936dccc389..78fdab3c6f77 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2769,13 +2769,6 @@ static void gfar_netpoll(struct net_device *dev)
>  }
>  #endif
>  
> -static void free_grp_irqs(struct gfar_priv_grp *grp)
> -{
> -	free_irq(gfar_irq(grp, TX)->irq, grp);
> -	free_irq(gfar_irq(grp, RX)->irq, grp);
> -	free_irq(gfar_irq(grp, ER)->irq, grp);
> -}
> -
>  static int register_grp_irqs(struct gfar_priv_grp *grp)
>  {
>  	struct gfar_private *priv = grp->priv;
> @@ -2789,80 +2782,58 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
>  		/* Install our interrupt handlers for Error,
>  		 * Transmit, and Receive
>  		 */
> -		err = request_irq(gfar_irq(grp, ER)->irq, gfar_error, 0,
> -				  gfar_irq(grp, ER)->name, grp);
> +		err = devm_request_irq(priv->dev, gfar_irq(grp, ER)->irq,
> +				       gfar_error, 0, gfar_irq(grp, ER)->name,
> +				       grp);

This is called during open/close, so the lifetime of the irqs
isn't tied to the struct device, devm won't apply here. If you
open/close/re-open the device, you'll request the same irq multiple
times.

Maxime

Re: [PATCH net-next 5/6] net: gianfar: use devm for request_irq

[Rosen Penev]

On Wed, Oct 2, 2024 at 12:37 AM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> Hi Rosen,
>
> On Tue,  1 Oct 2024 14:22:03 -0700
> Rosen Penev <rosenp@gmail.com> wrote:
>
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/freescale/gianfar.c | 67 +++++++-----------------
> >  1 file changed, 18 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > index 07936dccc389..78fdab3c6f77 100644
> > --- a/drivers/net/ethernet/freescale/gianfar.c
> > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > @@ -2769,13 +2769,6 @@ static void gfar_netpoll(struct net_device *dev)
> >  }
> >  #endif
> >
> > -static void free_grp_irqs(struct gfar_priv_grp *grp)
> > -{
> > -     free_irq(gfar_irq(grp, TX)->irq, grp);
> > -     free_irq(gfar_irq(grp, RX)->irq, grp);
> > -     free_irq(gfar_irq(grp, ER)->irq, grp);
> > -}
> > -
> >  static int register_grp_irqs(struct gfar_priv_grp *grp)
> >  {
> >       struct gfar_private *priv = grp->priv;
> > @@ -2789,80 +2782,58 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
> >               /* Install our interrupt handlers for Error,
> >                * Transmit, and Receive
> >                */
> > -             err = request_irq(gfar_irq(grp, ER)->irq, gfar_error, 0,
> > -                               gfar_irq(grp, ER)->name, grp);
> > +             err = devm_request_irq(priv->dev, gfar_irq(grp, ER)->irq,
> > +                                    gfar_error, 0, gfar_irq(grp, ER)->name,
> > +                                    grp);
>
> This is called during open/close, so the lifetime of the irqs
> isn't tied to the struct device, devm won't apply here. If you
> open/close/re-open the device, you'll request the same irq multiple
> times.
Good point. Would it make sense to move to probe?
> Maxime

Re: [PATCH net-next 5/6] net: gianfar: use devm for request_irq

[Andrew Lunn]

On Wed, Oct 02, 2024 at 12:29:04PM -0700, Rosen Penev wrote:
> On Wed, Oct 2, 2024 at 12:37 AM Maxime Chevallier
> <maxime.chevallier@bootlin.com> wrote:
> >
> > Hi Rosen,
> >
> > On Tue,  1 Oct 2024 14:22:03 -0700
> > Rosen Penev <rosenp@gmail.com> wrote:
> >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  drivers/net/ethernet/freescale/gianfar.c | 67 +++++++-----------------
> > >  1 file changed, 18 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > > index 07936dccc389..78fdab3c6f77 100644
> > > --- a/drivers/net/ethernet/freescale/gianfar.c
> > > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > > @@ -2769,13 +2769,6 @@ static void gfar_netpoll(struct net_device *dev)
> > >  }
> > >  #endif
> > >
> > > -static void free_grp_irqs(struct gfar_priv_grp *grp)
> > > -{
> > > -     free_irq(gfar_irq(grp, TX)->irq, grp);
> > > -     free_irq(gfar_irq(grp, RX)->irq, grp);
> > > -     free_irq(gfar_irq(grp, ER)->irq, grp);
> > > -}
> > > -
> > >  static int register_grp_irqs(struct gfar_priv_grp *grp)
> > >  {
> > >       struct gfar_private *priv = grp->priv;
> > > @@ -2789,80 +2782,58 @@ static int register_grp_irqs(struct gfar_priv_grp *grp)
> > >               /* Install our interrupt handlers for Error,
> > >                * Transmit, and Receive
> > >                */
> > > -             err = request_irq(gfar_irq(grp, ER)->irq, gfar_error, 0,
> > > -                               gfar_irq(grp, ER)->name, grp);
> > > +             err = devm_request_irq(priv->dev, gfar_irq(grp, ER)->irq,
> > > +                                    gfar_error, 0, gfar_irq(grp, ER)->name,
> > > +                                    grp);
> >
> > This is called during open/close, so the lifetime of the irqs
> > isn't tied to the struct device, devm won't apply here. If you
> > open/close/re-open the device, you'll request the same irq multiple
> > times.
> Good point. Would it make sense to move to probe?

Do you have the hardware? Can you test such a change?

	Andrew

RE: [PATCH net-next 5/6] net: gianfar: use devm for request_irq

[Claudiu Manoil]

> -----Original Message-----
> From: Rosen Penev <rosenp@gmail.com>
> Sent: Wednesday, October 2, 2024 10:29 PM
> To: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Cc: netdev@vger.kernel.org; andrew@lunn.ch; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; linux-
> kernel@vger.kernel.org; Claudiu Manoil <claudiu.manoil@nxp.com>
> Subject: Re: [PATCH net-next 5/6] net: gianfar: use devm for request_irq
> 
> On Wed, Oct 2, 2024 at 12:37 AM Maxime Chevallier
> <maxime.chevallier@bootlin.com> wrote:
> >
> > Hi Rosen,
> >
> > On Tue,  1 Oct 2024 14:22:03 -0700
> > Rosen Penev <rosenp@gmail.com> wrote:
> >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  drivers/net/ethernet/freescale/gianfar.c | 67
> > > +++++++-----------------
> > >  1 file changed, 18 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/freescale/gianfar.c
> > > b/drivers/net/ethernet/freescale/gianfar.c
> > > index 07936dccc389..78fdab3c6f77 100644
> > > --- a/drivers/net/ethernet/freescale/gianfar.c
> > > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > > @@ -2769,13 +2769,6 @@ static void gfar_netpoll(struct net_device
> > > *dev)  }  #endif
> > >
> > > -static void free_grp_irqs(struct gfar_priv_grp *grp) -{
> > > -     free_irq(gfar_irq(grp, TX)->irq, grp);
> > > -     free_irq(gfar_irq(grp, RX)->irq, grp);
> > > -     free_irq(gfar_irq(grp, ER)->irq, grp);
> > > -}
> > > -
> > >  static int register_grp_irqs(struct gfar_priv_grp *grp)  {
> > >       struct gfar_private *priv = grp->priv; @@ -2789,80 +2782,58 @@
> > > static int register_grp_irqs(struct gfar_priv_grp *grp)
> > >               /* Install our interrupt handlers for Error,
> > >                * Transmit, and Receive
> > >                */
> > > -             err = request_irq(gfar_irq(grp, ER)->irq, gfar_error, 0,
> > > -                               gfar_irq(grp, ER)->name, grp);
> > > +             err = devm_request_irq(priv->dev, gfar_irq(grp, ER)->irq,
> > > +                                    gfar_error, 0, gfar_irq(grp, ER)->name,
> > > +                                    grp);
> >
> > This is called during open/close, so the lifetime of the irqs isn't
> > tied to the struct device, devm won't apply here. If you
> > open/close/re-open the device, you'll request the same irq multiple
> > times.
> Good point. Would it make sense to move to probe?

Hello,
Many drivers do request_irq() at device open(), i.e. Intel (e1000, igb),
Broadcom, Marvell, to name a few. And I think that calling request_irq()
at open() is a good practice at least. Do you plan to transition all these drivers
to devm_request_irq()?

-Claudiu

[PATCH net-next 6/6] net: gianfar: use devm for of_iomap

[Rosen Penev]

Avoids having to manually unmap.

Removes all gotos in probe.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 26 +++++-------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 78fdab3c6f77..96eeef0d6bd3 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -441,15 +441,6 @@ static int gfar_alloc_rx_queues(struct gfar_private *priv)
 	return 0;
 }
 
-static void unmap_group_regs(struct gfar_private *priv)
-{
-	int i;
-
-	for (i = 0; i < MAXGROUPS; i++)
-		if (priv->gfargrp[i].regs)
-			iounmap(priv->gfargrp[i].regs);
-}
-
 static void disable_napi(struct gfar_private *priv)
 {
 	int i;
@@ -483,7 +474,7 @@ static int gfar_parse_group(struct device_node *np,
 			return -ENOMEM;
 	}
 
-	grp->regs = of_iomap(np, 0);
+	grp->regs = devm_of_iomap(priv->dev, np, 0, NULL);
 	if (!grp->regs)
 		return -ENOMEM;
 
@@ -698,13 +689,13 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 			err = gfar_parse_group(child, priv, model);
 			if (err) {
 				of_node_put(child);
-				goto err_grp_init;
+				return err;
 			}
 		}
 	} else { /* SQ_SG_MODE */
 		err = gfar_parse_group(np, priv, model);
 		if (err)
-			goto err_grp_init;
+			return err;
 	}
 
 	if (of_property_read_bool(np, "bd-stash")) {
@@ -727,7 +718,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 
 	err = of_get_ethdev_address(np, dev);
 	if (err == -EPROBE_DEFER)
-		goto err_grp_init;
+		return err;
 	if (err) {
 		eth_hw_addr_random(dev);
 		dev_info(&ofdev->dev, "Using random MAC address: %pM\n", dev->dev_addr);
@@ -775,7 +766,7 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 	if (!priv->phy_node && of_phy_is_fixed_link(np)) {
 		err = of_phy_register_fixed_link(np);
 		if (err)
-			goto err_grp_init;
+			return err;
 
 		priv->phy_node = of_node_get(np);
 	}
@@ -784,10 +775,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 	priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
 
 	return 0;
-
-err_grp_init:
-	unmap_group_regs(priv);
-	return err;
 }
 
 static u32 cluster_entry_per_class(struct gfar_private *priv, u32 rqfar,
@@ -3293,7 +3280,6 @@ static int gfar_probe(struct platform_device *ofdev)
 register_fail:
 	if (of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
-	unmap_group_regs(priv);
 	of_node_put(priv->phy_node);
 	of_node_put(priv->tbi_node);
 	return err;
@@ -3309,8 +3295,6 @@ static void gfar_remove(struct platform_device *ofdev)
 
 	if (of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
-
-	unmap_group_regs(priv);
 }
 
 #ifdef CONFIG_PM
-- 
2.46.2

Re: [PATCH net-next 6/6] net: gianfar: use devm for of_iomap

[Maxime Chevallier]

Hi,

On Tue,  1 Oct 2024 14:22:04 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> Avoids having to manually unmap.
> 
> Removes all gotos in probe.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

That's a short and not very helpful commit log...

Besides that,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime