Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1463816 > unrolled thread

[RFC 00/17] clk: Add per-controller locks to fix deadlocks

Back to article view | Back to linux.kernel

Contents

  [RFC 00/17] clk: Add per-controller locks to fix deadlocks Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-08-16 15:50 +0200
    [RFC 03/17] clk: s2mps11: Switch to new clock controller API Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-08-16 15:50 +0200
    [RFC 04/17] clk: samsung: Allocate a clock controller in context Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-08-16 15:50 +0200
    [RFC 07/17] clk: mux: Switch to new clock controller API Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-08-16 15:50 +0200
    [RFC 08/17] clk: fixed-factor: Switch to new clock controller API Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-08-16 15:50 +0200
    Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-08-16 16:00 +0200
      Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2016-08-19 16:50 +0200
        Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks Krzysztof Kozlowski <krzk@kernel.org> - 2016-08-19 19:00 +0200
    Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks Javier Martinez Canillas <javier@osg.samsung.com> - 2016-08-19 21:40 +0200
      Re: [RFC 00/17] clk: Add per-controller locks to fix deadlocks Krzysztof Kozlowski <krzk@kernel.org> - 2016-08-20 18:10 +0200

#1463816 — [RFC 00/17] clk: Add per-controller locks to fix deadlocks

Hi,

RFC, please, do not apply, maybe except patch #1 which is harmless.


Introduction
============
The patchset brings new entity: clock controller representing a hardware
block.  The clock controller comes with its own prepare lock which
is used then in many places.  The idea is to fix the deadlock mentioned
in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
by keeping clock prepared").


Disclaimer
==========
Request for comments, so:
1. Only exynos_defconfig builds,
2. A lot of FIXME/TODO note still,
3. Checkpatch not run, lines not aligned,
4. Other (non-exynos) drivers not converted,
5. Probably not yet bisectable,
6. Locking became quite complex.
   The previous one lock was simple.  Inefficient and dead-lock prone but
   simple.  Because of clock hierarchy spanning through controllers, the
   new locking became quite complicated.  I don't like it but...


Details
=======
In Exynos-based boards case the deadlock occurs between clock's
prepare_lock and regmap-i2c's lock:

CPU #0:                             CPU #1:
lock(regmap)
                                    s2mps11-clk: clk_prepare_lock()

i2c-exynos: clk_prepare_lock() - wait
                                    lock(regmap) - wait

The clk_prepare_lock() on both CPUs come from different clock drivers
and components:
1. I2C clock is part of SoC block and is required by i2c-s3c2410/i2c-exynos5
   driver,
2. S2MPS11 clock is separate device, however driver uses I2C regmap.

The deadlock was reported by lockdep (always) and was happening
in 20% of boots of Odroid XU3 with multi_v7 defconfig.  Workaround for
deadlock was implemented by removing prepare/unprepare calls from I2C
transfers.  However these are just workarounds... which after applying
this patch can be reverted.

Additionally Marek Szyprowski's work on domains/clocks/pinctrl exposed
the deadlock again in different configuration.


Comments as usual are welcomed.

Best regards,
Krzysztof

Krzysztof Kozlowski (17):
  clk: bcm2835: Rename clk_register to avoid name conflict
  clk: Add clock controller to fine-grain the prepare lock
  clk: s2mps11: Switch to new clock controller API
  clk: samsung: Allocate a clock controller in context
  clk: fixed-rate: Switch to new clock controller API
  clk: gate: Switch to new clock controller API
  clk: mux: Switch to new clock controller API
  clk: fixed-factor: Switch to new clock controller API
  clk: divider: Switch to new clock controller API
  clk: composite: Switch to new clock controller API
  clk: gpio: Switch to new clock controller API
  ASoC: samsung: Switch to new clock controller API
  clk: samsung: audss: samsung: Switch to new clock controller API
  clk: samsung: clkout: samsung: Switch to new clock controller API
  clk: Use per-controller locking
  Revert "i2c: exynos5: Fix possible ABBA deadlock by keeping I2C clock
    prepared"
  Revert "i2c: s3c2410: fix ABBA deadlock by keeping clock prepared"

 drivers/clk/bcm/clk-bcm2835.c           |   8 +-
 drivers/clk/clk-composite.c             |   8 +-
 drivers/clk/clk-divider.c               |  10 +-
 drivers/clk/clk-fixed-factor.c          |  11 +-
 drivers/clk/clk-fixed-rate.c            |  28 +-
 drivers/clk/clk-fractional-divider.c    |   5 +-
 drivers/clk/clk-gate.c                  |   8 +-
 drivers/clk/clk-gpio.c                  |  29 +-
 drivers/clk/clk-mux.c                   |  32 ++-
 drivers/clk/clk-s2mps11.c               |  10 +-
 drivers/clk/clk.c                       | 456 +++++++++++++++++++++++++++-----
 drivers/clk/samsung/clk-exynos-audss.c  |  30 ++-
 drivers/clk/samsung/clk-exynos-clkout.c |  11 +-
 drivers/clk/samsung/clk.c               |  25 +-
 drivers/clk/samsung/clk.h               |   1 +
 drivers/i2c/busses/i2c-exynos5.c        |  24 +-
 drivers/i2c/busses/i2c-s3c2410.c        |  23 +-
 include/linux/clk-provider.h            |  88 ++++--
 include/linux/clk.h                     |   1 +
 sound/soc/samsung/i2s.c                 |  13 +-
 20 files changed, 612 insertions(+), 209 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1463818 — [RFC 03/17] clk: s2mps11: Switch to new clock controller API

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/clk-s2mps11.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
index fbaa84a33c46..881f1e226867 100644
--- a/drivers/clk/clk-s2mps11.c
+++ b/drivers/clk/clk-s2mps11.c
@@ -41,6 +41,7 @@ struct s2mps11_clk {
 	struct clk_hw hw;
 	struct clk *clk;
 	struct clk_lookup *lookup;
+	struct clk_ctrl *clk_ctrl;
 	u32 mask;
 	unsigned int reg;
 };
