Re: [PATCH 1/3] serial: qcom-geni: Propagate errors from optional IRQ lookup

On Fri, Aug 7, 2026 at 4:17 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The error cleanup path in `qcom_geni_serial_probe()` leaks the IDA allocated for the UART line index, which is severely exacerbated by the newly added IRQ probe deferral path.
> --
>
> --- Patch [1]: [PATCH 1/3] serial: qcom-geni: Propagate errors from optional IRQ lookup ---
> commit 9ec5f0b3d9989a6071c2d4680336709af497a5bf
> Author: bui duc phuc <[email protected]>
>
> serial: qcom-geni: Propagate errors from optional IRQ lookup
>
> This commit updates qcom_geni_serial_probe() to propagate errors, such as
> -EPROBE_DEFER, from platform_get_irq_optional() for the wakeup IRQ.
> Previously, errors other than -ENXIO were ignored and probe continued.
>
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 7ead87b4eb65b..240a6d67d8475 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1883,8 +1883,13 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> >       uport->irq = irq;
> >       uport->has_sysrq = IS_ENABLED(CONFIG_SERIAL_QCOM_GENI_CONSOLE);
> >
> > -     if (!data->console)
> > +     if (!data->console) {
> >               port->wakeup_irq = platform_get_irq_optional(pdev, 1);
> > +             if (port->wakeup_irq < 0 && port->wakeup_irq != -ENXIO) {
> > +                     ret = port->wakeup_irq;
> > +                     goto error;
>
> [Severity: High]
> This is a pre-existing issue, but does this goto error path leak the port IDA?
>
> In get_port_from_line(), an IDA is allocated for the UART line index with a
> very restricted range:
>
> drivers/tty/serial/qcom_geni_serial.c:get_port_from_line() {
>     ...
>                 if (line < 0)
>                         line = ida_alloc_range(&port_ida, max_alias_num + 1,
>                                                nr_ports - 1, GFP_KERNEL);
>                 else
>                         line = ida_alloc_range(&port_ida, line,
>                                                nr_ports - 1, GFP_KERNEL);
>     ...
> }
>
> However, when jumping to the error label in qcom_geni_serial_probe(), the
> cleanup only detaches PM domains and misses calling ida_free():
>
> drivers/tty/serial/qcom_geni_serial.c:qcom_geni_serial_probe() {
>     ...
> error:
>         dev_pm_domain_detach_list(port->pd_list);
>         return ret;
> }
>
> Since this patch adds a check that propagates -EPROBE_DEFER when the optional
> wakeup IRQ is not yet ready, this error path will be frequently hit during
> normal boot.
>
> Because the max IDA limit is very small, just a few probe deferrals will
> exhaust the available IDs, causing get_port_from_line() to return -ENXIO and
> permanently preventing the UART driver from probing on subsequent attempts.
>
> Should ida_free(&port_ida, uport->line) be called before jumping to the error
> label, or added to the error label itself, to prevent exhausting the ID space?
>
> > +             }
> > +     }
> >
> >       if (of_property_read_bool(pdev->dev.of_node, "rx-tx-swap"))
> >               port->rx_tx_swap = true;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1


I noticed that get_port_from_line() is also called from other
functions, such as qcom_geni_console_setup()
and qcom_geni_serial_console_write().
Since this is a pre-existing issue, I think it would be better to
address it in a separate patch if we decide to fix it.
For the current patch, I would prefer to keep it as is.