Re: [PATCH 3/3] serial: 8250_mtk: Propagate errors from optional IRQ lookup

On Fri, Aug 7, 2026 at 4:22 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Critical] The probe function exits with an error without unregistering the previously registered 8250 port, leading to a port leak and a use-after-free of devm-allocated memory.
>
> Pre-existing issues:
> - [High] Runtime PM is enabled after the UART port is registered, violating TTY subsystem guidelines and risking circular wait conditions during probe.
> --
>
> --- Patch [3]: [PATCH 3/3] serial: 8250_mtk: Propagate errors from optional IRQ lookup ---
> commit b5b9e4c48c3e7b1094e5e1c9834f9f8130a17c26
> Author: bui duc phuc <[email protected]>
>
> serial: 8250_mtk: Propagate errors from optional IRQ lookup
>
> This commit updates mtk8250_probe() to propagate error codes such as
> -EPROBE_DEFER or -EINVAL from platform_get_irq_optional(), rather than
> continuing without an IRQ.
>
> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > index e6a56cf54ae0c..d0d2aecf90059 100644
> > --- a/drivers/tty/serial/8250/8250_mtk.c
> > +++ b/drivers/tty/serial/8250/8250_mtk.c
>
> [ ... ]
>
> > @@ -587,6 +587,8 @@ static int mtk8250_probe(struct platform_device *pdev)
> >               return data->line;
> >
> >       data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
> > +     if (data->rx_wakeup_irq < 0 && data->rx_wakeup_irq != -ENXIO)
> > +             return data->rx_wakeup_irq;
>
> [Severity: Critical]
> Does this error path leak the registered serial port?
>
> The port was successfully registered earlier in mtk8250_probe() via
> serial8250_register_8250_port(), but there is no call to
> serial8250_unregister_port() before returning the error here.
>
> Could this result in a use-after-free if the tty device is later accessed?
> The devm-allocated memory for the data structure will be automatically freed
> when the probe fails, but the port remains active globally.
>
> >
> >       pm_runtime_set_active(&pdev->dev);
> >       pm_runtime_enable(&pdev->dev);
>

I agree that this makes sense. I'll make the change and send v2.

> [Severity: High]
> This is a pre-existing issue, but should runtime PM be enabled before
> the serial port is registered?
>
> The mtk8250_do_pm() callback invokes pm_runtime_get_sync(). If
> serial8250_register_8250_port() synchronously invokes the pm callback
> during registration, it will attempt to use uninitialized runtime PM
> infrastructure, which can lead to circular wait conditions.
>
> >
> >       return 0;
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

In mtk8250_probe(), uart.port.pm is set to mtk8250_do_pm:

uart.port.pm = mtk8250_do_pm;

In serial8250_register_8250_port(), this part:

if (uart->port.type != PORT_8250_CIR) {
        if (uart_console_registered(&uart->port))
                pm_runtime_get_sync(uart->port.dev);

calls pm_runtime_get_sync() directly on the device, and I don't see a
direct call to port.pm here.
it may be invoked in some other helper function called within
serial8250_register_8250_port() itself.

That said, this is a pre-existing issue, so I'd like to keep this
patch scoped as-is and not address it here.