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

[Patch 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 clk_sel bits in bootloaders, there 
are high probablity of returning 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.

Patch series fixes the bug and also minor optimzation while re-parenting orphan clk.
 
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
^M[    0.000000] LR is at __clk_init+0x16c/0x3cc^M
^M[    0.000000] pc : [<c033af7c>]    lr : [<c04ca314>]    psr: 600001d3^M
^M[    0.000000] sp : c09d1f70  ip : c0a12b3c  fp : 00000000^M
^M[    0.000000] r10: 00000000  r9 : c0a0f8b4  r8 : 8000406a^M
^M[    0.000000] r7 : c09d1fe4  r6 : c0a0ff40  r5 : c102888c  r4 : c0a0ff40^M
^M[    0.000000] r3 : 00000000  r2 : 00000067  r1 : 00000000  r0 : c0731c14^M
^M[    0.000000] Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel^M
^M[    0.000000] Control: 10c53c7d  Table: 8000406a  DAC: 00000017^M
^M[    0.000000] Process swapper (pid: 0, stack limit = 0xc09d0240)^M
^M[    0.000000] Stack: (0xc09d1f70 to 0xc09d2000)^M
^M[    0.000000] 1f60:                                     00000000 c05c0518 c0731dd4 c0a0c9f8^M
^M[    0.000000] 1f80: c0a0d340 c0838f58 c09d1fe4 8000406a 412fc0f2 00000000 00000000 c0814370^M
^M[    0.000000] 1fa0: 00000005 c0837140 c0a92b78 c0806124 00000000 c09d1fc4 c100d780 c09d0000^M
^M[    0.000000] 1fc0: 00000001 00000000 c09dd6c4 c0802728 00000000 00000000 00000000 00000000^M
^M[    0.000000] 1fe0: 00000000 c0838f58 10c53c7d c09d894c c0838f54 80008078 00000000 00000000^M
^M[    0.000000] [<c033af7c>] (strcmp+0x8/0x34) from [<c04ca314>] (__clk_init+0x16c/0x3cc)^M
^M[    0.000000] [<c04ca314>] (__clk_init+0x16c/0x3cc) from [<c0814370>] (dra7xx_clk_init+0x5c/0xa8)^M
^M[    0.000000] [<c0814370>] (dra7xx_clk_init+0x5c/0xa8) from [<c0806124>] (setup_arch+0x148/0x194)^M
^M[    0.000000] [<c0806124>] (setup_arch+0x148/0x194) from [<c0802728>] (start_kernel+0x80/0x2b8)^M
^M[    0.000000] [<c0802728>] (start_kernel+0x80/0x2b8) from [<80008078>] (0x80008078)^M
^M[    0.000000] Code: e8bd0070 e12fff1e e3a03000 e7d02003 (e7d1c003) ^M
^M[    0.000000] ---[ end trace 1b75b31a2719ed1c ]---^M
^M[    0.000000] Kernel panic - not syncing: Fatal exception^M 

Note:
1) With omap2plus_defconfig following warning are generated with the patches applied, will fix it on
reviewing the patches series. 
2) Other platforms too might have similar warnings.    

arch/arm/mach-omap2/cclock2420_data.c:116: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock2420_data.c:369: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock2420_data.c:417: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock2420_data.c:745: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock2430_data.c:116: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock2430_data.c:349: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock2430_data.c:397: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock2430_data.c:724: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:118: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:257: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:367: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:585: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:619: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:1176: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:2600: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock3xxx_data.c:3091: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock44xx_data.c:159: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock44xx_data.c:257: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock44xx_data.c:327: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock44xx_data.c:334: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock44xx_data.c:398: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock44xx_data.c:614: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock44xx_data.c:1329: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock33xx_data.c:105: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock33xx_data.c:182: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock33xx_data.c:225: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock33xx_data.c:481: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock33xx_data.c:601: warning: initialization from incompatible pointer type
arch/arm/mach-omap2/cclock33xx_data.c:644: warning: initialization from incompatible pointer type


