[PATCH V2 0/3] Fix to clk framework while handling orphan clks

[PATCH V2 0/3] Fix to clk framework while handling orphan clks

[Ambresh K]

From: Ambresh K <ambresh@ti.com>

On a possible HW bug or in-correct configuration of MUX register's in
bootloader; might return a value greater than available parent clocks
for a MUX clk.     
Sensing invalid parent index, clk_mux_get_parent returns -EINVALID. 
Due to function's "u8" return type it will get converted into signed value,
thus causing following pointer dereference on test platform.

M[    0.000000] dra7xx_clk_init: clk init (gpu_core_gclk_mux)^M
^M[    0.000000] Unable to handle kernel NULL pointer dereference at virtual
address 00000000^M
^M[    0.000000] pgd = c0004000^M
^M[    0.000000] [00000000] *pgd=00000000^M
^M[    0.000000] Internal error: Oops: 5 [#1] SMP ARM^M
^M[    0.000000] Modules linked in:^M
^M[    0.000000] CPU: 0    Not tainted  (3.8.4-gb746a7c-dirty #59)^M
^M[    0.000000] PC is at strcmp+0x8/0x34^M


Changes since v1:
- Fixed review comments.
- Fixed return type of .get_parent callback function.

Ambresh K (3):
  clk: fix clk_mux_get_parent return's signed value
  clk: skip re-parenting orphan clk
  ARM: OMAP2+: clk: Fix return type of callbacks

 arch/arm/mach-omap2/clkt_clksel.c |    2 +-
 arch/arm/mach-omap2/clkt_dpll.c   |    2 +-
 arch/arm/mach-omap2/clock.h       |    4 ++--
 drivers/clk/clk-mux.c             |    2 +-
 drivers/clk/clk.c                 |   16 +++++++++++++++-
 include/linux/clk-provider.h      |    6 +++---
 6 files changed, 23 insertions(+), 9 deletions(-)

-- 
1.7.4.1

[PATCH V2 1/3] clk: fix clk_mux_get_parent return's signed value

[Ambresh K]

From: Ambresh K <ambresh@ti.com>

clk_mux_get_parent should return an error if the value read
from the register is erroneous.

Currently if the value read is greater than the number of
available parents clk_mux_get_parent return's signed error
which will result in NULL pointer dereferencing in the
calling functions.

Signed-off-by: Ambresh K <ambresh@ti.com>
---
 drivers/clk/clk-mux.c        |    2 +-
 drivers/clk/clk.c            |   12 +++++++++++-
 include/linux/clk-provider.h |    6 +++---
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 614444c..001b4df 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -29,7 +29,7 @@
 
 #define to_clk_mux(_hw) container_of(_hw, struct clk_mux, hw)
 
-static u8 clk_mux_get_parent(struct clk_hw *hw)
+static int clk_mux_get_parent(struct clk_hw *hw)
 {
 	struct clk_mux *mux = to_clk_mux(hw);
 	int num_parents = __clk_get_num_parents(hw->clk);
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index edf3fe1..2842450 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1281,7 +1281,7 @@ EXPORT_SYMBOL_GPL(clk_get_parent);
 static struct clk *__clk_init_parent(struct clk *clk)
 {
 	struct clk *ret = NULL;
-	u8 index;
+	int index;
 
 	/* handle the trivial cases */
 
@@ -1309,6 +1309,11 @@ static struct clk *__clk_init_parent(struct clk *clk)
 	 */
 
 	index = clk->ops->get_parent(clk->hw);
+	if (index < 0) {
+		pr_err("%s: clk(%s) invalid parent index(%d)\n",
+				__func__, clk->name, index);
+		goto out;
+	}
 
 	if (!clk->parents)
 		clk->parents =
@@ -1630,6 +1635,11 @@ int __clk_init(struct device *dev, struct clk *clk)
 	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
 		if (orphan->ops->get_parent) {
 			i = orphan->ops->get_parent(orphan->hw);
+			if (i < 0) {
+				pr_err_once("%s: clk(%s) has invalid parent\n",
+						__func__, orphan->name);
+				continue;
+			}
 			if (!strcmp(clk->name, orphan->parent_names[i]))
 				__clk_reparent(orphan, clk);
 			continue;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1ec14a7..b01cbdb 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -79,8 +79,8 @@ struct clk_hw;
  * @round_rate:	Given a target rate as input, returns the closest rate actually
  * 		supported by the clock.
  *
- * @get_parent:	Queries the hardware to determine the parent of a clock.  The
- * 		return value is a u8 which specifies the index corresponding to
+ * @get_parent:	Queries the hardware to determine the parent of a clock. The
+ * 		return value which specifies the index corresponding to
  * 		the parent clock.  This index can be applied to either the
  * 		.parent_names or .parents arrays.  In short, this function
  * 		translates the parent value read from hardware into an array
@@ -127,7 +127,7 @@ struct clk_ops {
 	long		(*round_rate)(struct clk_hw *hw, unsigned long,
 					unsigned long *);
 	int		(*set_parent)(struct clk_hw *hw, u8 index);
-	u8		(*get_parent)(struct clk_hw *hw);
+	int		(*get_parent)(struct clk_hw *hw);
 	int		(*set_rate)(struct clk_hw *hw, unsigned long,
 				    unsigned long);
 	void		(*init)(struct clk_hw *hw);
-- 
1.7.4.1

[PATCH V2 2/3] clk: skip re-parenting orphan clk

[Ambresh K]

From: Ambresh K <ambresh@ti.com>

If clk is same as orphan clk then skip the iteration, there
by avoiding unnecessary look-up.

Signed-off-by: Ambresh K <ambresh@ti.com>
---
 drivers/clk/clk.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2842450..57bb94a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1633,6 +1633,10 @@ int __clk_init(struct device *dev, struct clk *clk)
 	 * this clock
 	 */
 	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
+		/* Skip if clk is same as orphan clk */
+		if (!strcmp(clk->name, orphan->name))
+			continue;
+
 		if (orphan->ops->get_parent) {
 			i = orphan->ops->get_parent(orphan->hw);
 			if (i < 0) {
-- 
1.7.4.1

[PATCH V2 3/3] ARM: OMAP2+: clk: Fix return type of callbacks

[Ambresh K]

From: Ambresh K <ambresh@ti.com>

clk_ops's .get_parent member data return's signed value.

Signed-off-by: Ambresh K <ambresh@ti.com>
---
 arch/arm/mach-omap2/clkt_clksel.c |    2 +-
 arch/arm/mach-omap2/clkt_dpll.c   |    2 +-
 arch/arm/mach-omap2/clock.h       |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/clkt_clksel.c b/arch/arm/mach-omap2/clkt_clksel.c
index 0ec9f6f..2773657 100644
--- a/arch/arm/mach-omap2/clkt_clksel.c
+++ b/arch/arm/mach-omap2/clkt_clksel.c
@@ -303,7 +303,7 @@ u32 omap2_clksel_round_rate_div(struct clk_hw_omap *clk,
  * way to return an error, so if we encounter an error, just WARN()
  * and pretend that we know that we're doing.
  */
-u8 omap2_clksel_find_parent_index(struct clk_hw *hw)
+int omap2_clksel_find_parent_index(struct clk_hw *hw)
 {
 	struct clk_hw_omap *clk = to_clk_hw_omap(hw);
 	const struct clksel *clks;
diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
index 924c230..54b8c49 100644
--- a/arch/arm/mach-omap2/clkt_dpll.c
+++ b/arch/arm/mach-omap2/clkt_dpll.c
@@ -186,7 +186,7 @@ static int _dpll_test_mult(int *m, int n, unsigned long *new_rate,
 }
 
 /* Public functions */
-u8 omap2_init_dpll_parent(struct clk_hw *hw)
+int omap2_init_dpll_parent(struct clk_hw *hw)
 {
 	struct clk_hw_omap *clk = to_clk_hw_omap(hw);
 	u32 v;
diff --git a/arch/arm/mach-omap2/clock.h b/arch/arm/mach-omap2/clock.h
index 7aa32cd..2784087 100644
--- a/arch/arm/mach-omap2/clock.h
+++ b/arch/arm/mach-omap2/clock.h
@@ -384,7 +384,7 @@ void __init omap2_clk_disable_clkdm_control(void);
 u32 omap2_clksel_round_rate_div(struct clk_hw_omap *clk,
 				unsigned long target_rate,
 				u32 *new_div);
-u8 omap2_clksel_find_parent_index(struct clk_hw *hw);
+int omap2_clksel_find_parent_index(struct clk_hw *hw);
 unsigned long omap2_clksel_recalc(struct clk_hw *hw, unsigned long parent_rate);
 long omap2_clksel_round_rate(struct clk_hw *hw, unsigned long target_rate,
 				unsigned long *parent_rate);
@@ -396,7 +396,7 @@ int omap2_clksel_set_parent(struct clk_hw *hw, u8 field_val);
 extern void omap2_clkt_iclk_allow_idle(struct clk_hw_omap *clk);
 extern void omap2_clkt_iclk_deny_idle(struct clk_hw_omap *clk);
 
-u8 omap2_init_dpll_parent(struct clk_hw *hw);
+int omap2_init_dpll_parent(struct clk_hw *hw);
 unsigned long omap2_get_dpll_rate(struct clk_hw_omap *clk);
 
 int omap2_dflt_clk_enable(struct clk_hw *hw);
-- 
1.7.4.1

Re: [PATCH V2 3/3] ARM: OMAP2+: clk: Fix return type of callbacks

[Ambresh K]

On Friday 21 June 2013 11:43 PM, Mike Turquette wrote:
> Quoting Ambresh K (2013-06-17 01:39:44)
>> From: Ambresh K <ambresh@ti.com>
>>
>> clk_ops's .get_parent member data return's signed value.
>>
>> Signed-off-by: Ambresh K <ambresh@ti.com>
> 
> Unsurprisingly I get the following errors when building
> multi_v7_defconfig:
> 
> drivers/clk/clk-composite.c: In function ‘clk_register_composite’:
> drivers/clk/clk-composite.c:148:33: warning: assignment from incompatible pointer type [enabled by default]
> drivers/clk/clk-prima2.c:411:2: warning: initialization from incompatible pointer type [enabled by default]
> drivers/clk/clk-prima2.c:411:2: warning: (near initialization for ‘msi_ops.get_parent’) [enabled by default]
> drivers/clk/clk-prima2.c:459:2: warning: initialization from incompatible pointer type [enabled by default]
> drivers/clk/clk-prima2.c:459:2: warning: (near initialization for ‘cpu_ops.get_parent’) [enabled by default]
> drivers/clk/clk-prima2.c:485:2: warning: initialization from incompatible pointer type [enabled by default]
> drivers/clk/clk-prima2.c:485:2: warning: (near initialization for ‘dmn_ops.get_parent’) [enabled by default]
> drivers/clk/clk-zynq.c:118:2: warning: initialization from incompatible pointer type [enabled by default]
> drivers/clk/clk-zynq.c:118:2: warning: (near initialization for ‘zynq_periph_clk_ops.get_parent’) [enabled by default]
> drivers/clk/clk-zynq.c:228:2: warning: initialization from incompatible pointer type [enabled by default]
> drivers/clk/clk-zynq.c:228:2: warning: (near initialization for ‘zynq_cpu_clk_ops.get_parent’) [enabled by default]
> 
> All definitions of .get_parent callbacks need to be updated, not just
> your platform of choice. And there may well be more than the ones above.
> 
> I'm not taking this series for 3.11. After the merge window can you
> resubmit this series with fixes for all of the .get_parent definitions?
> Not just for OMAP.
> 
> Otherwise the rest of the series looks good and I'll be happy to take
> this in towards 3.12.

Thanks Mike! Will fix for other platforms too and resend v3.

> 
> Thanks,
> Mike
> 
>> ---
>>  arch/arm/mach-omap2/clkt_clksel.c |    2 +-
>>  arch/arm/mach-omap2/clkt_dpll.c   |    2 +-
>>  arch/arm/mach-omap2/clock.h       |    4 ++--
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/clkt_clksel.c b/arch/arm/mach-omap2/clkt_clksel.c
>> index 0ec9f6f..2773657 100644
>> --- a/arch/arm/mach-omap2/clkt_clksel.c
>> +++ b/arch/arm/mach-omap2/clkt_clksel.c
>> @@ -303,7 +303,7 @@ u32 omap2_clksel_round_rate_div(struct clk_hw_omap *clk,
>>   * way to return an error, so if we encounter an error, just WARN()
>>   * and pretend that we know that we're doing.
>>   */
>> -u8 omap2_clksel_find_parent_index(struct clk_hw *hw)
>> +int omap2_clksel_find_parent_index(struct clk_hw *hw)
>>  {
>>         struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>>         const struct clksel *clks;
>> diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
>> index 924c230..54b8c49 100644
>> --- a/arch/arm/mach-omap2/clkt_dpll.c
>> +++ b/arch/arm/mach-omap2/clkt_dpll.c
>> @@ -186,7 +186,7 @@ static int _dpll_test_mult(int *m, int n, unsigned long *new_rate,
>>  }
>>  
>>  /* Public functions */
>> -u8 omap2_init_dpll_parent(struct clk_hw *hw)
>> +int omap2_init_dpll_parent(struct clk_hw *hw)
>>  {
>>         struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>>         u32 v;
>> diff --git a/arch/arm/mach-omap2/clock.h b/arch/arm/mach-omap2/clock.h
>> index 7aa32cd..2784087 100644
>> --- a/arch/arm/mach-omap2/clock.h
>> +++ b/arch/arm/mach-omap2/clock.h
>> @@ -384,7 +384,7 @@ void __init omap2_clk_disable_clkdm_control(void);
>>  u32 omap2_clksel_round_rate_div(struct clk_hw_omap *clk,
>>                                 unsigned long target_rate,
>>                                 u32 *new_div);
>> -u8 omap2_clksel_find_parent_index(struct clk_hw *hw);
>> +int omap2_clksel_find_parent_index(struct clk_hw *hw);
>>  unsigned long omap2_clksel_recalc(struct clk_hw *hw, unsigned long parent_rate);
>>  long omap2_clksel_round_rate(struct clk_hw *hw, unsigned long target_rate,
>>                                 unsigned long *parent_rate);
>> @@ -396,7 +396,7 @@ int omap2_clksel_set_parent(struct clk_hw *hw, u8 field_val);
>>  extern void omap2_clkt_iclk_allow_idle(struct clk_hw_omap *clk);
>>  extern void omap2_clkt_iclk_deny_idle(struct clk_hw_omap *clk);
>>  
>> -u8 omap2_init_dpll_parent(struct clk_hw *hw);
>> +int omap2_init_dpll_parent(struct clk_hw *hw);
>>  unsigned long omap2_get_dpll_rate(struct clk_hw_omap *clk);
>>  
>>  int omap2_dflt_clk_enable(struct clk_hw *hw);
>> -- 
>> 1.7.4.1
>