linux-kernel - Re: [linux-sunxi] [PATCH v2 2/5] clk: sun6i: Reparent AHB clock on PLL6

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] [day] [month] [year] [list]

Date:	Mon, 3 Mar 2014 14:26:22 +0100
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Hans de Goede <hdegoede@...hat.com>
Cc:	linux-sunxi@...glegroups.com, Emilio Lopez <emilio@...pez.com.ar>,
	Dan Williams <dan.j.williams@...el.com>,
	Vinod Koul <vinod.koul@...el.com>,
	Mike Turquette <mturquette@...aro.org>,
	linux-arm-kernel@...ts.infradead.org, dmaengine@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	kevin.z.m.zh@...il.com, sunny@...winnertech.com,
	shuge@...winnertech.com, zhuzhenhua@...winnertech.com
Subject: Re: [linux-sunxi] [PATCH v2 2/5] clk: sun6i: Reparent AHB clock on
 PLL6

Hi Hans,

On Sat, Mar 01, 2014 at 05:32:40PM +0100, Hans de Goede wrote:
> On 02/28/2014 05:37 PM, Maxime Ripard wrote:
> > In order for the DMA controller to work for SDRAM to devices transfers, the AHB
> > clock should be reparented on the PLL6.
> > 
> > Force that parenting in the clock driver.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> > ---
> >  drivers/clk/sunxi/clk-sunxi.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> > index f6f61cc..a5c5882 100644
> > --- a/drivers/clk/sunxi/clk-sunxi.c
> > +++ b/drivers/clk/sunxi/clk-sunxi.c
> > @@ -1286,7 +1286,7 @@ static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_mat
> >   */
> >  static void __init sunxi_clock_protect(void)
> >  {
> > -	struct clk *clk;
> > +	struct clk *clk, *parent;
> >  
> >  	/* memory bus clock - sun5i+ */
> >  	clk = clk_get(NULL, "mbus");
> > @@ -1307,6 +1307,22 @@ static void __init sunxi_clock_protect(void)
> >  	if (!IS_ERR(clk))
> >  		clk_prepare_enable(clk);
> >  
> > +	clk = clk_get(NULL, "ahb1_mux");
> > +	if (IS_ERR(clk)) {
> > +		pr_err("Couldn't get AHB1 Mux\n");
> > +		return;
> > +	}
> 
> I think Emilio already made a similar remark for v1, but since this
> is still here in v2, ahb1_mux is only available on sun6i, so
> at a minimum the pr_err should be dropped. Preferably I would like
> to see this changed to something like:
> 
> 	clk = clk_get(NULL, "ahb1_mux");
> 	parent = clk_get(NULL, "pll6");
> 	if (!IS_ERR(clk) && !IS_ERR(parent))
> 		clk_set_parent(clk, parent);
> 	if (!IS_ERR(parent))
> 		clk_put(parent);
> 	if (!IS_ERR(clk))
> 		clk_put(clk);

Still. The DMA doesn't work at all if the parenting is not set, so
having an error if it fails is mandatory.

And the statement that you made can also apply to any clock we enable
here, that might or might not be present in all the SoCs.

I guess we can solve this in two steps:
  - split out the reparenting, and move this into the DMA driver. That
    would isolate things in the driver itself, and prevent a
    reparenting attempt on every SoCs.
  - Split this function into several functions, one for each SoC. That
    would allow to do pretty much anything we want without having to
    care about wether it applies to other platforms.

> > +	parent = clk_get(NULL, "pll6");
> > +	if (IS_ERR(clk)) {
> 
> Copy paste error should be IS_ERR(parent).

Ah, yep, thanks!

> 
> > +		pr_err("Couldn't get PLL6\n");
> 
> If we keep things this way this error path should do a clk_put(clk);

Good catch too.

> > +		return;
> > +	}
> > +
> > +	clk_set_parent(clk, parent);
> > +
> > +	clk_put(clk);
> > +	clk_put(parent);
> >  }
> >  
> >  static void __init sunxi_init_clocks(void)
> > 
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> Given Russell's remarks it would also be good to have a
> patch in this set dropping the clk_put calls of the existing
> enables / protections.

Yes, I'll do it in the v3.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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