Ambresh K (3):
  clk: fix clk_mux_get_parent return's signed value
  clk: skip re-parenting orphan clk
  clk: Avoid re-parenting orphan clk's having invalid parent index.

 drivers/clk/clk-mux.c        |    2 +-
 drivers/clk/clk.c            |   37 ++++++++++++++++++++++++++++++++++---
 include/linux/clk-provider.h |    4 ++--
 3 files changed, 37 insertions(+), 6 deletions(-)

-- 
1.7.4.1

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

[Ambresh K]

From: Ambresh K <ambresh@ti.com>

If for some reason, the value read from clksel field return
erroneous due to HW bug or improper configuration, then
clk_mux_get_parent should return appropriate error's.

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 |    4 ++--
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 25b1734..be0857e 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 934cfd1..c187321 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 clk_sel index(%d)\n",
+				__func__, clk->name, index);
+		goto out;
+	}
 
 	if (!clk->parents)
 		clk->parents =
@@ -1632,6 +1637,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("%s: orphan clk(%s) 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 1186098..96337a1 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -80,7 +80,7 @@ struct clk_hw;
  * 		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
+ * 		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 2/3] clk: skip re-parenting orphan clk

[Ambresh K]

From: Ambresh K <ambresh@ti.com>

If clk is same as orphan clk than 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 c187321..f4d2c73 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1635,6 +1635,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 3/3] clk: Avoid re-parenting orphan clk's having invalid parent index.

[Ambresh K]

From: Ambresh K <ambresh@ti.com>

Add orhan clk nodes having invalid parent index to list and use
the list to skip re-parenting orphan clk having invalid parents.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f4d2c73..54d2aa3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -32,6 +32,7 @@ static int enable_refcnt;
 
 static HLIST_HEAD(clk_root_list);
 static HLIST_HEAD(clk_orphan_list);
+static HLIST_HEAD(clk_orphan_invalid_parent);
 static LIST_HEAD(clk_notifier_list);
 
 /***           locking             ***/
