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]
Message-Id: <D5864C0GXLOD.2R2G7Y8CA3T6B@bootlin.com>
Date: Tue, 29 Oct 2024 10:04:48 +0100
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Stephen Boyd" <sboyd@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>,
 "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Michael Turquette"
 <mturquette@...libre.com>, "Rob Herring" <robh@...nel.org>
Cc: <devicetree@...r.kernel.org>, <linux-clk@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "Vladimir Kondratiev"
 <vladimir.kondratiev@...ileye.com>, Grégory Clement
 <gregory.clement@...tlin.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH v5 4/4] clk: eyeq: add driver

On Mon Oct 28, 2024 at 11:52 PM CET, Stephen Boyd wrote:
> Quoting Théo Lebrun (2024-10-24 05:50:16)
> > The bug is elsewhere: we do get valid clocks from PL011. Both clk_get()
> > calls give proper pointers.
> > 
> > The issue is that we are using `compatible = "fixed-factor-clock"`
> > clocks in the middle, and those don't wait for their parents to be
> > active.
> > 
> > Simplified clock graph is: pll-per -> occ-periph.
> > pll-per is register by our driver. occ-periph looks like:
> > 
> >         occ_periph: occ-periph {
> >                 compatible = "fixed-factor-clock";
> >                 clocks = <&olb EQ5C_PLL_PER>;
> >                 #clock-cells = <0>;
> >                 clock-div = <16>;
> >                 clock-mult = <1>;
> >         };
>
> Why is this fixed factor clk registered from DT vs. from the driver that
> registers pll-per? Is it useful to describe it in DT because the factor
> can change? Where does it physically exist? In the SoC?

Those are internal SoC clocks, yes. No reason for them to change from
one board to another. Adding them from the driver makes the most sense,
I'll patch that up.

> > Sequence is:
> >  - eqc_early_init(): it registers a clock provider that will return
> >    EPROBE_DEFER for our pll-per.
> >  - _of_fixed_factor_clk_setup(): it registers occ-periph, even though
> >    its parent is EPROBE_DEFER. clk_core_populate_parent_map() runs all
> >    fine without complaining; logical as it doesn't query the clk_hw for
> >    its parent, it only stores indexes.
> >  - amba_get_enable_pclk(): it does a clk_get() which works because
> >    occ-periph exists.
> > 
> > Maybe __clk_register() should check the clk_hw for each parent: if any
> > is an EPROBE_DEFER then it should EPROBE_DEFER itself? That looks like
> > a rather big behavioral change.
> > 
> > The other solution is to keep as-is: provide all clocks consumed by
> > fixed-factor-clocks at of_clk_init() stage.
>
> Another solution is to register the fixed factor clk from the pll-per
> clk provider.
>
> And yet another solution is to return EPROBE_DEFER for orphaned clks. We
> have everything in place for that but we ran into trouble with consumers
> wanting to get orphaned clks in their probe or during assigned-clocks
> handling.

No suprise this kind of edge-case behavior can cause trouble.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