[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

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

[Mike Turquette]

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

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

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