@@ -1532,8 +1533,8 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
  */
 int __clk_init(struct device *dev, struct clk *clk)
 {
-	int i, ret = 0;
-	struct clk *orphan;
+	int i, ret = 0, skip = 0;
+	struct clk *orphan, *has_invalid_parent;
 	struct hlist_node *tmp2;
 
 	if (!clk)
@@ -1639,11 +1640,27 @@ int __clk_init(struct device *dev, struct clk *clk)
 		if (!strcmp(clk->name, orphan->name))
 			continue;
 
+		/* Skip iteration if orphan has invalid parent */
+		hlist_for_each_entry(has_invalid_parent,
+				&clk_orphan_invalid_parent, child_node) {
+			if (!strcmp(orphan->name, has_invalid_parent->name)) {
+				skip = 1;
+				break;
+			}
+		}
+
+		if (skip) {
+			skip = 0;
+			continue;
+		}
+
 		if (orphan->ops->get_parent) {
 			i = orphan->ops->get_parent(orphan->hw);
 			if (i < 0) {
 				pr_err("%s: orphan clk(%s) invalid parent\n",
 						__func__, orphan->name);
+				hlist_add_head(&orphan->child_node,
+						&clk_orphan_invalid_parent);
 				continue;
 			}
 			if (!strcmp(clk->name, orphan->parent_names[i]))
-- 
1.7.4.1

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

[skannan at codeaurora.org]

Ambresh K wrote:
> From: Ambresh K <ambresh@ti.com>
>
> If clk is same as orphan clk than skip the iteration, there

Typo: than => then

> 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 c187321..f4d2c73 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1635,6 +1635,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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[Patch 3/3] clk: Avoid re-parenting orphan clk's having invalid parent index.

[Mike Turquette]

Quoting Ambresh K (2013-05-01 23:25:29)
> From: Ambresh K <ambresh@ti.com>
> 
> Add orhan clk nodes having invalid parent index to list and use
> the list to skip re-parenting orphan clk having invalid parents.
> 
> Signed-off-by: Ambresh K <ambresh@ti.com>
> ---
>  drivers/clk/clk.c |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f4d2c73..54d2aa3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -32,6 +32,7 @@ static int enable_refcnt;
>  
>  static HLIST_HEAD(clk_root_list);
>  static HLIST_HEAD(clk_orphan_list);
> +static HLIST_HEAD(clk_orphan_invalid_parent);

Why not re-use the clk_orphan_list?  Having an invalid value programmed
into the hardware for selecting a parent essetially orphans a clock
from a software point of view.  Is there a specific need for the new
list?

Regards,
Mike

>  static LIST_HEAD(clk_notifier_list);
>  
>  /***           locking             ***/
> @@ -1532,8 +1533,8 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
>   */
>  int __clk_init(struct device *dev, struct clk *clk)
>  {
> -       int i, ret = 0;
> -       struct clk *orphan;
> +       int i, ret = 0, skip = 0;
> +       struct clk *orphan, *has_invalid_parent;
>         struct hlist_node *tmp2;
>  
>         if (!clk)
> @@ -1639,11 +1640,27 @@ int __clk_init(struct device *dev, struct clk *clk)
>                 if (!strcmp(clk->name, orphan->name))
>                         continue;
>  
> +               /* Skip iteration if orphan has invalid parent */
> +               hlist_for_each_entry(has_invalid_parent,
> +                               &clk_orphan_invalid_parent, child_node) {
> +                       if (!strcmp(orphan->name, has_invalid_parent->name)) {
> +                               skip = 1;
> +                               break;
> +                       }
> +               }
> +
> +               if (skip) {
> +                       skip = 0;
> +                       continue;
> +               }
> +
>                 if (orphan->ops->get_parent) {
>                         i = orphan->ops->get_parent(orphan->hw);
>                         if (i < 0) {
>                                 pr_err("%s: orphan clk(%s) invalid parent\n",
>                                                 __func__, orphan->name);
> +                               hlist_add_head(&orphan->child_node,
> +                                               &clk_orphan_invalid_parent);
>                                 continue;
>                         }
>                         if (!strcmp(clk->name, orphan->parent_names[i]))
> -- 
> 1.7.4.1

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

[Mike Turquette]

Quoting Ambresh K (2013-05-01 23:25:27)
> From: Ambresh K <ambresh@ti.com>
> 
> If for some reason, the value read from clksel field return
> erroneous due to HW bug or improper configuration, then
> clk_mux_get_parent should return appropriate error's.
> 

clksel is an omap-centric term.  How about:

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

The general approach looks good to me.  Can you submit a V2 which
removes all of the clksel-isms and updates definitions for .get_parent
functions to avoid the warnings you mentioned in the cover letter?  You
might find cocinelle useful for that last task.

Regards,
Mike

> 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 |    4 ++--
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> index 25b1734..be0857e 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 934cfd1..c187321 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 clk_sel index(%d)\n",
> +                               __func__, clk->name, index);
> +               goto out;
> +       }
>  
>         if (!clk->parents)
>                 clk->parents =
> @@ -1632,6 +1637,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("%s: orphan clk(%s) 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 1186098..96337a1 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -80,7 +80,7 @@ struct clk_hw;
>   *             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
> + *             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 1/3] clk: fix clk_mux_get_parent return's signed value

[Ambresh K]

> 
> clksel is an omap-centric term.  How about:
> 
> "clk_mux_get_parent should return an error if the value read from the
> register is erroneous."
> 

Make sense, will fix it.   

> The general approach looks good to me.  Can you submit a V2 which
> removes all of the clksel-isms and updates definitions for .get_parent
> functions to avoid the warnings you mentioned in the cover letter?  You
> might find cocinelle useful for that last task.
> 

Thanks! 
Will fix them and add your ack to it? 


> Regards,
> Mike
> 
>> 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 |    4 ++--
>>  3 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
>> index 25b1734..be0857e 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 934cfd1..c187321 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 clk_sel index(%d)\n",
>> +                               __func__, clk->name, index);
>> +               goto out;
>> +       }
>>  
>>         if (!clk->parents)
>>                 clk->parents =
>> @@ -1632,6 +1637,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("%s: orphan clk(%s) 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 1186098..96337a1 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -80,7 +80,7 @@ struct clk_hw;
>>   *             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
>> + *             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 1/3] clk: fix clk_mux_get_parent return's signed value

