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

[Michael Turquette]

Quoting Grygorii Strashko (2015-10-20 13:59:19)
> 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'. 

Well that does sound like a clusterfuck. Out of curiosity, where do you
need to call clk_enable/clk_disable from atomic context?

I wonder if the very idea of calling clk_enable/clk_disable from atomic
context is valid for -rt... Clearly the clk.h api states that these
functions are safe to call in irq context, which it seems is true for
-rt, right?

The problem is that you want to call them in a context whose meaning is
different in the -rt world. So it would be helpful for me to get a
better understanding of some examples of where things are going wrong
for you.

> 
> 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

Right, so things go sideways here because omap_gpio_debounce uses
raw_spin_lock_irqsave. Is this necessary? I notice that similar
implementations of the same .set_debounce callback are using regmap or
regular spinlocks (e.g. wm831x_gpio_set_debounce).

Thanks for educating me on this. I never look at the -rt stuff so I'm
sure to ask some dumb questions.

Regards,
Mike

> 
> 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/