[PATCH 0/3] clk : Fix SOCFPGA clk crash and some clean up

[PATCH 0/3] clk : Fix SOCFPGA clk crash and some clean up

[Charles Manning]

After some digging into a crash on the SOCFPGA initialisation it was
determined that this was due to the SOCFPGA's name look-up dereferencing
the clock init pointer. This is a bad thing to do because the init data is
constructed on the stack and thus a pointer to the init data is no longer
valid after clk_register() has been called.

Most, if not all, drivers create the init data on the stack. Thus init 
pointer should not be left hanging after it has been used in clk_register.
Thus one of these patches NULLs the pointer so it can't be abused.

In the long term, it would be way, way better to pass the init pointer as
an argument rather than as a pointer in hw.


Charles Manning (3):
  clk : Clean up checkpatch warnings
  clk: Prevent a hanging pointer being abused
  clk: socfpga Change name look-up to not use the init pointer

 drivers/clk/clk.c                | 37 +++++++++++++++++++--------------
 drivers/clk/socfpga/clk-gate.c   | 44 ++++++++++++++++++++++++++--------------
 drivers/clk/socfpga/clk-periph.c | 16 ++++++++++-----
 drivers/clk/socfpga/clk-pll.c    | 13 ++++++++----
 drivers/clk/socfpga/clk.h        |  7 ++++---
 5 files changed, 75 insertions(+), 42 deletions(-)

-- 
1.9.1

[PATCH 1/3] clk : Clean up checkpatch warnings

[Charles Manning]

Signed-off-by: Charles Manning <cdhmanning@gmail.com>
---
 drivers/clk/clk.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8b73ede..ec41922 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -99,7 +99,7 @@ static void clk_enable_unlock(unsigned long flags)
 
 static struct dentry *rootdir;
 static struct dentry *orphandir;
-static int inited = 0;
+static int inited;
 
 static void clk_summary_show_one(struct seq_file *s, struct clk *c, int level)
 {
@@ -182,11 +182,11 @@ static void clk_dump_subtree(struct seq_file *s, struct clk *c, int level)
 	clk_dump_one(s, c, level);
 
 	hlist_for_each_entry(child, &c->children, child_node) {
-		seq_printf(s, ",");
+		seq_puts(s, ",");
 		clk_dump_subtree(s, child, level + 1);
 	}
 
-	seq_printf(s, "}");
+	seq_puts(s, "}");
 }
 
 static int clk_dump(struct seq_file *s, void *data)
@@ -194,25 +194,25 @@ static int clk_dump(struct seq_file *s, void *data)
 	struct clk *c;
 	bool first_node = true;
 
-	seq_printf(s, "{");
+	seq_puts(s, "{");
 
 	clk_prepare_lock();
 
 	hlist_for_each_entry(c, &clk_root_list, child_node) {
 		if (!first_node)
-			seq_printf(s, ",");
+			seq_puts(s, ",");
 		first_node = false;
 		clk_dump_subtree(s, c, 0);
 	}
 
 	hlist_for_each_entry(c, &clk_orphan_list, child_node) {
-		seq_printf(s, ",");
+		seq_puts(s, ",");
 		clk_dump_subtree(s, c, 0);
 	}
 
 	clk_prepare_unlock();
 
-	seq_printf(s, "}");
+	seq_puts(s, "}");
 	return 0;
 }
 