[Mike Turquette]

On Mon, Jun 3, 2013 at 11:27 PM, Ambresh K <ambresh@ti.com> wrote:
>
>>
>> clksel is an omap-centric term.  How about:
>>
>> "clk_mux_get_parent should return an error if the value read from the
>> register is erroneous."
>>
>
> Make sense, will fix it.
>
>> The general approach looks good to me.  Can you submit a V2 which
>> removes all of the clksel-isms and updates definitions for .get_parent
>> functions to avoid the warnings you mentioned in the cover letter?  You
>> might find cocinelle useful for that last task.
>>
>
> Thanks!
> Will fix them and add your ack to it?

Great, thanks!  Don't worry about the Ack.  I'll take the patch
through the clk tree so it will have my SoB.

Regards,
Mike

>
>
>> Regards,
>> Mike
>>
>>> 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 |    4 ++--
>>>  3 files changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
>>> index 25b1734..be0857e 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 934cfd1..c187321 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 clk_sel index(%d)\n",
>>> +                               __func__, clk->name, index);
>>> +               goto out;
>>> +       }
>>>
>>>         if (!clk->parents)
>>>                 clk->parents =
>>> @@ -1632,6 +1637,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("%s: orphan clk(%s) 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 1186098..96337a1 100644
>>> --- a/include/linux/clk-provider.h
>>> +++ b/include/linux/clk-provider.h
>>> @@ -80,7 +80,7 @@ struct clk_hw;
>>>   *             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
>>> + *             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 3/3] clk: Avoid re-parenting orphan clk's having invalid parent index.

[Ambresh K]

On Wednesday 29 May 2013 12:48 PM, Mike Turquette wrote:
> Quoting Ambresh K (2013-05-01 23:25:29)
>> From: Ambresh K <ambresh@ti.com>
>>
>> Add orhan clk nodes having invalid parent index to list and use
>> the list to skip re-parenting orphan clk having invalid parents.
>>
>> Signed-off-by: Ambresh K <ambresh@ti.com>
>> ---
>>  drivers/clk/clk.c |   21 +++++++++++++++++++--
>>  1 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index f4d2c73..54d2aa3 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -32,6 +32,7 @@ static int enable_refcnt;
>>  
>>  static HLIST_HEAD(clk_root_list);
>>  static HLIST_HEAD(clk_orphan_list);
>> +static HLIST_HEAD(clk_orphan_invalid_parent);
> 
> Why not re-use the clk_orphan_list?  Having an invalid value programmed
> into the hardware for selecting a parent essetially orphans a clock
> from a software point of view.  Is there a specific need for the new
> list?

Sorry for not being descriptive in commit message.  

a) Avoids unnecessary re-parenting cycle for orphan clock's with invalid parent for every clock

