Common clock framework API vs RT patchset

Common clock framework API vs RT patchset

[Grygorii Strashko]

Hi All,

I'd very appreciated if someone can clarify one point for me.

Is allowed/expected/prohibited to use CLK API like clk_enable/disable
in atomic context now on RT-Kernel (HW IRQ or under RAW spinlocks)?

How safe is it to allow CLK API to be preemptive?

-- 
regards,
-grygorii

Re: Common clock framework API vs RT patchset

[Thomas Gleixner]

On Tue, 4 Aug 2015, Grygorii Strashko wrote:

> Hi All,
> 
> I'd very appreciated if someone can clarify one point for me.
> 
> Is allowed/expected/prohibited to use CLK API like clk_enable/disable
> in atomic context now on RT-Kernel (HW IRQ or under RAW spinlocks)?

It's not possible, if the spinlock in the clock framework is not a raw
spinlock.
 
> How safe is it to allow CLK API to be preemptive?

The functions are protected by the locks. So it's not an issue at all.

Thanks,

	tglx

Re: Common clock framework API vs RT patchset

[Nishanth Menon]

On 08/04/2015 07:06 AM, Thomas Gleixner wrote:
> On Tue, 4 Aug 2015, Grygorii Strashko wrote:
> 
>> Hi All,
>>
>> I'd very appreciated if someone can clarify one point for me.
>>
>> Is allowed/expected/prohibited to use CLK API like clk_enable/disable
>> in atomic context now on RT-Kernel (HW IRQ or under RAW spinlocks)?
> 
> It's not possible, if the spinlock in the clock framework is not a raw
> spinlock.
>  
>> How safe is it to allow CLK API to be preemptive?
> 
> The functions are protected by the locks. So it's not an issue at all.

interesting topic, looping in linux-clk. original thread in [1].

clk apis are protected by spinlocks which in turn should make the clock
apis pre-emptible in RT kernel build.

I think Grygorii's question is "what happens(is it safe) when clock APIs
are pre-empted"? I think it is a bit dependent on the hardware behavior
I think..

Two angles to this:

i) is there an upper time bound in the programming sequence: if any step
x to step y in the sequence takes more than N time, then we can have  a
failure - but then, that is a broken hardware in itself.. Linux (non-RT)
cannot guarantee a strict upper bound either.

ii) if by preempting the operation itself CPU cannot guarantee time
accuracy for scheduling. This can be an issue. more below:

Consider clk_enable/disable/set_parent/setfreq operations. none of these
operations are "atomic" from hardware point of view. instead, they are a
set of steps which culminates to moving from state A to state B of the
clock tree configuration.

specific example could be as follows: changing clock parent in the clock
tree results in kernel tick timer configuration to be in an intermediate
state before going to a final "relocked" state which is expected at the
end of clock setparent call, could be involved in a few steps:
step a) unlock pll (put in bypass)
step b) change parent
step c) relock pll with new parent frequency

if interrupted between (a), (c), we could be in "bypass" frequency or
"new parent" frequency... final frequency is guarenteed only when step
(c) is completed.

In the above example if the clock generated is fundamental to kernel
scheduler accuracy, well, we do have a problem..


I am not sure any of these cases can happen could happen in at least the
hardware I am aware of... not sure if other SoC vendors might have
interesting scenarios..

[1] http://marc.info/?t=143868984600001&r=1&w=2
-- 
Regards,
Nishanth Menon

Re: Common clock framework API vs RT patchset

[Russell King - ARM Linux]

On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote:
> Consider clk_enable/disable/set_parent/setfreq operations. none of these
> operations are "atomic" from hardware point of view. instead, they are a
> set of steps which culminates to moving from state A to state B of the
> clock tree configuration.

There's a world of difference between clk_enable()/clk_disable() and
the rest of the clk API.

clk_enable()/clk_disable() _should_ be callable from any context, since
you may need to enable or disable a clock from any context.  The remainder
of the clk API is callable only from contexts where sleeping is permissible.