@@ -176,6 +177,10 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 	if (IS_ERR(s2mps11_clks->clk_np))
 		return PTR_ERR(s2mps11_clks->clk_np);
 
+	s2mps11_clks->clk_ctrl = clk_ctrl_register(&pdev->dev);
+	if (IS_ERR(s2mps11_clks->clk_ctrl))
+		return PTR_ERR(s2mps11_clks->clk_ctrl); // FIXME: use devm-like
+
 	for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
 		if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
 			continue; /* Skip clocks not present in some devices */
@@ -184,7 +189,8 @@ static int s2mps11_clk_probe(struct platform_device *pdev)
 		s2mps11_clks[i].mask = 1 << i;
 		s2mps11_clks[i].reg = s2mps11_reg;
 
-		s2mps11_clks[i].clk = devm_clk_register(&pdev->dev,
+		s2mps11_clks[i].clk = devm_clk_register_with_ctrl(&pdev->dev,
+							s2mps11_clks->clk_ctrl,
 							&s2mps11_clks[i].hw);
 		if (IS_ERR(s2mps11_clks[i].clk)) {
 			dev_err(&pdev->dev, "Fail to register : %s\n",
@@ -233,6 +239,8 @@ static int s2mps11_clk_remove(struct platform_device *pdev)
 		clkdev_drop(s2mps11_clks[i].lookup);
 	}
 
+	clk_ctrl_unregister(s2mps11_clks->clk_ctrl);
+
 	return 0;
 }
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1463821 — [RFC 04/17] clk: samsung: Allocate a clock controller in context

Allocate a clock controller and store it in context so it will be passed
later for creating clocks.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/samsung/clk.c | 4 ++++
 drivers/clk/samsung/clk.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index b7d87d6db9dc..fdeb35a48d3a 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -67,6 +67,10 @@ struct samsung_clk_provider *__init samsung_clk_init(struct device_node *np,
 	if (!ctx)
 		panic("could not allocate clock provider context.\n");
 
+	ctx->clk_ctrl = clk_ctrl_register(NULL);
+	if (!ctx->clk_ctrl)
+		panic("could not allocate clock provider controller.\n");
+
 	clk_table = kcalloc(nr_clks, sizeof(struct clk *), GFP_KERNEL);
 	if (!clk_table)
 		panic("could not allocate clock lookup table\n");
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index da3bdebabf1e..cb0ef6266b6d 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -28,6 +28,7 @@ struct samsung_clk_provider {
 	void __iomem *reg_base;
 	struct clk_onecell_data clk_data;
 	spinlock_t lock;
+	struct clk_ctrl *clk_ctrl;
 };
 
 /**
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1463822 — [RFC 07/17] clk: mux: Switch to new clock controller API

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/clk-mux.c        | 32 ++++++++++++++++++--------------
 drivers/clk/samsung/clk.c    |  4 ++--
 include/linux/clk-provider.h | 12 ++++++++----
 3 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 16a3d5717f4e..a85bbef1992b 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -113,7 +113,8 @@ const struct clk_ops clk_mux_ro_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_mux_ro_ops);
 
-struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux_table(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,
@@ -159,7 +160,7 @@ struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
 	mux->hw.init = &init;
 
 	hw = &mux->hw;
-	ret = clk_hw_register(dev, hw);
+	ret = clk_hw_register_with_ctrl(dev, ctrl, hw);
 	if (ret) {
 		kfree(mux);
 		hw = ERR_PTR(ret);
@@ -169,7 +170,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_mux_table);
 
-struct clk *clk_register_mux_table(struct device *dev, const char *name,
+struct clk *clk_register_mux_table(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,
@@ -177,16 +179,17 @@ struct clk *clk_register_mux_table(struct device *dev, const char *name,
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_mux_table(dev, name, parent_names, num_parents,
-				       flags, reg, shift, mask, clk_mux_flags,
-				       table, lock);
+	hw = clk_hw_register_mux_table(dev, ctrl, name, parent_names,
+				       num_parents, flags, reg, shift, mask,
+				       clk_mux_flags, table, lock);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
 	return hw->clk;
 }
 EXPORT_SYMBOL_GPL(clk_register_mux_table);
 
-struct clk *clk_register_mux(struct device *dev, const char *name,
+struct clk *clk_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
@@ -194,13 +197,14 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 {
 	u32 mask = BIT(width) - 1;
 
-	return clk_register_mux_table(dev, name, parent_names, num_parents,
-				      flags, reg, shift, mask, clk_mux_flags,
-				      NULL, lock);
+	return clk_register_mux_table(dev, ctrl, name, parent_names,
+				      num_parents, flags, reg, shift, mask,
+				      clk_mux_flags, NULL, lock);
 }
 EXPORT_SYMBOL_GPL(clk_register_mux);
 
-struct clk_hw *clk_hw_register_mux(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
@@ -208,9 +212,9 @@ struct clk_hw *clk_hw_register_mux(struct device *dev, const char *name,
 {
 	u32 mask = BIT(width) - 1;
 
-	return clk_hw_register_mux_table(dev, name, parent_names, num_parents,
-				      flags, reg, shift, mask, clk_mux_flags,
-				      NULL, lock);
+	return clk_hw_register_mux_table(dev, ctrl, name, parent_names,
+					 num_parents, flags, reg, shift, mask,
+					 clk_mux_flags, NULL, lock);
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_mux);
 
diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 79332011f258..1a296bbabd47 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -197,8 +197,8 @@ void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
 	unsigned int idx, ret;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
-		clk = clk_register_mux(NULL, list->name, list->parent_names,
-			list->num_parents, list->flags,
+		clk = clk_register_mux(NULL, ctx->clk_ctrl, list->name,
+			list->parent_names, list->num_parents, list->flags,
 			ctx->reg_base + list->offset,
 			list->shift, list->width, list->mux_flags, &ctx->lock);
 		if (IS_ERR(clk)) {
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 289290655283..f5f062d267a9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -493,23 +493,27 @@ struct clk_mux {
 extern const struct clk_ops clk_mux_ops;
 extern const struct clk_ops clk_mux_ro_ops;
 
-struct clk *clk_register_mux(struct device *dev, const char *name,
+struct clk *clk_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_mux_flags, spinlock_t *lock);
-struct clk_hw *clk_hw_register_mux(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_mux_flags, spinlock_t *lock);
 
-struct clk *clk_register_mux_table(struct device *dev, const char *name,
+struct clk *clk_register_mux_table(struct device *dev, struct clk_ctrl *ctrl,
+		const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,
 		u8 clk_mux_flags, u32 *table, spinlock_t *lock);
-struct clk_hw *clk_hw_register_mux_table(struct device *dev, const char *name,
+struct clk_hw *clk_hw_register_mux_table(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned long flags,
 		void __iomem *reg, u8 shift, u32 mask,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1463823 — [RFC 08/17] clk: fixed-factor: Switch to new clock controller API

Allocate a clock controller and use new clk_register_with_ctrl() API.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/clk-fixed-factor.c | 11 +++++++----
 drivers/clk/samsung/clk.c      |  2 +-
 include/linux/clk-provider.h   |  4 +++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 4db3be214077..921a99b6e421 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -69,6 +69,7 @@ const struct clk_ops clk_fixed_factor_ops = {
 EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
 
 struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div)
 {
@@ -103,13 +104,14 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor);
 
-struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
+struct clk *clk_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div)
 {
 	struct clk_hw *hw;
 
-	hw = clk_hw_register_fixed_factor(dev, name, parent_name, flags, mult,
+	hw = clk_hw_register_fixed_factor(dev, ctrl, name, parent_name, flags, mult,
 					  div);
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
@@ -176,8 +178,9 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 	if (of_match_node(set_rate_parent_matches, node))
 		flags |= CLK_SET_RATE_PARENT;
 
-	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
-					mult, div);
+	/* TODO: convert to clk_ctrl */
+	clk = clk_register_fixed_factor(NULL, NULL, clk_name, parent_name,
+					flags, mult, div);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
 }
diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 1a296bbabd47..7bfd895781c5 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -176,7 +176,7 @@ void __init samsung_clk_register_fixed_factor(struct samsung_clk_provider *ctx,
 	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
-		clk = clk_register_fixed_factor(NULL, list->name,
+		clk = clk_register_fixed_factor(NULL, ctx->clk_ctrl, list->name,
 			list->parent_name, list->flags, list->mult, list->div);
 		if (IS_ERR(clk)) {
 			pr_err("%s: failed to register clock %s\n", __func__,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index f5f062d267a9..26171815948e 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -545,11 +545,13 @@ struct clk_fixed_factor {
 #define to_clk_fixed_factor(_hw) container_of(_hw, struct clk_fixed_factor, hw)
 
 extern const struct clk_ops clk_fixed_factor_ops;
-struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
+struct clk *clk_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 void clk_unregister_fixed_factor(struct clk *clk);
 struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
+		struct clk_ctrl *ctrl,
 		const char *name, const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 void clk_hw_unregister_fixed_factor(struct clk_hw *hw);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1463829

On 08/16/2016 03:34 PM, Krzysztof Kozlowski wrote:
> Hi,
> 
> RFC, please, do not apply, maybe except patch #1 which is harmless.
> 
> 
> Introduction
> ============
> The patchset brings new entity: clock controller representing a hardware
> block.  The clock controller comes with its own prepare lock which
> is used then in many places.  The idea is to fix the deadlock mentioned
> in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
> I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
> by keeping clock prepared").
> 

Damn, I forgot to describe the overall idea. :) It is mentioned in patch
15 but probably not many will have enough of patience to reach it.

The locking idea
================
Clock controllers representing different hardware blocks, will contain
its own prepare lock which protects the clocks inside controller.  The
hierarchy itself is protected by global lock.

In prepare path, the global prepare lock is removed.  This is direct
solution for the deadlock.

Clock hierarchy imposes also hierarchy between controllers so when a
prepare happens, also parents have to be locked.

Following locking design was chosen:
1. For prepare/unprepare paths: lock only clock controller and its
   parents.
2. For recalc rates paths: lock global lock, the controller and its
   children.
3. For reparent paths: lock entire tree up down (children and parents)
   and the global lock as well.


In each case of traversing the clock hierarchy, the locking of
controllers is always from children to parents.


Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1466467

On Tue, Aug 16, 2016 at 03:51:10PM +0200, Krzysztof Kozlowski wrote:
> On 08/16/2016 03:34 PM, Krzysztof Kozlowski wrote:
> > Hi,
> > 
> > RFC, please, do not apply, maybe except patch #1 which is harmless.
> > 
> > 
> > Introduction
> > ============
> > The patchset brings new entity: clock controller representing a hardware
> > block.  The clock controller comes with its own prepare lock which
> > is used then in many places.  The idea is to fix the deadlock mentioned
> > in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
> > I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
> > by keeping clock prepared").
> > 
> 
> Damn, I forgot to describe the overall idea. :) It is mentioned in patch
> 15 but probably not many will have enough of patience to reach it.
> 
> The locking idea
> ================
> Clock controllers representing different hardware blocks, will contain
> its own prepare lock which protects the clocks inside controller.  The
> hierarchy itself is protected by global lock.
> 
> In prepare path, the global prepare lock is removed.  This is direct
> solution for the deadlock.
> 
> Clock hierarchy imposes also hierarchy between controllers so when a
> prepare happens, also parents have to be locked.
> 
> Following locking design was chosen:
> 1. For prepare/unprepare paths: lock only clock controller and its
>    parents.
> 2. For recalc rates paths: lock global lock, the controller and its
>    children.
> 3. For reparent paths: lock entire tree up down (children and parents)
>    and the global lock as well.
> 
> 
> In each case of traversing the clock hierarchy, the locking of
> controllers is always from children to parents.
> 
> 
> Best regards,
> Krzysztof

I have been playing with these patches on my Arndale board and
they certainly do seem to resolve the interaction issues I have
been SPI and the clocking framework, which is awesome and lets me
sensibly add the clocking framework into our codec drivers. I will
keep investigating and for what its worth have a little more
detailed look through the code.

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1466559

On Fri, Aug 19, 2016 at 03:46:40PM +0100, Charles Keepax wrote:
> On Tue, Aug 16, 2016 at 03:51:10PM +0200, Krzysztof Kozlowski wrote:
> > On 08/16/2016 03:34 PM, Krzysztof Kozlowski wrote:
> > > Hi,
> > > 
> > > RFC, please, do not apply, maybe except patch #1 which is harmless.
> > > 
> > > 
> > > Introduction
> > > ============
> > > The patchset brings new entity: clock controller representing a hardware
> > > block.  The clock controller comes with its own prepare lock which
> > > is used then in many places.  The idea is to fix the deadlock mentioned
> > > in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
> > > I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
> > > by keeping clock prepared").
> > > 
> > 
> > Damn, I forgot to describe the overall idea. :) It is mentioned in patch
> > 15 but probably not many will have enough of patience to reach it.
> > 
> > The locking idea
> > ================
> > Clock controllers representing different hardware blocks, will contain
> > its own prepare lock which protects the clocks inside controller.  The
> > hierarchy itself is protected by global lock.
> > 
> > In prepare path, the global prepare lock is removed.  This is direct
> > solution for the deadlock.
> > 
> > Clock hierarchy imposes also hierarchy between controllers so when a
> > prepare happens, also parents have to be locked.
> > 
> > Following locking design was chosen:
> > 1. For prepare/unprepare paths: lock only clock controller and its
> >    parents.
> > 2. For recalc rates paths: lock global lock, the controller and its
> >    children.
> > 3. For reparent paths: lock entire tree up down (children and parents)
> >    and the global lock as well.
> > 
> > 
> > In each case of traversing the clock hierarchy, the locking of
> > controllers is always from children to parents.
> > 
> > 
> > Best regards,
> > Krzysztof
> 
> I have been playing with these patches on my Arndale board and
> they certainly do seem to resolve the interaction issues I have
> been SPI and the clocking framework, which is awesome and lets me
> sensibly add the clocking framework into our codec drivers. I will
> keep investigating and for what its worth have a little more
> detailed look through the code.

I am really happy to hear it!

Along with other Samsung guys from Poland we really spent a lot of time
figuring out all the locking cases, possible scenarios and new issues
which could come out of it.

I am glad that it solves also other people's cases, not only ours!

Certainly there is a lot of things to improve in the patchset. Probably
merging the new "clock controller" entity into clock provider makes
sense.

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1466626

Hello Krzysztof,

On 08/16/2016 09:34 AM, Krzysztof Kozlowski wrote:
> Hi,
> 
> RFC, please, do not apply, maybe except patch #1 which is harmless.
> 
> 
> Introduction
> ============
> The patchset brings new entity: clock controller representing a hardware
> block.  The clock controller comes with its own prepare lock which
> is used then in many places.  The idea is to fix the deadlock mentioned
> in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
> I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
> by keeping clock prepared").
> 
> 

I'm not familiar enough with the common clock framework to do a proper review
of this patch-set, but I've tested both on an Exynos5800 Peach Pi Chromebook
and an Exynos5422 Odroid XU4 board and I didn't find any clock regressions.

Also, I confirmed that the possible deadlock in the Odroid XU4 that was fixed
by reverted commit 10ff4c5239a1 doesn't happen anymore with your patches.

Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1466826

On Fri, Aug 19, 2016 at 03:31:08PM -0400, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 08/16/2016 09:34 AM, Krzysztof Kozlowski wrote:
> > Hi,
> > 
> > RFC, please, do not apply, maybe except patch #1 which is harmless.
> > 
> > 
> > Introduction
> > ============
> > The patchset brings new entity: clock controller representing a hardware
> > block.  The clock controller comes with its own prepare lock which
> > is used then in many places.  The idea is to fix the deadlock mentioned
> > in commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA deadlock by keeping
> > I2C clock prepared") and commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA deadlock
> > by keeping clock prepared").
> > 
> > 
> 
> I'm not familiar enough with the common clock framework to do a proper review
> of this patch-set, but I've tested both on an Exynos5800 Peach Pi Chromebook
> and an Exynos5422 Odroid XU4 board and I didn't find any clock regressions.
> 
> Also, I confirmed that the possible deadlock in the Odroid XU4 that was fixed
> by reverted commit 10ff4c5239a1 doesn't happen anymore with your patches.
> 
> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Thanks for testing, I appreciate that!

Best regards,
Krzysztof

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web