[lkml]   [2011]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: Locking in the clk API
On 01/12/2011 10:03 AM, Russell King - ARM Linux wrote:

> That never has been, and that is called for the _system_ going into
> suspend. That's not what I'm talking about. I'm talking about drivers
> doing their own power management in response to (a) their knowledge of
> how the device behaves and (b) in response to the user state they know.
> In the case of a UART, that means enabling the clock when the user opens
> the device and turning it off when the user closes the device - and in
> the case of the UART being used as the system console, enabled when
> printk calls it, disabled when finished outputting the message.


> The latter is a case where you're called in atomic context.

This feels a bit like perfect being the enemy of good.

On platforms that need to sleep to enable the UART clock, configuring
the UART as the kernel console should be equivalent to userspace opening
the UART device, i.e. enable the clock. At least to me that feels like
an acceptable tradeoff, and if I wanted to save the last bit of power
I'll have to refrain from using UART as the kernel console.

If both printk to the console and disabling the clock is really really
neccesary, add a clk_enable_busywait, but that will be a bit of a hack.

> Another case - a storage device. While you may receive open/close calls,
> these are meaningless for power management - you'll receive an open call
> when you mount a filesystem, and a close call when you finally unmount it.
> That doesn't mean it's going to be used. Your request handler will
> generally run in an atomic context, so in order to do dynamic power
> saving you need to be able to enable/disable clocks in that context.

> A touchscreen controller which you communicate with over a dedicated bus.
> The touchscreen controller doesn't require the host to be constantly
> clocked - it only needs to be clocked when communicating with the
> touchscreen. The touchscreen controller raises an interrupt for service.
> You'd want to enable the clock in the interrupt handler to communicate
> with the device and turn it off afterwards.

Both of these feel like they should use a call such as clk_get_atomic
and be able to handle EWOULDBLOCK/EAGAIN (or whatever error code is used
to indicate that it would have to sleep) and delegate to a worker thread
to enable the clock. To catch uses of plain clk_enable from atomic
contects, add a WARN_ON/BUG_ON(in_atomic()). It won't catch everything,
but would help a bit at least.

Someone suggested splitting clk_enable into a part that can sleep and a
part that can't, would that be workable? I.e. extend clk_get so that it
knows what requirements the driver has on the clock. So a driver that does:

clk_get(dev, "my_clk", CLK_CANSLEEP);

must then either use clk_enable from contexts which can sleep or use
clk_enable_atomic and be able to handle EWOULDBLOCK.

A driver which can't handle EWOULDBLOCK would do:

clk_get(dev, "my_clk", CLK_ATOMIC)

informing the clock subsystem that clk_enable_atomic must always
succeed. If the clock source driver can't do that it has to enable the
clock at clk_get time instead of at clk_enable time. If a clock
requires a potentially slow PLL setup which needs to sleep but can gate
the clock atomically, do the PLL setup from clk_get and the gating from
clk_enable. This means that a clock might be on when it strictly isn't
necessary, but at least it will be correct and assuming that Peter Mundt
is correct in saying that the sleeping clock case is a corner case this
will usually not be a problem.

For the corner cases where someone ports a driver that uses CLK_ATOMIC
to a system with sleeping clocks and wants the last bit of power saving,
the burden is on that someone to add EWOULDBLOCK support to the driver.

Regarding the other functions in the clock API, generic code in
clk_disable_atomic could check if it is a sleeping clock and based on
that call clk_disable directly or delegate the call to a worker thread.
All other functions should be able to sleep, with the possible
exception of clk_get_rate which could be useful so that a driver can
check if the clock is running at the correct rate from atomic context
and delegate to a worker thread to change the clock rate if it is not.

To avoid unnecessary code churn it might be better to say that plain
clk_enable is the atomic variant and if it is a sleeping clock it will
be enbled at the time plain clk_get is called. People who want to use
sleeping clocks can then modify a driver at a time to use
clk_get_cansleep and clk_enable_cansleep. But I must say that the name
clk_enable_atomic feels a lot cleaner.


 \ /
  Last update: 2011-01-15 15:13    [W:0.159 / U:0.092 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site