linux-kernel - Re: clk: Per controller locks (prepare & enable)

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:	Thu, 7 Jul 2016 17:00:23 +0100
From:	Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
CC:	Javier Martinez Canillas <javier@....samsung.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	<linux-clk@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Tomasz Figa <tomasz.figa@...il.com>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Andrzej Hajda <a.hajda@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: clk: Per controller locks (prepare & enable)

On Thu, Jul 07, 2016 at 02:42:17PM +0200, Krzysztof Kozlowski wrote:
> On 07/07/2016 02:06 PM, Charles Keepax wrote:
> > On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote:
> >> Hello Krzysztof,
> >>
> >> On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote:
> >>> On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote:
> > I have also been have a brief look at this as we have been
> > encountering issues attempting to move some of the clocking on
> > our audio CODECs to the clock framework. The problems are even
> > worse when the device can be controlled over SPI as well, as the
> > SPI framework may occasionally defer the transfer to a worker
> > thread rather than doing it in the same thread which causes the
> > re-enterant behaviour of the clock locking to no longer function.
> 
> As you mentioned later, in such case per-controller-lock won't help.
> 

It should help as the SPI clocks and the (in this case) CODEC
clocks are unlikely to be on the same controller.

> > I could perhaps imagine a situation where one device is passing
> > a clock to second device and that device is doing some FLL/PLL
> > and passing the resulting clock back. For example supplying a
> > non-audio rate clock to a CODEC which then supplies back a clock
> > at an audio rate, which is used for audio functions on the first
> > device.
> 
> What do you think by "passing" here? Pass the pointer to struct?
> 

Apologies for being unclear there, I was really just referring to
where the source for each clock is coming from. Given controllers
C1 and C2, and putting the clock in brackets afterwards:

C1(MCLK@...Hz) is the parent of C2(FLL@...576MHz) which is the parent
of C1(AUDIO@...576MHz). Which makes C2 both a parent and child of
C1. Its probably not that likely but I could see it happening.

> > I had also been leaning more towards a lock per clock rather
> > than a lock per controller. But one other issue that needs to be
> > kept in mind (with both the controller or clock based locking)
> > through is that the enable and prepare operations propagate down
> > the clock tree, where as the set rate operations propagate up the
> > clock tree.  This makes things a rather furtile breeding ground
> > for mutex inversions as well.
> > 
> 
> Yeah, that is the problem we were thinking about just a sec ago. :) The
> set rate (and reparent which might cause set rate) is complicating the
> design.
> 
> Idea I have is (simplifying only to prepare lock... leave away the enable):

Certainly I think only worrying about prepare makes sense.

> 1. Hava a global lock which will protect:
> a. traversing clock controller hierarchy,
> b. acquiring per clock controller locks,
> 2. Add struct for clock controller.
> 3. Add lock per clock controller.
> 
> The basic locking in case of prepare for a simplified case one clock per
> clock controller:
> 
> A (top controller = top clock)
> \-B
>   \-C
> 
> clk_prepare(C) {
>   global_lock();
>   for (clk_ctrl = C) {
>     lock(clk_ctrl);
>     clk_ctrl = get_parent_controller(C);
>   }
>   global_unlock();
> 
>   prepare_cnt++;
>   // do the same for hierarchy
> 
>   for (clk_ctrl = C) {
>     unlock(clk_ctrl)
>     clock = get_parent_controller(C);
>   }
> }

I think this fixes the issues I have been having at my side. I
will try to find some time in the next few days to go through and
refresh my memory.

I guess lets wait and see if the clock guys have any thoughts.

Thanks,
Charles