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

[Hugo Villeneuve <hugo@hugovil.com>]

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