[PATCH -next] clk: tegra: tegra124-emc: fix missing clk_disable_unprepare() on error in emc_set_timing()

[PATCH -next] clk: tegra: tegra124-emc: fix missing clk_disable_unprepare() on error in emc_set_timing()

[Yang Yingliang]

After calling clk_prepare_enable(), clk_disable_unprepare() need
be called when prepare_timing_change() failed.

Fixes: 2db04f16b589 ("clk: tegra: Add EMC clock driver")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/clk/tegra/clk-tegra124-emc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-tegra124-emc.c b/drivers/clk/tegra/clk-tegra124-emc.c
index bdf6f4a51617..74c1d894cca8 100644
--- a/drivers/clk/tegra/clk-tegra124-emc.c
+++ b/drivers/clk/tegra/clk-tegra124-emc.c
@@ -249,8 +249,10 @@ static int emc_set_timing(struct tegra_clk_emc *tegra,
 	div = timing->parent_rate / (timing->rate / 2) - 2;
 
 	err = tegra->prepare_timing_change(emc, timing->rate);
-	if (err)
+	if (err) {
+		clk_disable_unprepare(timing->parent);
 		return err;
+	}
 
 	spin_lock_irqsave(tegra->lock, flags);
 
-- 
2.25.1

Re: [PATCH -next] clk: tegra: tegra124-emc: fix missing clk_disable_unprepare() on error in emc_set_timing()

[Stephen Boyd]

Why does the subject have -next in it?

Quoting Yang Yingliang (2021-05-17 21:42:47)
> After calling clk_prepare_enable(), clk_disable_unprepare() need
> be called when prepare_timing_change() failed.
> 
> Fixes: 2db04f16b589 ("clk: tegra: Add EMC clock driver")

And then the Fixes tag is for a patch that was merged in v4.10?

> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  drivers/clk/tegra/clk-tegra124-emc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra124-emc.c b/drivers/clk/tegra/clk-tegra124-emc.c
> index bdf6f4a51617..74c1d894cca8 100644
> --- a/drivers/clk/tegra/clk-tegra124-emc.c
> +++ b/drivers/clk/tegra/clk-tegra124-emc.c
> @@ -249,8 +249,10 @@ static int emc_set_timing(struct tegra_clk_emc *tegra,
>         div = timing->parent_rate / (timing->rate / 2) - 2;
>  
>         err = tegra->prepare_timing_change(emc, timing->rate);
> -       if (err)
> +       if (err) {
> +               clk_disable_unprepare(timing->parent);
>                 return err;
> +       }
>  
>         spin_lock_irqsave(tegra->lock, flags);
>  

Looks correct to me. I assume Thierry will pick it up for the next merge
window.

Re: [PATCH -next] clk: tegra: tegra124-emc: fix missing clk_disable_unprepare() on error in emc_set_timing()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

On Wed, Jun 02, 2021 at 12:58:07AM -0700, Stephen Boyd wrote:
> Why does the subject have -next in it?
> 
> Quoting Yang Yingliang (2021-05-17 21:42:47)
> > After calling clk_prepare_enable(), clk_disable_unprepare() need
> > be called when prepare_timing_change() failed.
> > 
> > Fixes: 2db04f16b589 ("clk: tegra: Add EMC clock driver")
> 
> And then the Fixes tag is for a patch that was merged in v4.10?

To be honest, I'm not sure it's worth backporting this. The probability
of this happening is very small and the worst that will happen is that
the parent clock may stay enabled even if it doesn't have to. However,
the parent clock for these is one of pll_p, pll_c or pll_m, all of which
are usually sourced from other peripherals.

Yes, it's nice to fix this, but I don't think it warrants a backport, so
I think I'll just drop the Fixes tag.

> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > ---
> >  drivers/clk/tegra/clk-tegra124-emc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-tegra124-emc.c b/drivers/clk/tegra/clk-tegra124-emc.c
> > index bdf6f4a51617..74c1d894cca8 100644
> > --- a/drivers/clk/tegra/clk-tegra124-emc.c
> > +++ b/drivers/clk/tegra/clk-tegra124-emc.c
> > @@ -249,8 +249,10 @@ static int emc_set_timing(struct tegra_clk_emc *tegra,
> >         div = timing->parent_rate / (timing->rate / 2) - 2;
> >  
> >         err = tegra->prepare_timing_change(emc, timing->rate);
> > -       if (err)
> > +       if (err) {
> > +               clk_disable_unprepare(timing->parent);
> >                 return err;
> > +       }
> >  
> >         spin_lock_irqsave(tegra->lock, flags);
> >  
> 
> Looks correct to me. I assume Thierry will pick it up for the next merge
> window.

Yes, I can do that. I've got a couple of minor fixes queued already, so
this will fit right in.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]