| 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
| ||
|
Date: Thu, 16 Apr 2015 17:09:18 -0500
From: Michael Welling <mwelling@...e.org>
To: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Cc: Tero Kristo <t-kristo@...com>,
Mike Turquette <mturquette@...aro.org>,
Stephen Boyd <sboyd@...eaurora.org>,
Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Russell King <linux@....linux.org.uk>,
devicetree <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Tony Lindgren <tony@...mide.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Daniel Mack <daniel@...que.org>
Subject: Re: AM335x OMAP2 common clock external fixed-clock registration
On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
> On 16.04.2015 18:17, Michael Welling wrote:
> >On Thu, Apr 16, 2015 at 07:32:32AM +0300, Tero Kristo wrote:
> >>On 04/15/2015 11:51 PM, Michael Welling wrote:
> >>>On Wed, Apr 15, 2015 at 01:45:53PM -0700, Mike Turquette wrote:
> >>>>On Wed, Apr 15, 2015 at 12:47 PM, Michael Welling <mwelling@...e.org> wrote:
> [...]
> >>>>>There is still an issue with the si5351.
> >>>>>
> >>>>>I had to comment out the clk_put here for the frequency to show up:
> >>>>>http://lxr.free-electrons.com/source/drivers/clk/clk-si5351.c#L1133
> >>>>>
> >>>>>Ideas?
> >>>>
> >>>>What is the most recent upstream commit that you are based on?
> >>>
> >>>I am working from 4.0.0-rc7.
> >>>
> >>>7b43b47373d40d557cd7e1a84a0bd8ebc4d745ab
> >>
> >>Hmm, I wonder why si5351 calls clk_put immediately after of_clk_get
> >>in the first place, as far as I understand this destroys the clock
> >>handle, which is still being used later in the code.
> >
> >Not sure how this ever worked. This has been in the code since the
> >initial commit.
>
> The reason it worked before may be related with recent rework of
> clk_put() itself and clk cookies instead of pointers. I lost track on
> the recent clk subsystem changes here, sorry.
>
> However, droping the clk immediately surely isn't right.
> The thing is, we can remove the clk_put() just because there is no
> _remove() for that driver. I remember that back in the days the driver
> was mainlined, clk removal wasn't too easy.
>
> FWIW, as soon as _remove() support will be added by someone, we'll have
> to rethink passing struct clk* by platform_data or at least
> double-check if we ever used [of_]clk_get() to obtain it.
>
> Mind to send a patch removing the clk_put() on !IS_ERR and add a proper
> error path instead? While of_clk_get() is the only calls that need
> cleanup on error in si5351_dt_parse() we should probably move that
> calls to the end of this function. Otherwise we'd also have to cleanup
> on every of_parse_foo() failure.
What would be the proper error path?
What cleanup is required?
It should be noted that there are more deep rooted issues with the driver
that I have noticed. For one the driver behaves differently if the debugging
is on and when it is off.
Here is what the kernel reports with debugging off:
root@...3517-som200:~# cat /sys/kernel/debug/clk/clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------
ref27 0 0 27000000 0 0
xtal 0 0 27000000 0 0
pllb 0 0 599999994 0 0
ms0 0 0 12499999 0 0
clk0 0 0 12499999 0 0
plla 0 0 599999994 0 0
ms2 0 0 8219178 0 0
clk2 0 0 8219178 0 0
ms1 0 0 94117646 0 0
clk1 0 0 94117646 0 0
Here is what the kernel reports with debugging on:
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------
ref27 0 0 27000000 0 0
xtal 0 0 27000000 0 0
pllb 0 0 884736000 0 0
ms0 0 0 18432000 0 0
clk0 0 0 18432000 0 0
plla 0 0 897023997 0 0
ms2 0 0 12287999 0 0
clk2 0 0 12287999 0 0
ms1 0 0 140709646 0 0
clk1 0 0 140709646 0 0
Note this is with the following devicetree entry:
si5351: clock-generator {
#address-cells = <1>;
#size-cells = <0>;
#clock-cells = <1>;
compatible = "silabs,si5351a-msop";
reg = <0x60>;
status = "okay";
/* connect xtal input to 27MHz reference */
clocks = <&ref27>;
/* connect xtal input as source of pll0 and pll1 */
silabs,pll-source = <0 0>, <1 0>;
clkout0: clkout0 {
reg = <0>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <1>;
silabs,clock-source = <0>;
silabs,pll-master;
clock-frequency = <18432000>;
};
clkout1: clkout1 {
reg = <1>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <0>;
silabs,clock-source = <0>;
clock-frequency = <8000000>;
};
clkout2: clkout2 {
reg = <2>;
silabs,drive-strength = <8>;
silabs,multisynth-source = <0>;
silabs,clock-source = <0>;
silabs,pll-master;
clock-frequency = <12288000>;
};
};
I am losing hope that this driver is stable enough to even use in production.
>
> Sebastian
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists