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

[Hugo Villeneuve <hugo@hugovil.com>]

Hi,

On Fri, 30 Oct 2026 16:55:07 +0800
laixiangwu <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.
> 
> All the later error paths of the probe already jump to the out_clk
> label to disable the clock. Do the same here.
> 
> Fixes: cb1b206cff46 ("serial: sc16is7xx: Use devm_clk_get_optional()")
> Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index fba588559..e464c6d72 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1619,10 +1619,12 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  			freq = uartclk;
>  		if (pfreq)
>  			freq = *pfreq;
> -		if (freq)
> +		if (freq) {
>  			dev_dbg(dev, "Clock frequency: %luHz\n", freq);
> -		else
> -			return -EINVAL;
> +		} else {
> +			ret = -EINVAL;
> +			goto out_clk;
> +		}
>  	}
>  
>  	s->devtype = devtype;
> -- 

You can probably switch to using devm_clk_get_optional_enabled(),
to simplify and completely get rid of the goto and out_clk label?

Something along:


-       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,8 @@ 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 +1705,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 +1731,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);
 
-- 
Hugo Villeneuve