The reason we have this split is because clk_enable()/clk_disable() have
historically been used in interrupt handlers, and they're specifically
not supposed to impose big delays.

Things like waiting for a PLL to re-lock is time-consuming, so it's not
something I'd expect to see behind a clk_enable() implementation (the
fact you can't sleep in there is a big hint.)  Such waits should be in
the clk_prepare() stage instead.

Now, as for clk_enable() being interrupted - if clk_enable() is interrupted
and another clk_enable() comes along for the same clock, that second
clk_enable() should not return until the clock has actually been enabled,
and it's up to the implementation to decode how to achieve that.  If that
means a RT implementation using a raw spinlock, then that's one option
(which basically would have the side effect of blocking until the preempted
clk_enable() finishes its business.)  Alternatively, if we can preempt
inside clk_enable(), then the clk_enable() implementation should be written
to cope with that (eg, by the second clk_enable() fiddling with the hardware,
and the first thread noticing that it has nothing to do.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: Common clock framework API vs RT patchset

[Grygorii Strashko]

Hi All,

On 08/04/2015 06:36 PM, Russell King - ARM Linux wrote:
> On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote:
>> Consider clk_enable/disable/set_parent/setfreq operations. none of these
>> operations are "atomic" from hardware point of view. instead, they are a
>> set of steps which culminates to moving from state A to state B of the
>> clock tree configuration.
> 
> There's a world of difference between clk_enable()/clk_disable() and
> the rest of the clk API.
> 
> clk_enable()/clk_disable() _should_ be callable from any context, since
> you may need to enable or disable a clock from any context.  The remainder
> of the clk API is callable only from contexts where sleeping is permissible.
> 
> The reason we have this split is because clk_enable()/clk_disable() have
> historically been used in interrupt handlers, and they're specifically
> not supposed to impose big delays.
> 
> Things like waiting for a PLL to re-lock is time-consuming, so it's not
> something I'd expect to see behind a clk_enable() implementation (the
> fact you can't sleep in there is a big hint.)  Such waits should be in
> the clk_prepare() stage instead.
> 
> Now, as for clk_enable() being interrupted - if clk_enable() is interrupted
> and another clk_enable() comes along for the same clock, that second
> clk_enable() should not return until the clock has actually been enabled,
> and it's up to the implementation to decode how to achieve that.  If that
> means a RT implementation using a raw spinlock, then that's one option
> (which basically would have the side effect of blocking until the preempted
> clk_enable() finishes its business.)  Alternatively, if we can preempt
> inside clk_enable(), then the clk_enable() implementation should be written
> to cope with that (eg, by the second clk_enable() fiddling with the hardware,
> and the first thread noticing that it has nothing to do.)
> 

Thanks a lot for your comments and explanations.

Now lock object in CCF is not a raw spinlock, so, seems, I have to update 
code and try to move clk_enable()/clk_disable() out of atomic context.

-- 
regards,
-grygorii

Re: Common clock framework API vs RT patchset

[Russell King - ARM Linux]

On Tue, Aug 11, 2015 at 10:23:46PM +0300, Grygorii Strashko wrote:
> Hi All,
> 
> On 08/04/2015 06:36 PM, Russell King - ARM Linux wrote:
> > On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote:
> >> Consider clk_enable/disable/set_parent/setfreq operations. none of these
> >> operations are "atomic" from hardware point of view. instead, they are a
> >> set of steps which culminates to moving from state A to state B of the
> >> clock tree configuration.
> > 
> > There's a world of difference between clk_enable()/clk_disable() and
> > the rest of the clk API.
> > 
> > clk_enable()/clk_disable() _should_ be callable from any context, since
> > you may need to enable or disable a clock from any context.  The remainder
> > of the clk API is callable only from contexts where sleeping is permissible.
> > 
> > The reason we have this split is because clk_enable()/clk_disable() have
> > historically been used in interrupt handlers, and they're specifically
> > not supposed to impose big delays.
> > 
> > Things like waiting for a PLL to re-lock is time-consuming, so it's not
> > something I'd expect to see behind a clk_enable() implementation (the
> > fact you can't sleep in there is a big hint.)  Such waits should be in
> > the clk_prepare() stage instead.
> > 
> > Now, as for clk_enable() being interrupted - if clk_enable() is interrupted
> > and another clk_enable() comes along for the same clock, that second
> > clk_enable() should not return until the clock has actually been enabled,
> > and it's up to the implementation to decode how to achieve that.  If that
> > means a RT implementation using a raw spinlock, then that's one option
> > (which basically would have the side effect of blocking until the preempted
> > clk_enable() finishes its business.)  Alternatively, if we can preempt
> > inside clk_enable(), then the clk_enable() implementation should be written
> > to cope with that (eg, by the second clk_enable() fiddling with the hardware,
> > and the first thread noticing that it has nothing to do.)
> > 
> 
> Thanks a lot for your comments and explanations.
> 
> Now lock object in CCF is not a raw spinlock, so, seems, I have to update 
> code and try to move clk_enable()/clk_disable() out of atomic context.

clk_enable/clk_disable _should_ be usable from atomic contexts.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: Common clock framework API vs RT patchset

[Michael Turquette]

Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
> On Tue, Aug 11, 2015 at 10:23:46PM +0300, Grygorii Strashko wrote:
> > Hi All,
> > 
> > On 08/04/2015 06:36 PM, Russell King - ARM Linux wrote:
> > > On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote:
> > >> Consider clk_enable/disable/set_parent/setfreq operations. none of these
> > >> operations are "atomic" from hardware point of view. instead, they are a
> > >> set of steps which culminates to moving from state A to state B of the
> > >> clock tree configuration.
> > > 
> > > There's a world of difference between clk_enable()/clk_disable() and
> > > the rest of the clk API.
> > > 
> > > clk_enable()/clk_disable() _should_ be callable from any context, since
> > > you may need to enable or disable a clock from any context.  The remainder
> > > of the clk API is callable only from contexts where sleeping is permissible.
> > > 
> > > The reason we have this split is because clk_enable()/clk_disable() have
> > > historically been used in interrupt handlers, and they're specifically
> > > not supposed to impose big delays.
> > > 
> > > Things like waiting for a PLL to re-lock is time-consuming, so it's not
> > > something I'd expect to see behind a clk_enable() implementation (the
> > > fact you can't sleep in there is a big hint.)  Such waits should be in
> > > the clk_prepare() stage instead.
> > > 
> > > Now, as for clk_enable() being interrupted - if clk_enable() is interrupted
> > > and another clk_enable() comes along for the same clock, that second
> > > clk_enable() should not return until the clock has actually been enabled,
> > > and it's up to the implementation to decode how to achieve that.  If that
> > > means a RT implementation using a raw spinlock, then that's one option
> > > (which basically would have the side effect of blocking until the preempted
> > > clk_enable() finishes its business.)  Alternatively, if we can preempt
> > > inside clk_enable(), then the clk_enable() implementation should be written
> > > to cope with that (eg, by the second clk_enable() fiddling with the hardware,
> > > and the first thread noticing that it has nothing to do.)
> > > 
> > 
> > Thanks a lot for your comments and explanations.
> > 
> > Now lock object in CCF is not a raw spinlock, so, seems, I have to update 
> > code and try to move clk_enable()/clk_disable() out of atomic context.
> 
> clk_enable/clk_disable _should_ be usable from atomic contexts.

Grygorii,

Note that the common clk implementation allows for the same thread to
re-enter the clock framework even while the lock is held. For instance
if calling clk_enable(foo) resulted in a call to clk_enable(bar), this
would not deadlock. However this re-entrant behavior is ONLY for the
same thread that is already holding the lock.

I doubt that the above bit of trivial will solve your problem and it
probably does not add any new complexity for you either, but it seems
relevant enough for me to add here.

Regards,
Mike

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Common clock framework API vs RT patchset

[Grygorii Strashko]

On 08/12/2015 01:06 AM, Michael Turquette wrote:
> Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
>> On Tue, Aug 11, 2015 at 10:23:46PM +0300, Grygorii Strashko wrote:
>>> Hi All,
>>>
>>> On 08/04/2015 06:36 PM, Russell King - ARM Linux wrote:
>>>> On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote:
>>>>> Consider clk_enable/disable/set_parent/setfreq operations. none of these
>>>>> operations are "atomic" from hardware point of view. instead, they are a
>>>>> set of steps which culminates to moving from state A to state B of the
>>>>> clock tree configuration.
>>>>
>>>> There's a world of difference between clk_enable()/clk_disable() and
>>>> the rest of the clk API.
>>>>
>>>> clk_enable()/clk_disable() _should_ be callable from any context, since
>>>> you may need to enable or disable a clock from any context.  The remainder
>>>> of the clk API is callable only from contexts where sleeping is permissible.
>>>>
>>>> The reason we have this split is because clk_enable()/clk_disable() have
>>>> historically been used in interrupt handlers, and they're specifically
>>>> not supposed to impose big delays.
>>>>
>>>> Things like waiting for a PLL to re-lock is time-consuming, so it's not
>>>> something I'd expect to see behind a clk_enable() implementation (the
>>>> fact you can't sleep in there is a big hint.)  Such waits should be in
>>>> the clk_prepare() stage instead.
>>>>
>>>> Now, as for clk_enable() being interrupted - if clk_enable() is interrupted
>>>> and another clk_enable() comes along for the same clock, that second
>>>> clk_enable() should not return until the clock has actually been enabled,
>>>> and it's up to the implementation to decode how to achieve that.  If that
>>>> means a RT implementation using a raw spinlock, then that's one option
>>>> (which basically would have the side effect of blocking until the preempted
>>>> clk_enable() finishes its business.)  Alternatively, if we can preempt
>>>> inside clk_enable(), then the clk_enable() implementation should be written
>>>> to cope with that (eg, by the second clk_enable() fiddling with the hardware,
>>>> and the first thread noticing that it has nothing to do.)
>>>>
>>>
>>> Thanks a lot for your comments and explanations.
>>>
>>> Now lock object in CCF is not a raw spinlock, so, seems, I have to update
>>> code and try to move clk_enable()/clk_disable() out of atomic context.
>>
>> clk_enable/clk_disable _should_ be usable from atomic contexts.

Thanks Russell - above is not true on -RT.

> 
> Grygorii,
> 
> Note that the common clk implementation allows for the same thread to
> re-enter the clock framework even while the lock is held. For instance
> if calling clk_enable(foo) resulted in a call to clk_enable(bar), this
> would not deadlock. However this re-entrant behavior is ONLY for the
> same thread that is already holding the lock.
> 
> I doubt that the above bit of trivial will solve your problem and it
> probably does not add any new complexity for you either, but it seems
> relevant enough for me to add here.

Thanks Mike.
I'm aware about above feature :) And I understand that CCF is implemented in
thread-safe manner. My problem is that the same part of code
works on vanilla kernel, but might not work on -RT due to locking issues.

Example:
	raw_spin_lock_irqsave(&bank->lock, flags);
	clk_enable(foo);
	  + clk_enable_lock
	   + spin_lock_irqsave (BUG on -RT)
 	<access hw>
	raw_spin_unlock_irqrestore(&bank->lock, flags);
- or - 
HW irq handler:
	clk_enable(bar);

in both cases it will produce
BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:917

This is first question I've asked.

The second one related to the fact that clk_enable/disable API can be
preempted on -RT now in the middle of HW accessing sequence -
from comments in this thread I understood that none know about or can
imaging possible issues related to above behavior. 
So, It's ok for CCF to be preemptive.


-- 
regards,
-grygorii

Re: Common clock framework API vs RT patchset

[Russell King - ARM Linux]

On Wed, Aug 12, 2015 at 01:05:58PM +0300, Grygorii Strashko wrote:
> On 08/12/2015 01:06 AM, Michael Turquette wrote:
> > Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
> >>
> >> clk_enable/clk_disable _should_ be usable from atomic contexts.
> 
> Thanks Russell - above is not true on -RT.

What I'm saying is that it _should_ be true.  You _should_ be able to
call clk_enable()/clk_disable() from atomic contexts.  It's been
documented since forever:

/**
 * clk_enable - inform the system when the clock source should be running.
 * @clk: clock source
 *
 * If the clock can not be enabled/disabled, this should return success.
 *
 * May be called from atomic contexts.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/**
 * clk_disable - inform the system when the clock source is no longer required.
 * @clk: clock source
 *
 * Inform the system that a clock source is no longer required by
 * a driver and may be shut down.
 *
 * May be called from atomic contexts.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If that's not true with CCF, that's a CCF bug, not a usage bug.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: Common clock framework API vs RT patchset

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

On Wed, Aug 12, 2015 at 11:11:51AM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 12, 2015 at 01:05:58PM +0300, Grygorii Strashko wrote:
> > On 08/12/2015 01:06 AM, Michael Turquette wrote:
> > > Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
> > >>
> > >> clk_enable/clk_disable _should_ be usable from atomic contexts.
> > 
> > Thanks Russell - above is not true on -RT.
> 
> What I'm saying is that it _should_ be true.  You _should_ be able to
> call clk_enable()/clk_disable() from atomic contexts.  It's been
> documented since forever:
> 
> /**
>  * clk_enable - inform the system when the clock source should be running.
>  * @clk: clock source
>  *
>  * If the clock can not be enabled/disabled, this should return success.
>  *
>  * May be called from atomic contexts.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> /**
>  * clk_disable - inform the system when the clock source is no longer required.
>  * @clk: clock source
>  *
>  * Inform the system that a clock source is no longer required by
>  * a driver and may be shut down.
>  *
>  * May be called from atomic contexts.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> If that's not true with CCF, that's a CCF bug, not a usage bug.

in that case, CCF's clock need to be converted to raw_spin_locks, that's
the only way to prevent its locks from being reimplemented as rt
mutexes.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Common clock framework API vs RT patchset

[Michael Turquette]

Quoting Felipe Balbi (2015-08-12 08:02:53)
> On Wed, Aug 12, 2015 at 11:11:51AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Aug 12, 2015 at 01:05:58PM +0300, Grygorii Strashko wrote:
> > > On 08/12/2015 01:06 AM, Michael Turquette wrote:
> > > > Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
> > > >>
> > > >> clk_enable/clk_disable _should_ be usable from atomic contexts.
> > > 
> > > Thanks Russell - above is not true on -RT.
> > 
> > What I'm saying is that it _should_ be true.  You _should_ be able to
> > call clk_enable()/clk_disable() from atomic contexts.  It's been
> > documented since forever:
> > 
> > /**
> >  * clk_enable - inform the system when the clock source should be running.
> >  * @clk: clock source
> >  *
> >  * If the clock can not be enabled/disabled, this should return success.
> >  *
> >  * May be called from atomic contexts.
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > /**
> >  * clk_disable - inform the system when the clock source is no longer required.
> >  * @clk: clock source
> >  *
> >  * Inform the system that a clock source is no longer required by
> >  * a driver and may be shut down.
> >  *
> >  * May be called from atomic contexts.
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > If that's not true with CCF, that's a CCF bug, not a usage bug.
> 
> in that case, CCF's clock need to be converted to raw_spin_locks, that's
> the only way to prevent its locks from being reimplemented as rt
> mutexes.

I do not keep up much with rt stuff, so I am going to ask a naive
question: is it common to simply do s/spin_lock/raw_spin_lock/g for
driver subsystems when using rt? Sounds like that is all that is
required...

Regards,
Mike

> 
> -- 
> balbi

Re: Common clock framework API vs RT patchset

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

On Wed, Aug 12, 2015 at 09:46:49AM -0700, Michael Turquette wrote:
> Quoting Felipe Balbi (2015-08-12 08:02:53)
> > On Wed, Aug 12, 2015 at 11:11:51AM +0100, Russell King - ARM Linux wrote:
> > > On Wed, Aug 12, 2015 at 01:05:58PM +0300, Grygorii Strashko wrote:
> > > > On 08/12/2015 01:06 AM, Michael Turquette wrote:
> > > > > Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
> > > > >>
> > > > >> clk_enable/clk_disable _should_ be usable from atomic contexts.
> > > > 
> > > > Thanks Russell - above is not true on -RT.
> > > 
> > > What I'm saying is that it _should_ be true.  You _should_ be able to
> > > call clk_enable()/clk_disable() from atomic contexts.  It's been
> > > documented since forever:
> > > 
> > > /**
> > >  * clk_enable - inform the system when the clock source should be running.
> > >  * @clk: clock source
> > >  *
> > >  * If the clock can not be enabled/disabled, this should return success.
> > >  *
> > >  * May be called from atomic contexts.
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > /**
> > >  * clk_disable - inform the system when the clock source is no longer required.
> > >  * @clk: clock source
> > >  *
> > >  * Inform the system that a clock source is no longer required by
> > >  * a driver and may be shut down.
> > >  *
> > >  * May be called from atomic contexts.
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > If that's not true with CCF, that's a CCF bug, not a usage bug.
> > 
> > in that case, CCF's clock need to be converted to raw_spin_locks, that's
> > the only way to prevent its locks from being reimplemented as rt
> > mutexes.
> 
> I do not keep up much with rt stuff, so I am going to ask a naive
> question: is it common to simply do s/spin_lock/raw_spin_lock/g for
> driver subsystems when using rt? Sounds like that is all that is
> required...

depends, if you want to guarantee your code isn't preemptable, then
yeah.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Common clock framework API vs RT patchset

[Thomas Gleixner]

On Tue, 4 Aug 2015, Russell King - ARM Linux wrote:
> On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote:
> > Consider clk_enable/disable/set_parent/setfreq operations. none of these
> > operations are "atomic" from hardware point of view. instead, they are a
> > set of steps which culminates to moving from state A to state B of the
> > clock tree configuration.
> 
> There's a world of difference between clk_enable()/clk_disable() and
> the rest of the clk API.
> 
> clk_enable()/clk_disable() _should_ be callable from any context, since
> you may need to enable or disable a clock from any context.  The remainder
> of the clk API is callable only from contexts where sleeping is permissible.
> 
> The reason we have this split is because clk_enable()/clk_disable() have
> historically been used in interrupt handlers, and they're specifically
> not supposed to impose big delays.
> 
> Things like waiting for a PLL to re-lock is time-consuming, so it's not
> something I'd expect to see behind a clk_enable() implementation (the
> fact you can't sleep in there is a big hint.)  Such waits should be in
> the clk_prepare() stage instead.

You wish. Drivers with loop/udelays in the enable/disable callbacks:

   drivers/clk/keystone/gate.c::keystone_clk_enable() -> psc_config()
   drivers/clk/shmobile/clk-mstp.c::cpg_mstp_clock_endisable()
   drivers/clk/qcom/clk-branch.c::clk_branch_enable() -> clk_branch_wait()
   drivers/clk/qcom/clk-pll.c::clk_pll_vote_enable() -> wait_for_pll()
   drivers/clk/ti/fapll.c::ti_fapll_enable() -> ti_fapll_wait_lock()
   drivers/clk/mmp/clk-gate.c::mmp_clk_gate_enable()
   drivers/clk/tegra/clk-pll.c::clk_pll_enable() -> clk_pll_wait_for_lock()
   drivers/clk/tegra/clk-pll.c::clk_plle_enable() -> clk_plle_training() [1]
   drivers/clk/zynq/pll.c::zynq_pll_enable()
   drivers/clk/ux500/clk-prcc.c::clk_prcc_pclk_enable()
   drivers/clk/clk-nomadik.c::src_clk_enable()
   drivers/clk/samsung/clk-pll.c::samsung_s3c2410_pll_enable()
   drivers/clk/sirf/clk-common.c::usb_pll_clk_enable()
   drivers/clk/st/clkgen-fsyn.c::quadfs_pll_enable()
   drivers/clk/st/clkgen-mux.c::clkgena_divmux_enable()
   drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_pll.c::mpd4_lvds_pll_enable()
   drivers/gpu/drm/msm/hdmi/hdmi_phy_8960.c::hdmi_pll_enable()
   
   [1] Worst case: timeout = jiffies + msecs_to_jiffies(100);

I'm sure that I missed quite some, but the above is horrible enough.

> Now, as for clk_enable() being interrupted - if clk_enable() is interrupted
> and another clk_enable() comes along for the same clock, that second
> clk_enable() should not return until the clock has actually been enabled,
> and it's up to the implementation to decode how to achieve that.  If that
> means a RT implementation using a raw spinlock, then that's one option
> (which basically would have the side effect of blocking until the preempted
> clk_enable() finishes its business.)  Alternatively, if we can preempt
> inside clk_enable(), then the clk_enable() implementation should be written
> to cope with that (eg, by the second clk_enable() fiddling with the hardware,
> and the first thread noticing that it has nothing to do.)

clk_enable() and clk_disable() take the enable_lock, so that's not
different in RT from !RT. The second caller to clk_enable() has to
wait for the first one to finish.

Now what's different in RT is that the enable_lock gets converted to a
'sleeping spinlock', which is again fine to take from interrupt
handlers as interrupt handlers are forced into threads on RT and are
preemptible.

The problem Grygorii is seeing is when code, which runs in atomic
context even on RT (demux handlers, interrupt disabled sections ..)
calls clk_en/disable. That results obviously in a
might_sleep/scheduling while atomic splat.

Of course we could solve that by making enable_lock a raw_spinlock,
but looking at the various implementations of clk_ops.enable tells me
that this is not a brilliant idea. See the PLL loops/delays crap
above. There is another issue:

Some callbacks have their own spinlocks which then need to be
converted to raw_spinlocks as well. Not a big deal, but some of the
clk drivers use that very same spinlock, which is supposed to protect
register access, for all kind of other crap, which is going to
introduce latencies. And that's a ratsnest of locks down to
regmap->lock ....

So for RT the only sensible choice at the moment is to leave
enable_lock as non raw spinlock and deal with the very few places
where clk_enable/disable() is really called from atomic context.

Thanks,

	tglx

Re: Common clock framework API vs RT patchset

[Russell King - ARM Linux]

On Mon, Sep 21, 2015 at 03:06:18PM +0200, Thomas Gleixner wrote:
> On Tue, 4 Aug 2015, Russell King - ARM Linux wrote:
> > On Tue, Aug 04, 2015 at 10:23:31AM -0500, Nishanth Menon wrote:
> > > Consider clk_enable/disable/set_parent/setfreq operations. none of these
> > > operations are "atomic" from hardware point of view. instead, they are a
> > > set of steps which culminates to moving from state A to state B of the
> > > clock tree configuration.
> > 
> > There's a world of difference between clk_enable()/clk_disable() and
> > the rest of the clk API.
> > 
> > clk_enable()/clk_disable() _should_ be callable from any context, since
> > you may need to enable or disable a clock from any context.  The remainder
> > of the clk API is callable only from contexts where sleeping is permissible.
> > 
> > The reason we have this split is because clk_enable()/clk_disable() have
> > historically been used in interrupt handlers, and they're specifically
> > not supposed to impose big delays.
> > 
> > Things like waiting for a PLL to re-lock is time-consuming, so it's not
> > something I'd expect to see behind a clk_enable() implementation (the
> > fact you can't sleep in there is a big hint.)  Such waits should be in
> > the clk_prepare() stage instead.
> 
> You wish. Drivers with loop/udelays in the enable/disable callbacks:

Most of what I've said above is entirely factual.  What idiotic games
people play inside clk_enable() is not my problem (I'm not even the CCF
maintainer - something which Linaro "took over".)

> Of course we could solve that by making enable_lock a raw_spinlock,
> but looking at the various implementations of clk_ops.enable tells me
> that this is not a brilliant idea. See the PLL loops/delays crap
> above. There is another issue:
> 
> Some callbacks have their own spinlocks which then need to be
> converted to raw_spinlocks as well. Not a big deal, but some of the
> clk drivers use that very same spinlock, which is supposed to protect
> register access, for all kind of other crap, which is going to
> introduce latencies. And that's a ratsnest of locks down to
> regmap->lock ....
> 
> So for RT the only sensible choice at the moment is to leave
> enable_lock as non raw spinlock and deal with the very few places
> where clk_enable/disable() is really called from atomic context.

Right.  Let's put it in the most direct and blunt way possible.  Those
responsible for this mess won't like it.  They're not _meant_ to like
this statement, because they're supposed to feel bad about the situation
they've created.  Maybe, in the face of this leve of humiliation, they'll
assess whether they _should_ be doing better, and try to step up their
game.

We have:

clk_enable() & clk_disable() - which are _supposed_ to be the atomic
  level operators, which take a spinlock and are supposed to be _fast_.

clk_prepare() & clk_unprepare() - which are _supposed_ to be the
  time consuming bits of clk_enable()/clk_disable()

However, due to the generally idiotic nature of Linux programmers, they've
totally destroyed the reason for clk_enable() and clk_disable() existing.
So, we now need a _third_ level of API to work around their idiotic nature,
maybe clk_atomic_enable() and clk_atomic_disable(), which do what the
original clk_enable() and clk_disable() do.

So, we end up needing three levels because Linux programmers are basically
idiots.

And yes, if what you've found is correct, I think those who have created
the crap have very much earned the prestigious title of being an "idiotic
programmer".

I'm pissed at these idiotic programmers.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Common clock framework API vs RT patchset\

[Thomas Gleixner]

On Mon, 21 Sep 2015, Russell King - ARM Linux wrote:
> On Mon, Sep 21, 2015 at 03:06:18PM +0200, Thomas Gleixner wrote:
> However, due to the generally idiotic nature of Linux programmers, they've
> totally destroyed the reason for clk_enable() and clk_disable() existing.
> So, we now need a _third_ level of API to work around their idiotic nature,
> maybe clk_atomic_enable() and clk_atomic_disable(), which do what the
> original clk_enable() and clk_disable() do.
> 
> So, we end up needing three levels because Linux programmers are basically
> idiots.
> 
> And yes, if what you've found is correct, I think those who have created
> the crap have very much earned the prestigious title of being an "idiotic
> programmer".
> 
> I'm pissed at these idiotic programmers.

I'm even more pissed at the CCF "maintainers" for giving a shit about
the sanity of the stuff they are maintaining.

One reason for this mess is that clk related patches go often
unreviewed through SoC trees, which again is a problem of the
maintainers who just let that happen.

It's not enough to have an entry in MAINTAINERS along with a git tree
in which random patches get collected.

This 'shove crap in no matter what' mentality needs to stop if we
don't want to end up with a completely unmaintainable nightmare.

Thanks,

	tglx