Re: [PATCH v2] serial: 8250_uniphier: Use devm_clk_get_enabled()

[Kunihiko Hayashi <hayashi.kunihiko@socionext.com>]

Hi Malathi,

On 2026/09/17 13:05, Malathi A wrote:
> uniphier_uart_probe() enables priv->clk by hand and is then responsible
> for disabling it again on every error path. It misses one: when
> uart_read_port_properties() fails the function returns directly, leaving
> the clock prepared and enabled. The neighbouring path, taken when
> serial8250_register_8250_port() fails, does call clk_disable_unprepare(),
> which shows the leak is an oversight rather than intent.
> 
> Rather than adding another manual unwind, switch to
> devm_clk_get_enabled() so the clock is released by devres. That removes
> the error path entirely, along with the explicit disables in the
> register failure path and in uniphier_uart_remove().
> 
> The clock is still disabled and re-enabled by hand across system sleep,
> which stays balanced: only SET_SYSTEM_SLEEP_PM_OPS is used, so the
> device cannot be unbound while suspended and devres always sees an
> enabled clock at detach.
> 
> Found by smatch:
> 
>    drivers/tty/serial/8250/8250_uniphier.c:232 uniphier_uart_probe() warn:
> 'priv->clk' from clk_prepare_enable() not released on lines: 205.
> 
> Fixes: 26e8349c0d76 ("serial: 8250_uniphier: Switch to use
> uart_read_port_properties()")
> Signed-off-by: Malathi A <malathi.a2000@gmail.com>
> ---
> Changes in v2:
> - Use devm_clk_get_enabled() instead of adding a clk_disable_unprepare()
>    to the failing path, as suggested in review. This also lets the
>    explicit disables in the serial8250_register_8250_port() error path and
>    in uniphier_uart_remove() go away.
> - Reworded the commit message accordingly.
> 
>   drivers/tty/serial/8250/8250_uniphier.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_uniphier.c
> b/drivers/tty/serial/8250/8250_uniphier.c
> index e3db60bf50c9..6fffac12277a 100644
> --- a/drivers/tty/serial/8250/8250_uniphier.c
> +++ b/drivers/tty/serial/8250/8250_uniphier.c
> @@ -180,16 +180,12 @@ static int uniphier_uart_probe(struct
> platform_device *pdev)
>   
>   	memset(&up, 0, sizeof(up));
>   
> -	priv->clk = devm_clk_get(dev, NULL);
> +	priv->clk = devm_clk_get_enabled(dev, NULL);
>   	if (IS_ERR(priv->clk)) {
> -		dev_err(dev, "failed to get clock\n");
> +		dev_err(dev, "failed to get and enable clock\n");
>   		return PTR_ERR(priv->clk);
>   	}
>   
> -	ret = clk_prepare_enable(priv->clk);
> -	if (ret)
> -		return ret;
> -
>   	up.port.uartclk = clk_get_rate(priv->clk);
>   
>   	spin_lock_init(&priv->atomic_write_lock);
> @@ -222,7 +218,6 @@ static int uniphier_uart_probe(struct platform_device
> *pdev)
>   	ret = serial8250_register_8250_port(&up);
>   	if (ret < 0) {
>   		dev_err(dev, "failed to register 8250 port\n");
> -		clk_disable_unprepare(priv->clk);
>   		return ret;
>   	}
>   	priv->line = ret;
> @@ -237,7 +232,6 @@ static void uniphier_uart_remove(struct
> platform_device *pdev)
>   	struct uniphier8250_priv *priv = platform_get_drvdata(pdev);
>   
>   	serial8250_unregister_port(priv->line);
> -	clk_disable_unprepare(priv->clk);
>   }
>   
>   static int __maybe_unused uniphier_uart_suspend(struct device *dev)

I'm a bit concerned about the error path in uniphier_uart_resume().
The clock may have been disabled in uniphier_uart_suspend(), and
clk_prepare_enable() can fail when trying to enable it again.

In that case, wouldn't devres try to disable an already disabled
clock on detach?

Thank you,

---
Best Regards
Kunihiko Hayashi