[4.1.3-rt10][RFC PATCH] clk: use raw locks for locking

From: Grygorii Strashko
Date: Tue Oct 20 2015 - 16:59:47 EST

Hi Mike, All,

[not for merge]

As we discussed I've prepared patch which introduces config option
COMMON_CLK_USE_RAW_LOCKS which, once enabled, switches CCF to use raw locks
for lockingâ. This way it will be possible to call clk_enable()/clk_disable()
from atomic context on -RT [1].
Unfortunately (and as expected [2]), if COMMON_CLK_USE_RAW_LOCKS is enebled
it starts raising issues with TI's clock drivers (and this is like a avalanche):
- some TI drivers calls platform specific code which, in turn, uses spinlocks;
- one driver is implemented using MMIO regmap which uses spinlocks.

As result, to get a complete solution I've had to make more patches:
regmap: use raw locks for locking
ARM: OMAP2+: powerdomain: use raw locks for locking
clk: ti: drop locking code from mux/divider drivers
clk: use raw locks for locking

This solution requires the use of raw locks in many places of kernel,
and it's bad for the -RT. For example, regmap is used by only one TI's clock,
but after switching to use raw locks it will affect also on all drivers which
use 'syscon'.

So, it seems that in many cases it might be more simple to just move
clk_enable()/clk_disable() calls out of atomic context :)

FYI: Complete set of patches (based on v4.1.10-rt10) can be found at:
- git@xxxxxxxxxx:~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git
- branch: linux-4.1.y-rt-10-clk-raw-lock

[1] http://marc.info/?l=linux-rt-users&m=143937432529680&w=2
[2] http://marc.info/?l=linux-rt-users&m=144284086804316&w=2

---------------------------------------------------------------------
This patch optionally allows CCF core to use raw locks in clk_enable()/
clk_disable() path. Also, convert generic clock drivers in
the similar way (clk-divider, clk-fractional-divider,
clk-gate, clk-mux).

This patch introduces Kconfig option COMMON_CLK_USE_RAW_LOCKS.
Once enabled, COMMON_CLK_USE_RAW_LOCKS will switch the common clock
framework to use raw locks for locking and, this way, makes it
possible to call clk_enable()/clk_disable() from atomic context.

This fixes backtrace like:
BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:917
in_atomic(): 1, irqs_disabled(): 128, pid: 128, name: sh
4 locks held by sh/128:
#0: (sb_writers#4){.+.+.+}, at: [<c019d44c>] vfs_write+0x13c/0x164
#1: (&of->mutex){+.+.+.}, at: [<c0214bbc>] kernfs_fop_write+0x48/0x19c
#2: (s_active#33){.+.+.+}, at: [<c0214bc4>] kernfs_fop_write+0x50/0x19c
#3: (&bank->lock){......}, at: [<c0405424>] omap_gpio_debounce+0x1c/0x120
irq event stamp: 3532
hardirqs last enabled at (3531): [<c0186ffc>] kfree+0xf8/0x464
hardirqs last disabled at (3532): [<c06a0ef0>] _raw_spin_lock_irqsave+0x1c/0x54
softirqs last enabled at (0): [<c004572c>] copy_process.part.54+0x3f4/0x1930
softirqs last disabled at (0): [< (null)>] (null)
Preemption disabled at:[< (null)>] (null)