@@ -294,7 +294,7 @@ out:
 static int clk_debug_create_subtree(struct clk *clk, struct dentry *pdentry)
 {
 	struct clk *child;
-	int ret = -EINVAL;;
+	int ret = -EINVAL;
 
 	if (!clk || !pdentry)
 		goto out;
@@ -1455,7 +1455,8 @@ out:
  * so that in case of an error we can walk down the whole tree again and
  * abort the change.
  */
-static struct clk *clk_propagate_rate_change(struct clk *clk, unsigned long event)
+static struct clk *clk_propagate_rate_change(struct clk *clk,
+					     unsigned long event)
 {
 	struct clk *child, *tmp_clk, *fail_clk = NULL;
 	int ret = NOTIFY_DONE;
@@ -1798,14 +1799,14 @@ int __clk_init(struct device *dev, struct clk *clk)
 	if (clk->ops->set_rate &&
 	    !((clk->ops->round_rate || clk->ops->determine_rate) &&
 	      clk->ops->recalc_rate)) {
-		pr_warning("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
+		pr_warn("%s: %s must implement .round_rate or .determine_rate in addition to .recalc_rate\n",
 				__func__, clk->name);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if (clk->ops->set_parent && !clk->ops->get_parent) {
-		pr_warning("%s: %s must implement .get_parent & .set_parent\n",
+		pr_warn("%s: %s must implement .get_parent & .set_parent\n",
 				__func__, clk->name);
 		ret = -EINVAL;
 		goto out;
@@ -2116,8 +2117,8 @@ void clk_unregister(struct clk *clk)
 {
 	unsigned long flags;
 
-       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
-               return;
+	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+		return;
 
 	clk_prepare_lock();
 
@@ -2194,6 +2195,7 @@ EXPORT_SYMBOL_GPL(devm_clk_register);
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {
 	struct clk *c = res;
+
 	if (WARN_ON(!c))
 		return 0;
 	return c == data;
@@ -2409,8 +2411,9 @@ EXPORT_SYMBOL_GPL(of_clk_src_onecell_get);
  * @data: context pointer for @clk_src_get callback.
  */
 int of_clk_add_provider(struct device_node *np,
-			struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
-						   void *data),
+			struct clk *(*clk_src_get)
+					(struct of_phandle_args *clkspec,
+					 void *data),
 			void *data)
 {
 	struct of_clk_provider *cp;
-- 
1.9.1

[PATCH 2/3] clk: Prevent a hanging pointer being abused

[Charles Manning]

Clock init structs are normally created on the stack.

If the pointer is left intact after clk_register then there is
opportunity for clk drivers to dereference the pointer.

This was causing a problem in socfpga/clk.c for instance.

Better to NULL out the pointer so it can't be abused.

Signed-off-by: Charles Manning <cdhmanning@gmail.com>
---
 drivers/clk/clk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ec41922..9e92170 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1973,6 +1973,10 @@ struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
 		clk->owner = NULL;
 
 	ret = __clk_init(dev, clk);
+
+	/* Prevent a hanging pointer being left around. */
+	hw->init = NULL;
+
 	if (ret)
 		return ERR_PTR(ret);
 
-- 
1.9.1

[PATCH 3/3] clk: socfpga Change name look-up to not use the init pointer

[Charles Manning]

The init pointer is not valid after clk_register so we store
the names with the socfpga clock structures so we can look them up.

Signed-off-by: Charles Manning <cdhmanning@gmail.com>
---
 drivers/clk/socfpga/clk-gate.c   | 44 ++++++++++++++++++++++++++--------------
 drivers/clk/socfpga/clk-periph.c | 16 ++++++++++-----
 drivers/clk/socfpga/clk-pll.c    | 13 ++++++++----
 drivers/clk/socfpga/clk.h        |  7 ++++---
 4 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
index dd3a78c..a319f01 100644
--- a/drivers/clk/socfpga/clk-gate.c
+++ b/drivers/clk/socfpga/clk-gate.c
@@ -34,7 +34,10 @@
 
 #define streq(a, b) (strcmp((a), (b)) == 0)
 
-#define to_socfpga_gate_clk(p) container_of(p, struct socfpga_gate_clk, hw.hw)
+#define to_socfpga_gate_clk(p) \
+		container_of(p, struct socfpga_gate_clk, base.hw.hw)
+#define to_socfpga_base_clk(p) \
+		container_of(p, struct socfpga_base_clk, hw.hw)
 
 /* SDMMC Group for System Manager defines */
 #define SYSMGR_SDMMCGRP_CTRL_OFFSET    0x108
@@ -45,21 +48,24 @@ static u8 socfpga_clk_get_parent(struct clk_hw *hwclk)
 {
 	u32 l4_src;
 	u32 perpll_src;
+	struct socfpga_base_clk *base_clk;
 
-	if (streq(hwclk->init->name, SOCFPGA_L4_MP_CLK)) {
+	base_clk = to_socfpga_base_clk(hwclk);
+
+	if (streq(base_clk->name, SOCFPGA_L4_MP_CLK)) {
 		l4_src = readl(clk_mgr_base_addr + CLKMGR_L4SRC);
 		return l4_src &= 0x1;
 	}
-	if (streq(hwclk->init->name, SOCFPGA_L4_SP_CLK)) {
+	if (streq(base_clk->name, SOCFPGA_L4_SP_CLK)) {
 		l4_src = readl(clk_mgr_base_addr + CLKMGR_L4SRC);
 		return !!(l4_src & 2);
 	}
 
 	perpll_src = readl(clk_mgr_base_addr + CLKMGR_PERPLL_SRC);
-	if (streq(hwclk->init->name, SOCFPGA_MMC_CLK))
+	if (streq(base_clk->name, SOCFPGA_MMC_CLK))
 		return perpll_src &= 0x3;
-	if (streq(hwclk->init->name, SOCFPGA_NAND_CLK) ||
-			streq(hwclk->init->name, SOCFPGA_NAND_X_CLK))
+	if (streq(base_clk->name, SOCFPGA_NAND_CLK) ||
+			streq(base_clk->name, SOCFPGA_NAND_X_CLK))
 			return (perpll_src >> 2) & 3;
 
 	/* QSPI clock */
@@ -70,24 +76,27 @@ static u8 socfpga_clk_get_parent(struct clk_hw *hwclk)
 static int socfpga_clk_set_parent(struct clk_hw *hwclk, u8 parent)
 {
 	u32 src_reg;
+	struct socfpga_base_clk *base_clk;
+
+	base_clk = to_socfpga_base_clk(hwclk);
 
-	if (streq(hwclk->init->name, SOCFPGA_L4_MP_CLK)) {
+	if (streq(base_clk->name, SOCFPGA_L4_MP_CLK)) {
 		src_reg = readl(clk_mgr_base_addr + CLKMGR_L4SRC);
 		src_reg &= ~0x1;
 		src_reg |= parent;
 		writel(src_reg, clk_mgr_base_addr + CLKMGR_L4SRC);
-	} else if (streq(hwclk->init->name, SOCFPGA_L4_SP_CLK)) {
+	} else if (streq(base_clk->name, SOCFPGA_L4_SP_CLK)) {
 		src_reg = readl(clk_mgr_base_addr + CLKMGR_L4SRC);
 		src_reg &= ~0x2;
 		src_reg |= (parent << 1);
 		writel(src_reg, clk_mgr_base_addr + CLKMGR_L4SRC);
 	} else {
 		src_reg = readl(clk_mgr_base_addr + CLKMGR_PERPLL_SRC);
-		if (streq(hwclk->init->name, SOCFPGA_MMC_CLK)) {
+		if (streq(base_clk->name, SOCFPGA_MMC_CLK)) {
 			src_reg &= ~0x3;
 			src_reg |= parent;
-		} else if (streq(hwclk->init->name, SOCFPGA_NAND_CLK) ||
-			streq(hwclk->init->name, SOCFPGA_NAND_X_CLK)) {
+		} else if (streq(base_clk->name, SOCFPGA_NAND_CLK) ||
+			streq(base_clk->name, SOCFPGA_NAND_X_CLK)) {
 			src_reg &= ~0xC;
 			src_reg |= (parent << 2);
 		} else {/* QSPI clock */
@@ -205,8 +214,8 @@ static void __init __socfpga_gate_init(struct device_node *node,
 		clk_gate[0] = 0;
 
 	if (clk_gate[0]) {
-		socfpga_clk->hw.reg = clk_mgr_base_addr + clk_gate[0];
-		socfpga_clk->hw.bit_idx = clk_gate[1];
+		socfpga_clk->base.hw.reg = clk_mgr_base_addr + clk_gate[0];
+		socfpga_clk->base.hw.bit_idx = clk_gate[1];
 
 		gateclk_ops.enable = clk_gate_ops.enable;
 		gateclk_ops.disable = clk_gate_ops.disable;
@@ -244,9 +253,14 @@ static void __init __socfpga_gate_init(struct device_node *node,
 
 	init.parent_names = parent_name;
 	init.num_parents = i;
-	socfpga_clk->hw.hw.init = &init;
+	socfpga_clk->base.hw.hw.init = &init;
+	socfpga_clk->base.name = kstrdup(clk_name, GFP_KERNEL);
+	if (WARN_ON(!socfpga_clk->base.name)) {
+		kfree(socfpga_clk);
+		return;
+	}
 
-	clk = clk_register(NULL, &socfpga_clk->hw.hw);
+	clk = clk_register(NULL, &socfpga_clk->base.hw.hw);
 	if (WARN_ON(IS_ERR(clk))) {
 		kfree(socfpga_clk);
 		return;
diff --git a/drivers/clk/socfpga/clk-periph.c b/drivers/clk/socfpga/clk-periph.c
index 46531c3..df2fd2c 100644
--- a/drivers/clk/socfpga/clk-periph.c
+++ b/drivers/clk/socfpga/clk-periph.c
@@ -23,7 +23,8 @@
 
 #include "clk.h"
 
-#define to_socfpga_periph_clk(p) container_of(p, struct socfpga_periph_clk, hw.hw)
+#define to_socfpga_periph_clk(p) \
+		container_of(p, struct socfpga_periph_clk, base.hw.hw)
 
 static unsigned long clk_periclk_recalc_rate(struct clk_hw *hwclk,
 					     unsigned long parent_rate)
@@ -39,7 +40,7 @@ static unsigned long clk_periclk_recalc_rate(struct clk_hw *hwclk,
 			val &= div_mask(socfpgaclk->width);
 			parent_rate /= (val + 1);
 		}
-		div = ((readl(socfpgaclk->hw.reg) & 0x1ff) + 1);
+		div = ((readl(socfpgaclk->base.hw.reg) & 0x1ff) + 1);
 	}
 
 	return parent_rate / div;
@@ -68,7 +69,7 @@ static __init void __socfpga_periph_init(struct device_node *node,
 	if (WARN_ON(!periph_clk))
 		return;
 
-	periph_clk->hw.reg = clk_mgr_base_addr + reg;
+	periph_clk->base.hw.reg = clk_mgr_base_addr + reg;
 
 	rc = of_property_read_u32_array(node, "div-reg", div_reg, 3);
 	if (!rc) {
@@ -94,9 +95,14 @@ static __init void __socfpga_periph_init(struct device_node *node,
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
 
-	periph_clk->hw.hw.init = &init;
+	periph_clk->base.hw.hw.init = &init;
+	periph_clk->base.name = kstrdup(clk_name, GFP_KERNEL);
+	if (WARN_ON(!periph_clk->base.name)) {
+		kfree(periph_clk);
+		return;
+	}
 
-	clk = clk_register(NULL, &periph_clk->hw.hw);
+	clk = clk_register(NULL, &periph_clk->base.hw.hw);
 	if (WARN_ON(IS_ERR(clk))) {
 		kfree(periph_clk);
 		return;
diff --git a/drivers/clk/socfpga/clk-pll.c b/drivers/clk/socfpga/clk-pll.c
index de6da95..d48f637 100644
--- a/drivers/clk/socfpga/clk-pll.c
+++ b/drivers/clk/socfpga/clk-pll.c
@@ -42,14 +42,14 @@
 #define CLK_MGR_PLL_CLK_SRC_SHIFT	22
 #define CLK_MGR_PLL_CLK_SRC_MASK	0x3
 
-#define to_socfpga_clk(p) container_of(p, struct socfpga_pll, hw.hw)
+#define to_socfpga_clk(p) container_of(p, struct socfpga_base_clk, hw.hw)
 
 void __iomem *clk_mgr_base_addr;
 
 static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 					 unsigned long parent_rate)
 {
-	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
+	struct socfpga_base_clk *socfpgaclk = to_socfpga_clk(hwclk);
 	unsigned long divf, divq, reg;
 	unsigned long long vco_freq;
 	unsigned long bypass;
@@ -69,7 +69,7 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 static u8 clk_pll_get_parent(struct clk_hw *hwclk)
 {
 	u32 pll_src;
-	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
+	struct socfpga_base_clk *socfpgaclk = to_socfpga_clk(hwclk);
 
 	pll_src = readl(socfpgaclk->hw.reg);
 	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
@@ -86,7 +86,7 @@ static __init struct clk *__socfpga_pll_init(struct device_node *node,
 {
 	u32 reg;
 	struct clk *clk;
-	struct socfpga_pll *pll_clk;
+	struct socfpga_base_clk *pll_clk;
 	const char *clk_name = node->name;
 	const char *parent_name[SOCFPGA_MAX_PARENTS];
 	struct clk_init_data init;
@@ -118,6 +118,11 @@ static __init struct clk *__socfpga_pll_init(struct device_node *node,
 	init.num_parents = i;
 	init.parent_names = parent_name;
 	pll_clk->hw.hw.init = &init;
+	pll_clk->name = kstrdup(clk_name, GFP_KERNEL);
+	if (WARN_ON(!pll_clk->name)) {
+		kfree(pll_clk);
+		return NULL;
+	}
 
 	pll_clk->hw.bit_idx = SOCFPGA_PLL_EXT_ENA;
 	clk_pll_ops.enable = clk_gate_ops.enable;
diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
index d291f60..c2d058c 100644
--- a/drivers/clk/socfpga/clk.h
+++ b/drivers/clk/socfpga/clk.h
@@ -35,12 +35,13 @@ void __init socfpga_pll_init(struct device_node *node);
 void __init socfpga_periph_init(struct device_node *node);
 void __init socfpga_gate_init(struct device_node *node);
 
-struct socfpga_pll {
+struct socfpga_base_clk {
+	const char *name;
 	struct clk_gate	hw;
 };
 
 struct socfpga_gate_clk {
-	struct clk_gate hw;
+	struct socfpga_base_clk base;
 	char *parent_name;
 	u32 fixed_div;
 	void __iomem *div_reg;
@@ -50,7 +51,7 @@ struct socfpga_gate_clk {
 };
 
 struct socfpga_periph_clk {
-	struct clk_gate hw;
+	struct socfpga_base_clk base;
 	char *parent_name;
 	u32 fixed_div;
 	void __iomem *div_reg;
-- 
1.9.1

Re: [PATCH 0/3] clk : Fix SOCFPGA clk crash and some clean up

[Steffen Trumtrar]

Hi!

As you touch clk.c, you should Cc

   Mike Turquette <mturquette@linaro.org>

He is the maintainer for that code.

Regards,
Steffen

-- 
Pengutronix e.K.                           | Steffen Trumtrar            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/3] clk : Fix SOCFPGA clk crash and some clean up

[Dinh Nguyen]

Hi Charles,

On Tue, 2014-07-22 at 08:00 +0200, Steffen Trumtrar wrote:
> Hi!
> 
> As you touch clk.c, you should Cc
> 
>    Mike Turquette <mturquette@linaro.org>
> 
> He is the maintainer for that code.
> 

Yes, please use get_maintainer.pl on any files that you have a patch
for.

./scripts/get_maintainer.pl -f drivers/clk/socfpga/clk.c
Dinh Nguyen <dinguyen@altera.com> (maintainer:ARM/SOCFPGA CLOCK...)
Mike Turquette <mturquette@linaro.org> (maintainer:COMMON CLK FRAMEWORK)
linux-kernel@vger.kernel.org (open list:COMMON CLK FRAMEWORK)

Thanks,
Dinh
> Regards,
> Steffen
> 

Re: [PATCH 1/3] clk : Clean up checkpatch warnings

[Pavel Machek]

On Tue 2014-07-22 14:20:32, Charles Manning wrote:
> Signed-off-by: Charles Manning <cdhmanning@gmail.com>

Acked-by: Pavel Machek <pavel@denx.de>
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/3] clk: Prevent a hanging pointer being abused

[Pavel Machek]

On Tue 2014-07-22 14:20:33, Charles Manning wrote:
> Clock init structs are normally created on the stack.
> 
> If the pointer is left intact after clk_register then there is
> opportunity for clk drivers to dereference the pointer.
> 
> This was causing a problem in socfpga/clk.c for instance.
> 
> Better to NULL out the pointer so it can't be abused.
> 
> Signed-off-by: Charles Manning <cdhmanning@gmail.com>

Acked-by: Pavel Machek <pavel@denx.de>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html