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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 2 Jun 2021 11:06:42 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Stephen Boyd <sboyd@...nel.org>
Cc:     Yang Yingliang <yangyingliang@...wei.com>,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-tegra@...r.kernel.org, jonathanh@...dia.com
Subject: Re: [PATCH -next] clk: tegra: tegra124-emc: fix missing
 clk_disable_unprepare() on error in emc_set_timing()

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@...wei.com>
> > Signed-off-by: Yang Yingliang <yangyingliang@...wei.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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)