CPU: 1 PID: 128 Comm: sh Not tainted 4.1.9-rt8-01685-g0660ad10-dirty #39
Hardware name: Generic DRA74X (Flattened Device Tree)
[<c0019148>] (unwind_backtrace) from [<c00144b0>] (show_stack+0x10/0x14)
[<c00144b0>] (show_stack) from [<c069b4dc>] (dump_stack+0x7c/0x98)
[<c069b4dc>] (dump_stack) from [<c06a148c>] (rt_spin_lock+0x20/0x60)
[<c06a148c>] (rt_spin_lock) from [<c056cb6c>] (clk_enable_lock+0x14/0xb0)
[<c056cb6c>] (clk_enable_lock) from [<c056fed8>] (clk_enable+0xc/0x2c)
[<c056fed8>] (clk_enable) from [<c0405474>] (omap_gpio_debounce+0x6c/0x120)
[<c0405474>] (omap_gpio_debounce) from [<c0404034>] (export_store+0x70/0xfc)
[<c0404034>] (export_store) from [<c0214c2c>] (kernfs_fop_write+0xb8/0x19c)
[<c0214c2c>] (kernfs_fop_write) from [<c019cb00>] (__vfs_write+0x20/0xd8)
[<c019cb00>] (__vfs_write) from [<c019d3a0>] (vfs_write+0x90/0x164)
[<c019d3a0>] (vfs_write) from [<c019dbc4>] (SyS_write+0x44/0x9c)
[<c019dbc4>] (SyS_write) from [<c0010300>] (ret_fast_syscall+0x0/0x54)
EXT4-fs (mmcblk1p2): error count since last fsck: 2
EXT4-fs (mmcblk1p2): initial error at time 1437484540: ext4_put_super:789
EXT4-fs (mmcblk1p2): last error at time 1437484540: ext4_put_super:789

Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
---
drivers/clk/Kconfig | 11 +++++++++++
drivers/clk/clk-divider.c | 10 +++++-----
drivers/clk/clk-fractional-divider.c | 10 +++++-----
drivers/clk/clk-gate.c | 6 +++---
drivers/clk/clk-mux.c | 8 ++++----
drivers/clk/clk.c | 12 +++++++++---
include/linux/clk-provider.h | 38 ++++++++++++++++++++++++++----------
7 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9897f35..458489d 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -24,6 +24,17 @@ config COMMON_CLK
menu "Common Clock Framework"
depends on COMMON_CLK

+config COMMON_CLK_USE_RAW_LOCKS
+ bool "Use raw locks for locking on -rt (EXPERIMENTAL)"
+ default n
+ depends on PREEMPT_RT_FULL
+ ---help---
+ This option switches the common clock framework to use raw locks
+ for locking and, this way, makes it possible to call
+ clk_enable()/clk_disable() from atomic context.
+ If unsure, do not use as it may affect on overall system stability
+ and -RT latencies.
+
config COMMON_CLK_WM831X
tristate "Clock driver for WM831x/2x PMICs"
depends on MFD_WM831X
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 25006a8..66ad38a 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -388,7 +388,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
divider->width, divider->flags);

if (divider->lock)
- spin_lock_irqsave(divider->lock, flags);
+ clk_spin_lock_irqsave(divider->lock, flags);

if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
val = div_mask(divider->width) << (divider->shift + 16);
@@ -400,7 +400,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
clk_writel(val, divider->reg);

if (divider->lock)
- spin_unlock_irqrestore(divider->lock, flags);
+ clk_spin_unlock_irqrestore(divider->lock, flags);

return 0;
}
@@ -416,7 +416,7 @@ static struct clk *_register_divider(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
u8 clk_divider_flags, const struct clk_div_table *table,
- spinlock_t *lock)
+ clk_spinlock_t *lock)
{
struct clk_divider *div;
struct clk *clk;
@@ -475,7 +475,7 @@ static struct clk *_register_divider(struct device *dev, const char *name,
struct clk *clk_register_divider(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
- u8 clk_divider_flags, spinlock_t *lock)
+ u8 clk_divider_flags, clk_spinlock_t *lock)
{
return _register_divider(dev, name, parent_name, flags, reg, shift,
width, clk_divider_flags, NULL, lock);
@@ -500,7 +500,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
u8 clk_divider_flags, const struct clk_div_table *table,
- spinlock_t *lock)
+ clk_spinlock_t *lock)
{
return _register_divider(dev, name, parent_name, flags, reg, shift,
width, clk_divider_flags, table, lock);
diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 6aa72d9..9e4e1a1 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -26,12 +26,12 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
u64 ret;

if (fd->lock)
- spin_lock_irqsave(fd->lock, flags);
+ clk_spin_lock_irqsave(fd->lock, flags);

val = clk_readl(fd->reg);

if (fd->lock)
- spin_unlock_irqrestore(fd->lock, flags);
+ clk_spin_unlock_irqrestore(fd->lock, flags);

m = (val & fd->mmask) >> fd->mshift;
n = (val & fd->nmask) >> fd->nshift;
@@ -79,7 +79,7 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
n = parent_rate / div;

if (fd->lock)
- spin_lock_irqsave(fd->lock, flags);
+ clk_spin_lock_irqsave(fd->lock, flags);

val = clk_readl(fd->reg);
val &= ~(fd->mmask | fd->nmask);
@@ -87,7 +87,7 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
clk_writel(val, fd->reg);

if (fd->lock)
- spin_unlock_irqrestore(fd->lock, flags);
+ clk_spin_unlock_irqrestore(fd->lock, flags);

return 0;
}
@@ -102,7 +102,7 @@ EXPORT_SYMBOL_GPL(clk_fractional_divider_ops);
struct clk *clk_register_fractional_divider(struct device *dev,
const char *name, const char *parent_name, unsigned long flags,
void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
- u8 clk_divider_flags, spinlock_t *lock)
+ u8 clk_divider_flags, clk_spinlock_t *lock)
{
struct clk_fractional_divider *fd;
struct clk_init_data init;
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 3f0e420..a77cce6 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -51,7 +51,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
set ^= enable;

if (gate->lock)
- spin_lock_irqsave(gate->lock, flags);
+ clk_spin_lock_irqsave(gate->lock, flags);

if (gate->flags & CLK_GATE_HIWORD_MASK) {
reg = BIT(gate->bit_idx + 16);
@@ -69,7 +69,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
clk_writel(reg, gate->reg);

if (gate->lock)
- spin_unlock_irqrestore(gate->lock, flags);
+ clk_spin_unlock_irqrestore(gate->lock, flags);
}

static int clk_gate_enable(struct clk_hw *hw)
@@ -121,7 +121,7 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);
struct clk *clk_register_gate(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 bit_idx,
- u8 clk_gate_flags, spinlock_t *lock)
+ u8 clk_gate_flags, clk_spinlock_t *lock)
{
struct clk_gate *gate;
struct clk *clk;
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 69a094c..c4acb55 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -84,7 +84,7 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
}

