Common clock framework API vs RT patchset
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?
Signature · 2 lines
regards, -grygorii
Quoted 6 lines · 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)?
It's not possible, if the spinlock in the clock framework is not a raw spinlock.
Quoted 1 line
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
Quoted 9 lines · Thomas Gleixner
Quoted 6 lines · 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)?
Quoted 6 lines
It's not possible, if the spinlock in the clock framework is not a raw spinlock.
Quoted 1 line
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
Signature · 2 lines
Regards, Nishanth Menon
Quoted 4 lines · Nishanth Menon
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.)
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Hi All,
Quoted 22 lines · Russell King - ARM Linux
Quoted 4 lines · Nishanth Menon
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 theclk_prepare()stage instead.
Quoted 11 lines
Now, as for
clk_enable()being interrupted - ifclk_enable()is interrupted and anotherclk_enable()comes along for the same clock, that secondclk_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 preemptedclk_enable()finishes its business.) Alternatively, if we can preempt insideclk_enable(), then theclk_enable()implementation should be written to cope with that (eg, by the secondclk_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.
Signature · 2 lines
regards, -grygorii
Quoted 37 lines · Grygorii Strashko
Hi All,
Quoted 22 lines · Russell King - ARM Linux
Quoted 4 lines · Nishanth Menon
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 theclk_prepare()stage instead.Quoted 11 lines
Now, as for
clk_enable()being interrupted - ifclk_enable()is interrupted and anotherclk_enable()comes along for the same clock, that secondclk_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 preemptedclk_enable()finishes its business.) Alternatively, if we can preempt insideclk_enable(), then theclk_enable()implementation should be written to cope with that (eg, by the secondclk_enable()fiddling with the hardware, and the first thread noticing that it has nothing to do.)
Quoted 4 lines
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.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
Quoted 7 lines · Grygorii Strashko
Quoted 6 lines
Hi All,
=Quoted 2 lines · Russell King - ARM Linux
Quoted 1 line · Nishanth Menon
Consider clk_enable/disable/set_parent/setfreq operations. none of t=
hese
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
operations are "atomic" from hardware point of view. instead, they a=
re a
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
set of steps which culminates to moving from state A to state B of t=
he
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
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, sin=
ce
Quoted 1 line
Quoted 1 line
Quoted 1 line
you may need to enable or disable a clock from any context. The rema=
inder
Quoted 1 line
Quoted 1 line
Quoted 1 line
of the clk API is callable only from contexts where sleeping is permi=
ssible.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
=
The reason we have this split is because clk_enable()/clk_disable() h=
ave
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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 n=
ot
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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 theclk_prepare()stage instead.
=Now, as for
clk_enable()being interrupted - ifclk_enable()is inter=
rupted
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
and another
clk_enable()comes along for the same clock, that secondclk_enable()should not return until the clock has actually been enab=
led,
Quoted 1 line
Quoted 1 line
Quoted 1 line
and it's up to the implementation to decode how to achieve that. If =
that
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
means a RT implementation using a raw spinlock, then that's one option (which basically would have the side effect of blocking until the pre=
empted
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
clk_enable()finishes its business.) Alternatively, if we can preempt insideclk_enable(), then theclk_enable()implementation should be w=
ritten
Quoted 1 line
Quoted 1 line
Quoted 1 line
to cope with that (eg, by the second
clk_enable()fiddling with the h=
ardware,
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
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 upda=
te =
Quoted 4 lines
Quoted 1 line
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
Quoted 10 lines
=
-- =
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 45 lines · Michael Turquette
Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
Quoted 44 lines · Grygorii Strashko
Quoted 37 lines
Hi All,
Quoted 22 lines · Russell King - ARM Linux
Quoted 4 lines · Nishanth Menon
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 theclk_prepare()stage instead.Quoted 11 lines
Now, as for
clk_enable()being interrupted - ifclk_enable()is interrupted and anotherclk_enable()comes along for the same clock, that secondclk_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 preemptedclk_enable()finishes its business.) Alternatively, if we can preempt insideclk_enable(), then theclk_enable()implementation should be written to cope with that (eg, by the secondclk_enable()fiddling with the hardware, and the first thread noticing that it has nothing to do.)Quoted 4 lines
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.
Quoted 9 lines
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 toclk_enable(bar), this would not deadlock. However this re-entrant behavior is ONLY for the same thread that is already holding the lock.
Quoted 3 lines
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.
Signature · 2 lines
regards, -grygorii
Quoted 6 lines · Grygorii Strashko
Quoted 3 lines · Michael Turquette
Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
Quoted 2 lines
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.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
Quoted 31 lines · Russell King - ARM Linux
Quoted 6 lines · Grygorii Strashko
Quoted 3 lines · Michael Turquette
Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
Quoted 2 lines
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.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 1 line
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.
Signature · 1 line
balbi
Quoting Felipe Balbi (2015-08-12 08:02:53)
Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Grygorii Strashko
Quoted 7 lines · Michael Turquette
Quoted 3 lines
Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
Quoted 2 lines
clk_enable/clk_disable _should_ be usable from atomic contexts.
=
Thanks Russell - above is not true on -RT.
=
Quoted 7 lines
Quoted 7 lines
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 runni=
ng.
Quoted 10 lines
Quoted 10 lines
* @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 r=
equired.
Quoted 9 lines
Quoted 8 lines
* @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.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
=
Quoted 6 lines
Quoted 1 line
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
Quoted 5 lines
=
-- =
balbi
Quoted 35 lines · Michael Turquette
Quoting Felipe Balbi (2015-08-12 08:02:53)
Quoted 34 lines · Russell King - ARM Linux
Quoted 32 lines · Grygorii Strashko
Quoted 6 lines · Michael Turquette
Quoted 3 lines
Quoting Russell King - ARM Linux (2015-08-11 12:25:15)
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
Signature · 1 line
balbi
Quoted 13 lines · Russell King - ARM Linux
Quoted 4 lines · Nishanth Menon
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.
Quoted 8 lines
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 theclk_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.
Quoted 10 lines
Now, as for
clk_enable()being interrupted - ifclk_enable()is interrupted and anotherclk_enable()comes along for the same clock, that secondclk_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 preemptedclk_enable()finishes its business.) Alternatively, if we can preempt insideclk_enable(), then theclk_enable()implementation should be written to cope with that (eg, by the secondclk_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
Quoted 18 lines · Thomas Gleixner
Quoted 17 lines · Russell King - ARM Linux
Quoted 4 lines · Nishanth Menon
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.
Quoted 6 lines
Quoted 4 lines
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 theclk_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".)
Quoted 12 lines
Of course we could solve that by making
enable_lockaraw_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_spinlocksas 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 toregmap->lock....
Quoted 3 lines
So for RT the only sensible choice at the moment is to leave
enable_lockas 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.
Signature · 2 lines
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\
Quoted 7 lines · Russell King - ARM Linux
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 forclk_enable()andclk_disable()existing. So, we now need a _third_ level of API to work around their idiotic nature, maybeclk_atomic_enable()andclk_atomic_disable(), which do what the originalclk_enable()andclk_disable()do.
Quoted 8 lines
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