b) With patch [1/3], after a clk with invalid parent was encountered, for every clk registered thereafter seeing following logs. 
<Snippet>
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
[    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent  

Please advice, if can be handled better. 

Thanks, 
Ambresh


> 
> Regards,
> Mike
> 
>>  static LIST_HEAD(clk_notifier_list);
>>  
>>  /***           locking             ***/
>> @@ -1532,8 +1533,8 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
>>   */
>>  int __clk_init(struct device *dev, struct clk *clk)
>>  {
>> -       int i, ret = 0;
>> -       struct clk *orphan;
>> +       int i, ret = 0, skip = 0;
>> +       struct clk *orphan, *has_invalid_parent;
>>         struct hlist_node *tmp2;
>>  
>>         if (!clk)
>> @@ -1639,11 +1640,27 @@ int __clk_init(struct device *dev, struct clk *clk)
>>                 if (!strcmp(clk->name, orphan->name))
>>                         continue;
>>  
>> +               /* Skip iteration if orphan has invalid parent */
>> +               hlist_for_each_entry(has_invalid_parent,
>> +                               &clk_orphan_invalid_parent, child_node) {
>> +                       if (!strcmp(orphan->name, has_invalid_parent->name)) {
>> +                               skip = 1;
>> +                               break;
>> +                       }
>> +               }
>> +
>> +               if (skip) {
>> +                       skip = 0;
>> +                       continue;
>> +               }
>> +
>>                 if (orphan->ops->get_parent) {
>>                         i = orphan->ops->get_parent(orphan->hw);
>>                         if (i < 0) {
>>                                 pr_err("%s: orphan clk(%s) invalid parent\n",
>>                                                 __func__, orphan->name);
>> +                               hlist_add_head(&orphan->child_node,
>> +                                               &clk_orphan_invalid_parent);
>>                                 continue;
>>                         }
>>                         if (!strcmp(clk->name, orphan->parent_names[i]))
>> -- 
>> 1.7.4.1
> 

[Patch 3/3] clk: Avoid re-parenting orphan clk's having invalid parent index.

[Mike Turquette]

Quoting Ambresh K (2013-06-04 00:16:32)
> On Wednesday 29 May 2013 12:48 PM, Mike Turquette wrote:
> > Quoting Ambresh K (2013-05-01 23:25:29)
> >> From: Ambresh K <ambresh@ti.com>
> >>
> >> Add orhan clk nodes having invalid parent index to list and use
> >> the list to skip re-parenting orphan clk having invalid parents.
> >>
> >> Signed-off-by: Ambresh K <ambresh@ti.com>
> >> ---
> >>  drivers/clk/clk.c |   21 +++++++++++++++++++--
> >>  1 files changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index f4d2c73..54d2aa3 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -32,6 +32,7 @@ static int enable_refcnt;
> >>  
> >>  static HLIST_HEAD(clk_root_list);
> >>  static HLIST_HEAD(clk_orphan_list);
> >> +static HLIST_HEAD(clk_orphan_invalid_parent);
> > 
> > Why not re-use the clk_orphan_list?  Having an invalid value programmed
> > into the hardware for selecting a parent essetially orphans a clock
> > from a software point of view.  Is there a specific need for the new
> > list?
> 
> Sorry for not being descriptive in commit message.  
> 
> a) Avoids unnecessary re-parenting cycle for orphan clock's with invalid parent for every clock
> 

True, but this is a minor optimisation.  If this is a big optimization
for you then you really need to fix your bootloader.  We shouldn't be
optimizing slow error paths just because we refuse to fix the errors.

> b) With patch [1/3], after a clk with invalid parent was encountered, for every clk registered thereafter seeing following logs. 
> <Snippet>
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent  
> 
> Please advice, if can be handled better. 
> 

First off, I don't think we should create new structures to work around
bugs that should be fixed.  pr_err_once() will let us know something is
wrong and won't flood the log.  Even then I'm inclined to say that
flooding the log is OK and will motivate you to fix up your bootloader.
Error prints are there for a reason.

Secondly, I spent like 10 minutes looking at this code and I'm still
confused.  For a clock with invalid parent programming, are you adding
it to BOTH the orphan list and the has_invalid_parent list?  Why?  Is
this just avoid the spurious prints?  For everyone clock registered we
walk the list of orphans to see if that orphan can be reparented.  This
patch adds another nested list walk (likely a short list) for each of
those orphans in the first list walk, so it starts to look like O(n^2).
I don't like it.

I think the first two patches in the series look good, but unless I am
misunderstanding this patch I feel that it can be dropped entirely.

Regards,
Mike

> Thanks, 
> Ambresh
> 
> 
> > 
> > Regards,
> > Mike
> > 
> >>  static LIST_HEAD(clk_notifier_list);
> >>  
> >>  /***           locking             ***/
> >> @@ -1532,8 +1533,8 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
> >>   */
> >>  int __clk_init(struct device *dev, struct clk *clk)
> >>  {
> >> -       int i, ret = 0;
> >> -       struct clk *orphan;
> >> +       int i, ret = 0, skip = 0;
> >> +       struct clk *orphan, *has_invalid_parent;
> >>         struct hlist_node *tmp2;
> >>  
> >>         if (!clk)
> >> @@ -1639,11 +1640,27 @@ int __clk_init(struct device *dev, struct clk *clk)
> >>                 if (!strcmp(clk->name, orphan->name))
> >>                         continue;
> >>  
> >> +               /* Skip iteration if orphan has invalid parent */
> >> +               hlist_for_each_entry(has_invalid_parent,
> >> +                               &clk_orphan_invalid_parent, child_node) {
> >> +                       if (!strcmp(orphan->name, has_invalid_parent->name)) {
> >> +                               skip = 1;
> >> +                               break;
> >> +                       }
> >> +               }
> >> +
> >> +               if (skip) {
> >> +                       skip = 0;
> >> +                       continue;
> >> +               }
> >> +
> >>                 if (orphan->ops->get_parent) {
> >>                         i = orphan->ops->get_parent(orphan->hw);
> >>                         if (i < 0) {
> >>                                 pr_err("%s: orphan clk(%s) invalid parent\n",
> >>                                                 __func__, orphan->name);
> >> +                               hlist_add_head(&orphan->child_node,
> >> +                                               &clk_orphan_invalid_parent);
> >>                                 continue;
> >>                         }
> >>                         if (!strcmp(clk->name, orphan->parent_names[i]))
> >> -- 
> >> 1.7.4.1
> >

[Patch 3/3] clk: Avoid re-parenting orphan clk's having invalid parent index.

[Ambresh K]

>> Sorry for not being descriptive in commit message.  
>>
>> a) Avoids unnecessary re-parenting cycle for orphan clock's with invalid parent for every clock
>>
> 
> True, but this is a minor optimisation.  If this is a big optimization
> for you then you really need to fix your bootloader.  We shouldn't be
> optimizing slow error paths just because we refuse to fix the errors.

Got it! Should be fixed in boot-loader. 

> 
>> b) With patch [1/3], after a clk with invalid parent was encountered, for every clk registered thereafter seeing following logs. 
>> <Snippet>
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent
>> [    0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent  
>>
>> Please advice, if can be handled better. 
>>
> 
> First off, I don't think we should create new structures to work around
> bugs that should be fixed.  pr_err_once() will let us know something is
> wrong and won't flood the log.  Even then I'm inclined to say that
> flooding the log is OK and will motivate you to fix up your bootloader.
> Error prints are there for a reason.
> 
> Secondly, I spent like 10 minutes looking at this code and I'm still
> confused.  For a clock with invalid parent programming, are you adding
> it to BOTH the orphan list and the has_invalid_parent list?  Why?  Is
> this just avoid the spurious prints?  For everyone clock registered we
> walk the list of orphans to see if that orphan can be reparented.  This
> patch adds another nested list walk (likely a short list) for each of
> those orphans in the first list walk, so it starts to look like O(n^2).
> I don't like it.
> 
> I think the first two patches in the series look good, but unless I am
> misunderstanding this patch I feel that it can be dropped entirely.
> 

Thanks for your time! 
Will drop this patch and send V2 for first 2 patches. 


Regards,
Ambresh