if (mux->lock)
- spin_lock_irqsave(mux->lock, flags);
+ clk_spin_lock_irqsave(mux->lock, flags);

if (mux->flags & CLK_MUX_HIWORD_MASK) {
val = mux->mask << (mux->shift + 16);
@@ -96,7 +96,7 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
clk_writel(val, mux->reg);

if (mux->lock)
- spin_unlock_irqrestore(mux->lock, flags);
+ clk_spin_unlock_irqrestore(mux->lock, flags);

return 0;
}
@@ -116,7 +116,7 @@ EXPORT_SYMBOL_GPL(clk_mux_ro_ops);
struct clk *clk_register_mux_table(struct device *dev, const char *name,
const char **parent_names, u8 num_parents, unsigned long flags,
void __iomem *reg, u8 shift, u32 mask,
- u8 clk_mux_flags, u32 *table, spinlock_t *lock)
+ u8 clk_mux_flags, u32 *table, clk_spinlock_t *lock)
{
struct clk_mux *mux;
struct clk *clk;
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(clk_register_mux_table);
struct clk *clk_register_mux(struct device *dev, const char *name,
const char **parent_names, u8 num_parents, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
- u8 clk_mux_flags, spinlock_t *lock)
+ u8 clk_mux_flags, clk_spinlock_t *lock)
{
u32 mask = BIT(width) - 1;

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9f9cadd..6eb7f8a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -24,7 +24,12 @@

#include "clk.h"

+#if defined(CONFIG_COMMON_CLK_USE_RAW_LOCKS)
+static DEFINE_RAW_SPINLOCK(enable_lock);
+#else /* PREEMPT_RT_FULL */
static DEFINE_SPINLOCK(enable_lock);
+#endif
+
static DEFINE_MUTEX(prepare_lock);

static struct task_struct *prepare_owner;
@@ -120,13 +125,14 @@ static unsigned long clk_enable_lock(void)
{
unsigned long flags;

- if (!spin_trylock_irqsave(&enable_lock, flags)) {
+ if (!clk_spin_trylock_irqsave(&enable_lock, flags)) {
if (enable_owner == current) {
enable_refcnt++;
return flags;
}
- spin_lock_irqsave(&enable_lock, flags);
+ clk_spin_lock_irqsave(&enable_lock, flags);
}
+
WARN_ON_ONCE(enable_owner != NULL);
WARN_ON_ONCE(enable_refcnt != 0);
enable_owner = current;
@@ -142,7 +148,7 @@ static void clk_enable_unlock(unsigned long flags)
if (--enable_refcnt)
return;
enable_owner = NULL;
- spin_unlock_irqrestore(&enable_lock, flags);
+ clk_spin_unlock_irqrestore(&enable_lock, flags);
}

/*** debugfs support ***/
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index df69531..a0f5d74 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -266,6 +266,24 @@ struct clk *clk_register_fixed_rate_with_accuracy(struct device *dev,

void of_fixed_clk_setup(struct device_node *np);

+#if defined(CONFIG_COMMON_CLK_USE_RAW_LOCKS)
+typedef raw_spinlock_t clk_spinlock_t;
+#define clk_spin_lock_irqsave(lock, flags) \
+ raw_spin_lock_irqsave(lock, flags)
+#define clk_spin_unlock_irqrestore(lock, flags) \
+ raw_spin_unlock_irqrestore(lock, flags)
+#define clk_spin_trylock_irqsave(lock, flags) \
+ raw_spin_trylock_irqsave(lock, flags)
+#else /* PREEMPT_RT_FULL */
+typedef spinlock_t clk_spinlock_t;
+#define clk_spin_lock_irqsave(lock, flags) \
+ spin_lock_irqsave(lock, flags)
+#define clk_spin_unlock_irqrestore(lock, flags) \
+ spin_unlock_irqrestore(lock, flags)
+#define clk_spin_trylock_irqsave(lock, flags) \
+ spin_trylock_irqsave(lock, flags)
+#endif
+
/**
* struct clk_gate - gating clock
*
@@ -291,7 +309,7 @@ struct clk_gate {
void __iomem *reg;
u8 bit_idx;
u8 flags;
- spinlock_t *lock;
+ clk_spinlock_t *lock;
};

#define CLK_GATE_SET_TO_DISABLE BIT(0)
@@ -301,7 +319,7 @@ extern const struct clk_ops clk_gate_ops;
struct clk *clk_register_gate(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 bit_idx,
- u8 clk_gate_flags, spinlock_t *lock);
+ u8 clk_gate_flags, clk_spinlock_t *lock);
void clk_unregister_gate(struct clk *clk);

struct clk_div_table {
@@ -350,7 +368,7 @@ struct clk_divider {
u8 width;
u8 flags;
const struct clk_div_table *table;
- spinlock_t *lock;
+ clk_spinlock_t *lock;
};

#define CLK_DIVIDER_ONE_BASED BIT(0)
@@ -375,12 +393,12 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
struct clk *clk_register_divider(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
- u8 clk_divider_flags, spinlock_t *lock);
+ u8 clk_divider_flags, clk_spinlock_t *lock);
struct clk *clk_register_divider_table(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
u8 clk_divider_flags, const struct clk_div_table *table,
- spinlock_t *lock);
+ clk_spinlock_t *lock);
void clk_unregister_divider(struct clk *clk);

/**
@@ -413,7 +431,7 @@ struct clk_mux {
u32 mask;
u8 shift;
u8 flags;
- spinlock_t *lock;
+ clk_spinlock_t *lock;
};

#define CLK_MUX_INDEX_ONE BIT(0)
@@ -428,12 +446,12 @@ extern const struct clk_ops clk_mux_ro_ops;
struct clk *clk_register_mux(struct device *dev, const char *name,
const char **parent_names, u8 num_parents, unsigned long flags,
void __iomem *reg, u8 shift, u8 width,
- u8 clk_mux_flags, spinlock_t *lock);
+ u8 clk_mux_flags, clk_spinlock_t *lock);

struct clk *clk_register_mux_table(struct device *dev, const char *name,
const char **parent_names, u8 num_parents, unsigned long flags,
void __iomem *reg, u8 shift, u32 mask,
- u8 clk_mux_flags, u32 *table, spinlock_t *lock);
+ u8 clk_mux_flags, u32 *table, clk_spinlock_t *lock);

void clk_unregister_mux(struct clk *clk);

@@ -484,14 +502,14 @@ struct clk_fractional_divider {
u8 nshift;
u32 nmask;
u8 flags;
- spinlock_t *lock;
+ clk_spinlock_t *lock;
};

extern const struct clk_ops clk_fractional_divider_ops;
struct clk *clk_register_fractional_divider(struct device *dev,
const char *name, const char *parent_name, unsigned long flags,
void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
- u8 clk_divider_flags, spinlock_t *lock);
+ u8 clk_divider_flags, clk_spinlock_t *lock);

/***
* struct clk_composite - aggregate clock of mux, divider and gate clocks
--
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/