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

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

[Malathi A]

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)
-- 
2.43.0

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

[Andy Shevchenko]

On Thu, Sep 17, 2026 at 04:05:09AM +0000, 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> -	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);

Consider fixing another latent bug here, id est with intermittent probe defer
this will spam the kernel dmesg a lot, so converting to use dev_err_probe()
will fix the issue (but at the same time you may convert all of the error
messages in probe to use return dev_err_probe() pattern). This needs to be
a separate change.

>  	}

-- 
With Best Regards,
Andy Shevchenko

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

[Kunihiko Hayashi]

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

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

[Andy Shevchenko]

On Thu, Sep 24, 2026 at 06:07:37PM +0900, Kunihiko Hayashi wrote:
> 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.

...

> 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?

Isn't there is a guarantee that the .remove() is called with PM runtime on and
get, meaning it's called with the enabled clock? But if not the case, we might
use PM runtime force operations (resume and suspend in the respective cases.

-- 
With Best Regards,
Andy Shevchenko

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

[Kunihiko Hayashi]

On 2026/09/24 23:09, Andy Shevchenko wrote:
> On Thu, Sep 24, 2026 at 06:07:37PM +0900, Kunihiko Hayashi wrote:
>> 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.
> 
> ...
> 
>> 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?
> 
> Isn't there is a guarantee that the .remove() is called with PM runtime on
> and
> get, meaning it's called with the enabled clock? But if not the case, we
> might
> use PM runtime force operations (resume and suspend in the respective
> cases.
> 

My concern was just the case where clk_prepare_enable() in the system
resume callback fails, leaving the clock disabled before devres cleanup.

This driver currently only uses SET_SYSTEM_SLEEP_PM_OPS(), so I wasn't
considering runtime PM here.

I don't have a strong opinion on whether runtime PM should be involved,
but if it guarantees that the clock is enabled before .remove(),
that would address my concern.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Andy Shevchenko]

On Fri, Sep 25, 2026 at 08:32:20PM +0900, Kunihiko Hayashi wrote:
> On 2026/09/24 23:09, Andy Shevchenko wrote:
> > On Thu, Sep 24, 2026 at 06:07:37PM +0900, Kunihiko Hayashi wrote:
> > > On 2026/09/17 13:05, Malathi A wrote:

...

> > > 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?
> > 
> > Isn't there is a guarantee that the .remove() is called with PM runtime on

After reading a code of driver core I see that this is the opposite actually.
The PM runtime might be off.

> > and get, meaning it's called with the enabled clock? But if not the case,
> > we might use PM runtime force operations (resume and suspend in the
> > respective cases.
> 
> My concern was just the case where clk_prepare_enable() in the system
> resume callback fails, leaving the clock disabled before devres cleanup.
> 
> This driver currently only uses SET_SYSTEM_SLEEP_PM_OPS(), so I wasn't
> considering runtime PM here.

So, in such a case how do you see the scenario when system is resumed (Right?
Otherwise we can't do anything, like detaching driver from the device.) and
clock is disabled?

> I don't have a strong opinion on whether runtime PM should be involved,
> but if it guarantees that the clock is enabled before .remove(),
> that would address my concern.

-- 
With Best Regards,
Andy Shevchenko