[PATCH net] net: gianfar: use of_irq_get()

[PATCH net] net: gianfar: use of_irq_get()

[Rosen Penev]

of_irq_get() differs from irq_of_parse_and_map() in that the latter
requires calling irq_dispose_mapping() when done, which is missing in the
driver. Meaning it leaks memory.

No need to map it anyway. Just need the value stored in the irq field.

Changed irq to an int as required by the of_irq_get API as it supports
-EPROBE_DEFER.

Fixes: b31a1d8b4151 ("gianfar: Convert gianfar to an of_platform_driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 12 ++++++------
 drivers/net/ethernet/freescale/gianfar.h |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 3271de5844f8..17a0d0787ed2 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -514,15 +514,15 @@ static int gfar_parse_group(struct device_node *np,
 	if (!grp->regs)
 		return -ENOMEM;
 
-	gfar_irq(grp, TX)->irq = irq_of_parse_and_map(np, 0);
+	gfar_irq(grp, TX)->irq = of_irq_get(np, 0);
 
 	/* If we aren't the FEC we have multiple interrupts */
 	if (model && strcasecmp(model, "FEC")) {
-		gfar_irq(grp, RX)->irq = irq_of_parse_and_map(np, 1);
-		gfar_irq(grp, ER)->irq = irq_of_parse_and_map(np, 2);
-		if (!gfar_irq(grp, TX)->irq ||
-		    !gfar_irq(grp, RX)->irq ||
-		    !gfar_irq(grp, ER)->irq)
+		gfar_irq(grp, RX)->irq = of_irq_get(np, 1);
+		gfar_irq(grp, ER)->irq = of_irq_get(np, 2);
+		if (gfar_irq(grp, TX)->irq < 0 ||
+		    gfar_irq(grp, RX)->irq < 0 ||
+		    gfar_irq(grp, ER)->irq < 0)
 			return -EINVAL;
 	}
 
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 68b59d3202e3..c6f1c0b6229e 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -1074,7 +1074,7 @@ enum gfar_irqinfo_id {
 };
 
 struct gfar_irqinfo {
-	unsigned int irq;
+	int irq;
 	char name[GFAR_INT_NAME_MAX];
 };
 
-- 
2.54.0

Re: [PATCH net] net: gianfar: use of_irq_get()

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: gianfar: use of_irq_get()

This patch switches gfar_parse_group() from irq_of_parse_and_map() to
of_irq_get() and widens the irq field in struct gfar_irqinfo from
unsigned int to int. The stated goals are to fix a leak from a missing
irq_dispose_mapping() and to enable -EPROBE_DEFER propagation.

> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 3271de5844f8..17a0d0787ed2 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -514,15 +514,15 @@ static int gfar_parse_group(struct device_node *np,
>  	if (!grp->regs)
>  		return -ENOMEM;
>
> -	gfar_irq(grp, TX)->irq = irq_of_parse_and_map(np, 0);
> +	gfar_irq(grp, TX)->irq = of_irq_get(np, 0);
>
>  	/* If we aren't the FEC we have multiple interrupts */
>  	if (model && strcasecmp(model, "FEC")) {
> -		gfar_irq(grp, RX)->irq = irq_of_parse_and_map(np, 1);
> -		gfar_irq(grp, ER)->irq = irq_of_parse_and_map(np, 2);
> -		if (!gfar_irq(grp, TX)->irq ||
> -		    !gfar_irq(grp, RX)->irq ||
> -		    !gfar_irq(grp, ER)->irq)
> +		gfar_irq(grp, RX)->irq = of_irq_get(np, 1);
> +		gfar_irq(grp, ER)->irq = of_irq_get(np, 2);
> +		if (gfar_irq(grp, TX)->irq < 0 ||
> +		    gfar_irq(grp, RX)->irq < 0 ||
> +		    gfar_irq(grp, ER)->irq < 0)
>  			return -EINVAL;
>  	}

[High]
Does this change miss the 0-return mapping failure case?

The kernel-doc on of_irq_get() in drivers/of/irq.c says it returns:

    Linux IRQ number on success, or 0 on the IRQ mapping failure, or
    -EPROBE_DEFER ..., or error code in case of any other failure.

The old test rejected an irq value of 0 with !gfar_irq(grp, *)->irq,
which caught both irq_of_parse_and_map()'s and of_irq_get()'s
zero-on-failure. The new test only rejects values strictly less than
zero, so a 0 returned from of_irq_get() is now stored as the IRQ and
later flows into request_irq()/free_irq() during probe and unbind.

Should the test be <= 0, or split so that negative errors are
propagated and 0 still triggers a failure?

[High]
Is -EPROBE_DEFER lost here?

The commit message states:

    Changed irq to an int as required by the of_irq_get API as it
    supports -EPROBE_DEFER.

However, the only error path is:

    if (gfar_irq(grp, TX)->irq < 0 ||
        gfar_irq(grp, RX)->irq < 0 ||
        gfar_irq(grp, ER)->irq < 0)
            return -EINVAL;

of_irq_get() returns -EPROBE_DEFER when irq_find_host() cannot yet
locate the parent interrupt controller. Collapsing every negative
return into -EINVAL prevents the driver core from retrying the probe
once the interrupt controller appears.

Should the actual error from of_irq_get() be propagated, for example:

    int ret = gfar_irq(grp, TX)->irq;
    if (ret < 0)
            return ret;

so that -EPROBE_DEFER reaches gfar_of_init() and then gfar_probe()?

[Medium]
Can the commit message description be revisited?

The message states:

    of_irq_get() differs from irq_of_parse_and_map() in that the latter
    requires calling irq_dispose_mapping() when done, which is missing
    in the driver. Meaning it leaks memory.

    No need to map it anyway. Just need the value stored in the irq
    field.

Looking at of_irq_get() in drivers/of/irq.c:

    int of_irq_get(struct device_node *dev, int index)
    {
            ...
            rc = irq_create_of_mapping(&oirq);
            ...
    }

of_irq_get() ends with the same irq_create_of_mapping() call that
irq_of_parse_and_map() makes, so the resulting virq mapping is created
identically by both APIs.

If the previous code leaked, does the new code not leak in the same
way? And does the Fixes: b31a1d8b4151 tag accurately describe this
change, given that it may cause stable-tree selection on the basis of a
leak that does not appear to exist?

Re: [PATCH net] net: gianfar: use of_irq_get()

[Rosen Penev]

On Thu, Jun 25, 2026 at 2:36 AM Simon Horman <horms@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> net: gianfar: use of_irq_get()
>
> This patch switches gfar_parse_group() from irq_of_parse_and_map() to
> of_irq_get() and widens the irq field in struct gfar_irqinfo from
> unsigned int to int. The stated goals are to fix a leak from a missing
> irq_dispose_mapping() and to enable -EPROBE_DEFER propagation.
>
> > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> > index 3271de5844f8..17a0d0787ed2 100644
> > --- a/drivers/net/ethernet/freescale/gianfar.c
> > +++ b/drivers/net/ethernet/freescale/gianfar.c
> > @@ -514,15 +514,15 @@ static int gfar_parse_group(struct device_node *np,
> >       if (!grp->regs)
> >               return -ENOMEM;
> >
> > -     gfar_irq(grp, TX)->irq = irq_of_parse_and_map(np, 0);
> > +     gfar_irq(grp, TX)->irq = of_irq_get(np, 0);
> >
> >       /* If we aren't the FEC we have multiple interrupts */
> >       if (model && strcasecmp(model, "FEC")) {
> > -             gfar_irq(grp, RX)->irq = irq_of_parse_and_map(np, 1);
> > -             gfar_irq(grp, ER)->irq = irq_of_parse_and_map(np, 2);
> > -             if (!gfar_irq(grp, TX)->irq ||
> > -                 !gfar_irq(grp, RX)->irq ||
> > -                 !gfar_irq(grp, ER)->irq)
> > +             gfar_irq(grp, RX)->irq = of_irq_get(np, 1);
> > +             gfar_irq(grp, ER)->irq = of_irq_get(np, 2);
> > +             if (gfar_irq(grp, TX)->irq < 0 ||
> > +                 gfar_irq(grp, RX)->irq < 0 ||
> > +                 gfar_irq(grp, ER)->irq < 0)
> >                       return -EINVAL;
> >       }
>
> [High]
> Does this change miss the 0-return mapping failure case?
>
> The kernel-doc on of_irq_get() in drivers/of/irq.c says it returns:
>
>     Linux IRQ number on success, or 0 on the IRQ mapping failure, or
>     -EPROBE_DEFER ..., or error code in case of any other failure.
>
> The old test rejected an irq value of 0 with !gfar_irq(grp, *)->irq,
> which caught both irq_of_parse_and_map()'s and of_irq_get()'s
> zero-on-failure. The new test only rejects values strictly less than
> zero, so a 0 returned from of_irq_get() is now stored as the IRQ and
> later flows into request_irq()/free_irq() during probe and unbind.
>
> Should the test be <= 0, or split so that negative errors are
> propagated and 0 still triggers a failure?
>
> [High]
> Is -EPROBE_DEFER lost here?
>
> The commit message states:
>
>     Changed irq to an int as required by the of_irq_get API as it
>     supports -EPROBE_DEFER.
>
> However, the only error path is:
>
>     if (gfar_irq(grp, TX)->irq < 0 ||
>         gfar_irq(grp, RX)->irq < 0 ||
>         gfar_irq(grp, ER)->irq < 0)
>             return -EINVAL;
>
> of_irq_get() returns -EPROBE_DEFER when irq_find_host() cannot yet
> locate the parent interrupt controller. Collapsing every negative
> return into -EINVAL prevents the driver core from retrying the probe
> once the interrupt controller appears.
>
> Should the actual error from of_irq_get() be propagated, for example:
>
>     int ret = gfar_irq(grp, TX)->irq;
>     if (ret < 0)
>             return ret;
>
> so that -EPROBE_DEFER reaches gfar_of_init() and then gfar_probe()?
>
> [Medium]
> Can the commit message description be revisited?
>
> The message states:
>
>     of_irq_get() differs from irq_of_parse_and_map() in that the latter
>     requires calling irq_dispose_mapping() when done, which is missing
>     in the driver. Meaning it leaks memory.
>
>     No need to map it anyway. Just need the value stored in the irq
>     field.
>
> Looking at of_irq_get() in drivers/of/irq.c:
>
>     int of_irq_get(struct device_node *dev, int index)
>     {
>             ...
>             rc = irq_create_of_mapping(&oirq);
>             ...
>     }
>
> of_irq_get() ends with the same irq_create_of_mapping() call that
> irq_of_parse_and_map() makes, so the resulting virq mapping is created
> identically by both APIs.
>
> If the previous code leaked, does the new code not leak in the same
> way? And does the Fixes: b31a1d8b4151 tag accurately describe this
> change, given that it may cause stable-tree selection on the basis of a
> leak that does not appear to exist?
Meh I'll submit to net-next. This is way too messy to fix for net.