[PATCH v2] serial: sc16is7xx: Fix clock leak on invalid clock frequency

[PATCH v2] serial: sc16is7xx: Fix clock leak on invalid clock frequency

[Xiangwu Lai]

sc16is7xx_probe() enables the clock with clk_prepare_enable() early
in the probe path.  If the resulting clock frequency cannot be
determined (neither from the clock itself, nor from the
"clock-frequency" property or platform data), the driver bails out
with -EINVAL, but it does so with a plain return, leaving the clock
prepared and enabled.

Switch to devm_clk_get_optional_enabled(), which drops the clock
reference and disables the clock automatically when the probe fails
and when the device is removed.  The manual clock management is no
longer needed: drop clk_prepare_enable(), the out_clk label and the
clk_disable_unprepare() call in sc16is7xx_remove().

Fixes: cb1b206cff46 ("serial: sc16is7xx: Use devm_clk_get_optional()")
Suggested-by: Hugo Villeneuve <hugo@hugovil.com>
Signed-off-by: Xiangwu Lai <laixiangwu@cmss.chinamobile.com>
---
Changes in v2:

- Rework the patch following Hugo Villeneuve's suggestion: switch to
  devm_clk_get_optional_enabled() so the clock is disabled
  automatically on probe failure and on remove, and remove the now
  unneeded manual clock management (clk_prepare_enable(), the
  out_clk label and the clk_disable_unprepare() call in
  sc16is7xx_remove()).  The -EINVAL path keeps its original plain
  return since devm handles the cleanup.

Link: https://lore.kernel.org/all/20261030085508.1882-1-laixiangwu@cmss.chinamobile.com
 drivers/tty/serial/sc16is7xx.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index fba588559..a69b71fb8 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1605,14 +1605,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		dev_dbg(dev,
 			"No interrupt pin definition, falling back to polling mode\n");
 
-	s->clk = devm_clk_get_optional(dev, NULL);
+	s->clk = devm_clk_get_optional_enabled(dev, NULL);
 	if (IS_ERR(s->clk))
 		return PTR_ERR(s->clk);
 
-	ret = clk_prepare_enable(s->clk);
-	if (ret)
-		return ret;
-
 	freq = clk_get_rate(s->clk);
 	if (freq == 0) {
 		if (uartclk)
@@ -1631,10 +1627,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	kthread_init_worker(&s->kworker);
 	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
 				      "sc16is7xx");
-	if (IS_ERR(s->kworker_task)) {
-		ret = PTR_ERR(s->kworker_task);
-		goto out_clk;
-	}
+	if (IS_ERR(s->kworker_task))
+		return PTR_ERR(s->kworker_task);
+
 	sched_set_fifo(s->kworker_task);
 
 	ret = sc16is7xx_reset(dev, regmaps[0]);
@@ -1711,9 +1706,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 out_kthread:
 	kthread_stop(s->kworker_task);
 
-out_clk:
-	clk_disable_unprepare(s->clk);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(sc16is7xx_probe);
@@ -1740,8 +1732,6 @@ void sc16is7xx_remove(struct device *dev)
 
 	kthread_flush_worker(&s->kworker);
 	kthread_stop(s->kworker_task);
-
-	clk_disable_unprepare(s->clk);
 }
 EXPORT_SYMBOL_GPL(sc16is7xx_remove);
 
-- 
2.50.1.windows.1

Re: [PATCH v2] serial: sc16is7xx: Fix clock leak on invalid clock frequency

[Hugo Villeneuve]

Hi,

On Fri, 13 Nov 2026 18:38:54 +0800
Xiangwu Lai <laixiangwu@cmss.chinamobile.com> wrote:

> sc16is7xx_probe() enables the clock with clk_prepare_enable() early
> in the probe path.  If the resulting clock frequency cannot be
> determined (neither from the clock itself, nor from the
> "clock-frequency" property or platform data), the driver bails out
> with -EINVAL, but it does so with a plain return, leaving the clock
> prepared and enabled.
> 
> Switch to devm_clk_get_optional_enabled(), which drops the clock
> reference and disables the clock automatically when the probe fails
> and when the device is removed.  The manual clock management is no
> longer needed: drop clk_prepare_enable(), the out_clk label and the
> clk_disable_unprepare() call in sc16is7xx_remove().
> 
> Fixes: cb1b206cff46 ("serial: sc16is7xx: Use devm_clk_get_optional()")
> Suggested-by: Hugo Villeneuve <hugo@hugovil.com>
> Signed-off-by: Xiangwu Lai <laixiangwu@cmss.chinamobile.com>

Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>


> ---
> Changes in v2:
> 
> - Rework the patch following Hugo Villeneuve's suggestion: switch to
>   devm_clk_get_optional_enabled() so the clock is disabled
>   automatically on probe failure and on remove, and remove the now
>   unneeded manual clock management (clk_prepare_enable(), the
>   out_clk label and the clk_disable_unprepare() call in
>   sc16is7xx_remove()).  The -EINVAL path keeps its original plain
>   return since devm handles the cleanup.
> 
> Link: https://lore.kernel.org/all/20261030085508.1882-1-laixiangwu@cmss.chinamobile.com
>  drivers/tty/serial/sc16is7xx.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index fba588559..a69b71fb8 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1605,14 +1605,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		dev_dbg(dev,
>  			"No interrupt pin definition, falling back to polling mode\n");
>  
> -	s->clk = devm_clk_get_optional(dev, NULL);
> +	s->clk = devm_clk_get_optional_enabled(dev, NULL);
>  	if (IS_ERR(s->clk))
>  		return PTR_ERR(s->clk);
>  
> -	ret = clk_prepare_enable(s->clk);
> -	if (ret)
> -		return ret;
> -
>  	freq = clk_get_rate(s->clk);
>  	if (freq == 0) {
>  		if (uartclk)
> @@ -1631,10 +1627,9 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  	kthread_init_worker(&s->kworker);
>  	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>  				      "sc16is7xx");
> -	if (IS_ERR(s->kworker_task)) {
> -		ret = PTR_ERR(s->kworker_task);
> -		goto out_clk;
> -	}
> +	if (IS_ERR(s->kworker_task))
> +		return PTR_ERR(s->kworker_task);
> +
>  	sched_set_fifo(s->kworker_task);
>  
>  	ret = sc16is7xx_reset(dev, regmaps[0]);
> @@ -1711,9 +1706,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  out_kthread:
>  	kthread_stop(s->kworker_task);
>  
> -out_clk:
> -	clk_disable_unprepare(s->clk);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(sc16is7xx_probe);
> @@ -1740,8 +1732,6 @@ void sc16is7xx_remove(struct device *dev)
>  
>  	kthread_flush_worker(&s->kworker);
>  	kthread_stop(s->kworker_task);
> -
> -	clk_disable_unprepare(s->clk);
>  }
>  EXPORT_SYMBOL_GPL(sc16is7xx_remove);
>  
> -- 
> 2.50.1.windows.1
> 
> 
> 
> 


-- 
Hugo Villeneuve