[1/2,RFC] clk: reentrancy via per-clock locking

This patch, while incomplete, implements a per-clock locking scheme
intended to enable two specific use cases for the clock framework.  The
changelog is really long; it describes what the patch does, how I came
to this design, a few implementation notes for anyone that wants to try
these patches and some possible different ways to solve the problems.
Any advice or design ideas would be greatly appreciated.

TL;DR

This patch implements per-clock locking, which is horrible and I need
some new ideas.  Please help me mailing list peoples.

		The two use cases:

1) nested calls to the clock framework in clk_prepare/clk_unprepare

Use case #1 is mandatory for preparing clocks on i2c devices.  Calling
clk_prepare on these clocks results in an i2c transaction which will
result in clk_prepare being called for the i2c controller's clocks.
This results in a deadlock.

2) reentrant calls to the clock framework from the rate change notifier
handlers in clk_set_rate

Use case #2 is needed to perform dvfs transitions via clk_set_rate.  It
is common to scale voltage with frequency and it is also common for
voltage to be scaled by an off-chip device.  Similar to use case #1 this
may result in an i2c transaction that calls clk_prepare from within
clk_set_rate.  This results in a deadlock.

		The new locking scheme:

A per-clock locking scheme at first seems like a solution, so long as
reentrant calls do not need to operate on the same clock.  Lockdep uses
the same lock class key for every per-clock lock that was dynamically
initialized in __clk_init() via mutex_init().  To work around this I
stuffed both a struct mutex and a struct lock_class_key into struct clk
and cheated the initialization by directly calling __mutex_init.

@@ -41,6 +41,11 @@ struct clk {
        struct hlist_head       children;
        struct hlist_node       child_node;
        unsigned int            notifier_count;
+       struct mutex            lock;
+       struct lock_class_key   lock_key;
+       struct clk              *top;
+       int                     depth;
+       struct clk              **parent_chain;

...

@@ -1296,6 +1392,16 @@ int __clk_init(struct device *dev, struct clk *clk)
                                break;
                        }

+       /* XXX initialize per-clock mutex */
+       __mutex_init(&clk->lock, clk->name, &clk->lock_key);

Note that this also warns us if the struct clk is not statically
allocated (mine are, odds are that yours are not).  This is gross but it
does generate per-clock locks which lockdep is able to evaluate
independently.

However lock direction must be taken into account; clk_prepare locks
upwards and clk_set_rate locks downwards.  I was unsuccessful in finding
a way to make those operations lock in the same direction without
creating circular locking dependencies.

The circular lock dependency issue above issues led me to try
mutex_lock_nested.  One way to make lockdep happy was to use separate
lock subclasses for clk_prepare and clk_set_rate.  The synaptics
touchscreen guys faced similar issues:
https://lkml.org/lkml/2006/9/14/133

This prevented lockdep from warning about circular dependencies, but
with a nasty side effect: clk_prepare() and clk_set_rate() became
essentially unsynchronized.  This is completely analogous to the racy
mess we have today between clk_prepare and clk_enable (independent mutex
& spinlock, respectively), but now applies to clk_prepare and
clk_set_rate.

Quick aside: one interesting observation here is that if we consider
only use case #2 (i.e. dvfs via reentrant calls to clk framework from
rate change notifier handlers) then the same result could have been
achieved without per-clock locks and instead a new global mutex (e.g.
DEFINE_MUTEX(topology_lock)).  Such a solution does NOT solve nested
calls to clk_prepare (use case #1).

		Some implementation notes:

Many functions in this patch have NOT been updated to use the per-clock
locks, notably clk_set_parent and the clock registration/initialization
path, as well as the debugfs stuff.  Races abound but my platform hasn't
been bitten by any of them in testing.

As mentioned above those with dynamically initialized clocks will get a
warning that the lock class key is not static data.  Since clk_put is
not implemented and we never get rid of clocks (or their locks) then
this is relatively safe to ignore.

		Alternative solutions:

For use case #1: Continue to use the single global clk mutex, but create
a separate clk_prepare_nested or provide a clk flag CLK_PREPARE_NESTED,
or something similar which applies to the i2c controller's struct clk.
Calling clk_prepare_nested or calling clk_prepare on a clk with the
CLK_PREPARE_NESTED flag results in mutex_lock_nested being used on the
global clk mutex.  This is not bomb-proof since there is still missing
context, such as, "I want to nest this call to clk_prepare(x) because it
is a direct dependency of clk_prepare(y)".

For use case #2: a separate api could be created solely for dvfs which
would avoid the reentrancy problem.  A naive example:

int dvfs_scale(struct device *dev, unsigned long rate)
{
	if (rate > old_rate)
		regulator_set_voltage(...);

	clk_set_rate(clk, rate);

	if (rate < old_rate)
		regulator_set_voltage(...);
}

By simply calling regulator_set_rate outside of clk_set_rate then the
reentrancy is avoided.

Not-signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/clk.c           |  202 +++++++++++++++++++++++++++++++------------
 include/linux/clk-private.h |    5 ++
 2 files changed, 154 insertions(+), 53 deletions(-)


[1/2,RFC] clk: reentrancy via per-clock locking

Commit Message

Patch