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

[Rosen Penev]

On Thu, Jun 25, 2026 at 2:36 AM Simon Horman <horms@xxxxxxxxxx> 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.