diff mbox series

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

Message ID 20241001212204.308758-6-rosenp@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series gianfar cleanups | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 12 this patch: 12
netdev/checkpatch warning WARNING: Missing commit description - Add an appropriate one
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

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

Comments

Maxime Chevallier Oct. 2, 2024, 7:37 a.m. UTC | #1 
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
Rosen Penev Oct. 2, 2024, 7:29 p.m. UTC | #2 
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
Andrew Lunn Oct. 2, 2024, 9:32 p.m. UTC | #3 
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
Claudiu Manoil Oct. 3, 2024, 7:37 p.m. UTC | #4 
> -----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
diff mbox series

Patch

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;
 }