[v1] ibm: emac: more cleanups

[PATCH net-next 13/13] net: ibm: emac: mal: use devm for request_irq

Posted by Rosen Penev 1 month, 4 weeks ago

Avoids manual frees. Also replaced irq_of_parse_and_map with
platform_get_irq since it's simpler and does the same thing.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/mal.c | 47 ++++++++++++-----------------
 1 file changed, 19 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
index 70019ced47ff..449f0abe6e9e 100644
--- a/drivers/net/ethernet/ibm/emac/mal.c
+++ b/drivers/net/ethernet/ibm/emac/mal.c
@@ -578,15 +578,15 @@ static int mal_probe(struct platform_device *ofdev)
 #endif
 	}
 
-	mal->txeob_irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
-	mal->rxeob_irq = irq_of_parse_and_map(ofdev->dev.of_node, 1);
-	mal->serr_irq = irq_of_parse_and_map(ofdev->dev.of_node, 2);
+	mal->txeob_irq = platform_get_irq(ofdev, 0);
+	mal->rxeob_irq = platform_get_irq(ofdev, 1);
+	mal->serr_irq = platform_get_irq(ofdev, 2);
 
 	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
 		mal->txde_irq = mal->rxde_irq = mal->serr_irq;
 	} else {
-		mal->txde_irq = irq_of_parse_and_map(ofdev->dev.of_node, 3);
-		mal->rxde_irq = irq_of_parse_and_map(ofdev->dev.of_node, 4);
+		mal->txde_irq = platform_get_irq(ofdev, 3);
+		mal->rxde_irq = platform_get_irq(ofdev, 4);
 	}
 
 	if (!mal->txeob_irq || !mal->rxeob_irq || !mal->serr_irq ||
@@ -660,21 +660,26 @@ static int mal_probe(struct platform_device *ofdev)
 		hdlr_rxde = mal_rxde;
 	}
 
-	err = request_irq(mal->serr_irq, hdlr_serr, irqflags, "MAL SERR", mal);
+	err = devm_request_irq(&ofdev->dev, mal->serr_irq, hdlr_serr, irqflags,
+			       "MAL SERR", mal);
 	if (err)
 		goto fail2;
-	err = request_irq(mal->txde_irq, hdlr_txde, irqflags, "MAL TX DE", mal);
+	err = devm_request_irq(&ofdev->dev, mal->txde_irq, hdlr_txde, irqflags,
+			       "MAL TX DE", mal);
 	if (err)
-		goto fail3;
-	err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
+		goto fail2;
+	err = devm_request_irq(&ofdev->dev, mal->txeob_irq, mal_txeob, 0,
+			       "MAL TX EOB", mal);
 	if (err)
-		goto fail4;
-	err = request_irq(mal->rxde_irq, hdlr_rxde, irqflags, "MAL RX DE", mal);
+		goto fail2;
+	err = devm_request_irq(&ofdev->dev, mal->rxde_irq, hdlr_rxde, irqflags,
+			       "MAL RX DE", mal);
 	if (err)
-		goto fail5;
-	err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
+		goto fail2;
+	err = devm_request_irq(&ofdev->dev, mal->rxeob_irq, mal_rxeob, 0,
+			       "MAL RX EOB", mal);
 	if (err)
-		goto fail6;
+		goto fail2;
 
 	/* Enable all MAL SERR interrupt sources */
 	set_mal_dcrn(mal, MAL_IER, MAL_IER_EVENTS);
@@ -693,14 +698,6 @@ static int mal_probe(struct platform_device *ofdev)
 
 	return 0;
 
- fail6:
-	free_irq(mal->rxde_irq, mal);
- fail5:
-	free_irq(mal->txeob_irq, mal);
- fail4:
-	free_irq(mal->txde_irq, mal);
- fail3:
-	free_irq(mal->serr_irq, mal);
  fail2:
 	dma_free_coherent(&ofdev->dev, bd_size, mal->bd_virt, mal->bd_dma);
  fail_dummy:
@@ -725,12 +722,6 @@ static void mal_remove(struct platform_device *ofdev)
 		       "mal%d: commac list is not empty on remove!\n",
 		       mal->index);
 
-	free_irq(mal->serr_irq, mal);
-	free_irq(mal->txde_irq, mal);
-	free_irq(mal->txeob_irq, mal);
-	free_irq(mal->rxde_irq, mal);
-	free_irq(mal->rxeob_irq, mal);
-
 	mal_reset(mal);
 
 	free_netdev(mal->dummy_dev);
