From: Ambresh K <[email protected]>
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
From: Ambresh K <[email protected]>
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 <[email protected]>
---
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
From: Ambresh K <[email protected]>
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 <[email protected]>
---
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
From: Ambresh K <[email protected]>
If clk is same as orphan clk than skip the iteration, there
by avoiding unnecessary look-up.
Signed-off-by: Ambresh K <[email protected]>
---
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
Ambresh K wrote:
> From: Ambresh K <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
> [email protected]
> 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
>
> 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 <[email protected]>
>> ---
>> 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
>
On Mon, Jun 3, 2013 at 11:27 PM, Ambresh K <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
>>
On Wednesday 29 May 2013 12:48 PM, Mike Turquette wrote:
> Quoting Ambresh K (2013-05-01 23:25:29)
>> From: Ambresh K <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>
>> 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