linux-kernel - Re: [PATCH] clk: imx: gate2: Fix the is

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20201028095057.cuaxqqr4yzxvzwvp@fsr-ub1664-175>
Date:   Wed, 28 Oct 2020 11:50:57 +0200
From:   Abel Vesa <abel.vesa@....com>
To:     Sascha Hauer <s.hauer@...gutronix.de>
Cc:     Mike Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <fabio.estevam@....com>,
        Anson Huang <anson.huang@....com>,
        Jacky Bai <ping.bai@....com>, Peng Fan <peng.fan@....com>,
        Dong Aisheng <aisheng.dong@....com>, linux-clk@...r.kernel.org,
        NXP Linux Team <linux-imx@....com>,
        linux-arm-kernel@...ts.infradead.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clk: imx: gate2: Fix the is_enabled op

On 20-10-28 09:24:12, Sascha Hauer wrote:
> Hi Abel,
> 
> On Mon, Oct 26, 2020 at 08:50:48PM +0200, Abel Vesa wrote:
> > The clock is considered to be enabled only if the controlling bits
> > match the cgr_val mask. Also make sure the is_enabled returns the
> > correct vaule by locking the access to the register.
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@....com>
> > Fixes: 1e54afe9fcfe ("clk: imx: gate2: Allow single bit gating clock")
> > ---
> >  drivers/clk/imx/clk-gate2.c | 60 ++++++++++++++++++++-------------------------
> >  drivers/clk/imx/clk.h       |  8 ++----
> >  2 files changed, 29 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c
> > index 7eed708..f320bd2b 100644
> > --- a/drivers/clk/imx/clk-gate2.c
> > +++ b/drivers/clk/imx/clk-gate2.c
> > @@ -37,10 +37,22 @@ struct clk_gate2 {
> >  
> >  #define to_clk_gate2(_hw) container_of(_hw, struct clk_gate2, hw)
> >  
> > +static void clk_gate2_do_shared_clks(struct clk_hw *hw, bool enable)
> > +{
> > +	struct clk_gate2 *gate = to_clk_gate2(hw);
> > +	u32 reg;
> > +
> > +	reg = readl(gate->reg);
> > +	if (enable)
> > +		reg |= gate->cgr_val << gate->bit_idx;
> > +	else
> > +		reg &= ~(gate->cgr_val << gate->bit_idx);
> 
> Shouldn't this be:
> 
> 	reg &= ~(3 << gate->bit_idx);
> 	if (enable)
> 		reg |= gate->cgr_val << gate->bit_idx;
> 
> At least that's how it was without this patch and that's how it makes
> sense to me with cgr_val != 3.
> 

Well, that's the actual problem. The value 3 forces all the clocks
that register with this clock type to have 2 bits for controlling the gate.

My patch (though now I think I should split it into 2 separate patches) allows
two HW gates to be controlled by as many bits necessary. For example, there
could be multiple HW gates that are controled by the same bit. By passing
the cgr_val when registering the clocks you can specify how many bits (as a mask)
control all those HW gates that share their control bits.

> Sascha
>