Re: [PATCH 1/2] serial: amba-pl011: unprepare console clock on unregister

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing NULL check for `uap` in console callbacks leads to kernel panic if `uart_add_one_port` fails.
--

--- Patch [1]: [PATCH 1/2] serial: amba-pl011: unprepare console clock on unregister ---
commit 7c5636c95e763ddaf7574887aeb1e24ce1d44fec
Author: Karl Mehltretter <[email protected]>

serial: amba-pl011: unprepare console clock on unregister

This patch adds a console .exit() callback that calls clk_unprepare() on the
UART clock when the console is unregistered. This balances the clk_prepare()
called in pl011_console_setup().

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8ed91e1da22be..1aa43994a3cdc 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2552,6 +2552,15 @@ static int pl011_console_setup(struct console *co, char *options)
>  	return uart_set_options(&uap->port, co, baud, parity, bits, flow);
>  }
>  
> +static int pl011_console_exit(struct console *co)
> +{
> +	struct uart_amba_port *uap = amba_ports[co->index];
> +

[Severity: High]
This is a pre-existing issue, but does uap need a NULL check here to
prevent a kernel panic on error paths?

If uart_add_one_port() fails internally (e.g., an allocation failure in
serial_core_add_one_port() returning -ENOMEM), it returns an error but
crucially fails to unregister the console that was already registered
earlier in uart_configure_port().

The caller sees the error and clears the pointer:

drivers/tty/serial/amba-pl011.c:pl011_register_port() {
    ...
    ret = uart_add_one_port(&amba_reg, &uap->port);
    if (ret)
        pl011_unregister_port(uap);
    ...
}

Because pl011_unregister_port() clears amba_ports[co->index] to NULL, but
the console remains active in the system's console list, will
unconditionally dereferencing uap here (and in pl011_console_write_atomic)
cause a crash when invoked?

> +	clk_unprepare(uap->clk);
> +
> +	return 0;
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1