-- 
2.46.2

Re: [PATCH net-next 13/13] net: ibm: emac: mal: use devm for request_irq

Posted by Simon Horman 1 month, 4 weeks ago

On Mon, Sep 30, 2024 at 11:00:36AM -0700, Rosen Penev wrote:
> Avoids manual frees. Also replaced irq_of_parse_and_map with
> platform_get_irq since it's simpler and does the same thing.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/ibm/emac/mal.c | 47 ++++++++++++-----------------
>  1 file changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
> index 70019ced47ff..449f0abe6e9e 100644
> --- a/drivers/net/ethernet/ibm/emac/mal.c
> +++ b/drivers/net/ethernet/ibm/emac/mal.c
> @@ -578,15 +578,15 @@ static int mal_probe(struct platform_device *ofdev)
>  #endif
>  	}
>  
> -	mal->txeob_irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> -	mal->rxeob_irq = irq_of_parse_and_map(ofdev->dev.of_node, 1);
> -	mal->serr_irq = irq_of_parse_and_map(ofdev->dev.of_node, 2);
> +	mal->txeob_irq = platform_get_irq(ofdev, 0);
> +	mal->rxeob_irq = platform_get_irq(ofdev, 1);
> +	mal->serr_irq = platform_get_irq(ofdev, 2);
>  
>  	if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
>  		mal->txde_irq = mal->rxde_irq = mal->serr_irq;
>  	} else {
> -		mal->txde_irq = irq_of_parse_and_map(ofdev->dev.of_node, 3);
> -		mal->rxde_irq = irq_of_parse_and_map(ofdev->dev.of_node, 4);
> +		mal->txde_irq = platform_get_irq(ofdev, 3);
> +		mal->rxde_irq = platform_get_irq(ofdev, 4);
>  	}
>  
>  	if (!mal->txeob_irq || !mal->rxeob_irq || !mal->serr_irq ||

I think this error handing needs to be updated.
platform_get_irq() returns either a non-zero IRQ number or
a negative error value. It does not return zero.

Flagged by Smatch.

...

Re: [PATCH net-next 13/13] net: ibm: emac: mal: use devm for request_irq

Posted by Rosen Penev 1 month, 3 weeks ago

On Tue, Oct 1, 2024 at 6:32 AM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Sep 30, 2024 at 11:00:36AM -0700, Rosen Penev wrote:
> > Avoids manual frees. Also replaced irq_of_parse_and_map with
> > platform_get_irq since it's simpler and does the same thing.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/ibm/emac/mal.c | 47 ++++++++++++-----------------
> >  1 file changed, 19 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
> > index 70019ced47ff..449f0abe6e9e 100644
> > --- a/drivers/net/ethernet/ibm/emac/mal.c
> > +++ b/drivers/net/ethernet/ibm/emac/mal.c
> > @@ -578,15 +578,15 @@ static int mal_probe(struct platform_device *ofdev)
> >  #endif
> >       }
> >
> > -     mal->txeob_irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> > -     mal->rxeob_irq = irq_of_parse_and_map(ofdev->dev.of_node, 1);
> > -     mal->serr_irq = irq_of_parse_and_map(ofdev->dev.of_node, 2);
> > +     mal->txeob_irq = platform_get_irq(ofdev, 0);
> > +     mal->rxeob_irq = platform_get_irq(ofdev, 1);
> > +     mal->serr_irq = platform_get_irq(ofdev, 2);
> >
> >       if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> >               mal->txde_irq = mal->rxde_irq = mal->serr_irq;
> >       } else {
> > -             mal->txde_irq = irq_of_parse_and_map(ofdev->dev.of_node, 3);
> > -             mal->rxde_irq = irq_of_parse_and_map(ofdev->dev.of_node, 4);
> > +             mal->txde_irq = platform_get_irq(ofdev, 3);
> > +             mal->rxde_irq = platform_get_irq(ofdev, 4);
> >       }
> >
> >       if (!mal->txeob_irq || !mal->rxeob_irq || !mal->serr_irq ||
>
> I think this error handing needs to be updated.
> platform_get_irq() returns either a non-zero IRQ number or
> a negative error value. It does not return zero.
I wonder if error handling here is needed. devm_request_irq will error
if something is wrong.
>
> Flagged by Smatch.
>
> ...