Fix to clk framework while handling orphan clks
Subsystems (5)
v2 cover Fix to clk framework while handling orphan clks
From: Ambresh K <[email protected]>
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(-)
Signature · 1 line
1.7.4.1
PATCH v2 1/3 clk: fix clk_mux_get_parent return's signed value
From: Ambresh K <[email protected]>
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.
--- drivers/clk/clk-mux.c | 2 +- drivers/clk/clk.c | 12 +++++++++++- include/linux/clk-provider.h | 6 +++--- 3 files changed, 15 insertions(+), 5 deletions(-)
drivers/clk/clk-mux.c +1 -1
@@ -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);
drivers/clk/clk.c +11 -1
@@ -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 ;
include/linux/clk-provider.h +3 -3
@@ -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);
Signature · 1 line
1.7.4.1
PATCH v2 2/3 clk: skip re-parenting orphan clk
In reply to Ambresh KFrom: Ambresh K <[email protected]>
If clk is same as orphan clk then skip the iteration, there by avoiding unnecessary look-up.
--- drivers/clk/clk.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/clk/clk.c +4 -0
@@ -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 ) {
Signature · 1 line
1.7.4.1
PATCH v2 3/3 ARM: OMAP2+: clk: Fix return type of callbacks
In reply to Ambresh KFrom: Ambresh K <[email protected]>
clk_ops's .get_parent member data return's signed value.
--- 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(-)
arch/arm/mach-omap2/clkt_clksel.c +1 -1
@@ -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;
arch/arm/mach-omap2/clkt_dpll.c +1 -1
@@ -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;
arch/arm/mach-omap2/clock.h +2 -2
@@ -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);
Signature · 1 line
1.7.4.1
PATCH v2 3/3 ARM: OMAP2+: clk: Fix return type of callbacks
Quoting Ambresh K (2013-06-17 01:39:44)
Quoted 5 lines
From: Ambresh K <[email protected]>
clk_ops's .get_parent member data return's signed value.
Signed-off-by Ambresh K <[email protected]>
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
Quoted 56 lines
--- 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(-)arch/arm/mach-omap2/clkt_clksel.c +1
-1@@ -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;arch/arm/mach-omap2/clkt_dpll.c +1
-1@@ -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;arch/arm/mach-omap2/clock.h +2
-2@@ -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);Signature · 1 line
1.7.4.1
Re: [PATCH V2 3/3] ARM: OMAP2+: clk: Fix return type of callbacks
Quoted 26 lines · Mike Turquette
Quoting Ambresh K (2013-06-17 01:39:44)
Quoted 5 lines
From: Ambresh K <[email protected]>
clk_ops's .get_parent member data return's signed value.
Signed-off-by Ambresh K <[email protected]>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.
Quoted 6 lines
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.
Quoted 61 lines
Thanks,
MikeQuoted 56 lines
--- 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(-)arch/arm/mach-omap2/clkt_clksel.c +1
-1@@ -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;arch/arm/mach-omap2/clkt_dpll.c +1
-1@@ -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;arch/arm/mach-omap2/clock.h +2
-2@@ -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);Signature · 1 line
1.7.4.1