From: Sean Wang <[email protected]>
Fixed the signedness bug returning '(-22)' on the return type as u8 with
moving the sanity checker into clk_cpumux_set_parent() to ensure always
validity in clk_cpumux_get_parent() got called.
Fixes: commit 1e17de9049da ("clk: mediatek: add missing cpu mux causing Mediatek cpufreq can't work")
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Sean Wang <[email protected]>
---
drivers/clk/mediatek/clk-cpumux.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c
index edd8e69..c6a3a1a 100644
--- a/drivers/clk/mediatek/clk-cpumux.c
+++ b/drivers/clk/mediatek/clk-cpumux.c
@@ -27,7 +27,6 @@ static inline struct mtk_clk_cpumux *to_mtk_clk_cpumux(struct clk_hw *_hw)
static u8 clk_cpumux_get_parent(struct clk_hw *hw)
{
struct mtk_clk_cpumux *mux = to_mtk_clk_cpumux(hw);
- int num_parents = clk_hw_get_num_parents(hw);
unsigned int val;
regmap_read(mux->regmap, mux->reg, &val);
@@ -35,17 +34,18 @@ static u8 clk_cpumux_get_parent(struct clk_hw *hw)
val >>= mux->shift;
val &= mux->mask;
- if (val >= num_parents)
- return -EINVAL;
-
return val;
}
static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
{
struct mtk_clk_cpumux *mux = to_mtk_clk_cpumux(hw);
+ int num_parents = clk_hw_get_num_parents(hw);
u32 mask, val;
+ if (index >= num_parents)
+ return -EINVAL;
+
val = index << mux->shift;
mask = mux->mask << mux->shift;
--
2.7.4
On 07/11, [email protected] wrote:
> diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c
> index edd8e69..c6a3a1a 100644
> --- a/drivers/clk/mediatek/clk-cpumux.c
> +++ b/drivers/clk/mediatek/clk-cpumux.c
> @@ -27,7 +27,6 @@ static inline struct mtk_clk_cpumux *to_mtk_clk_cpumux(struct clk_hw *_hw)
> static u8 clk_cpumux_get_parent(struct clk_hw *hw)
> {
> struct mtk_clk_cpumux *mux = to_mtk_clk_cpumux(hw);
> - int num_parents = clk_hw_get_num_parents(hw);
> unsigned int val;
>
> regmap_read(mux->regmap, mux->reg, &val);
> @@ -35,17 +34,18 @@ static u8 clk_cpumux_get_parent(struct clk_hw *hw)
> val >>= mux->shift;
> val &= mux->mask;
>
> - if (val >= num_parents)
> - return -EINVAL;
> -
Yeah we really need to fix the get_parent() op to return a
clk_hw pointer instead. Time for another migration plan...
> return val;
> }
>
> static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
> {
> struct mtk_clk_cpumux *mux = to_mtk_clk_cpumux(hw);
> + int num_parents = clk_hw_get_num_parents(hw);
> u32 mask, val;
>
> + if (index >= num_parents)
> + return -EINVAL;
When would we call this function with an invalid index? The
framework should be making sure to only call it with an index
that's valid. So perhaps this hunk can be left out?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, 2017-07-12 at 16:17 -0700, Stephen Boyd wrote:
> On 07/11, [email protected] wrote:
> > diff --git a/drivers/clk/mediatek/clk-cpumux.c b/drivers/clk/mediatek/clk-cpumux.c
> > index edd8e69..c6a3a1a 100644
> > --- a/drivers/clk/mediatek/clk-cpumux.c
> > +++ b/drivers/clk/mediatek/clk-cpumux.c
> > @@ -27,7 +27,6 @@ static inline struct mtk_clk_cpumux *to_mtk_clk_cpumux(struct clk_hw *_hw)
> > static u8 clk_cpumux_get_parent(struct clk_hw *hw)
> > {
> > struct mtk_clk_cpumux *mux = to_mtk_clk_cpumux(hw);
> > - int num_parents = clk_hw_get_num_parents(hw);
> > unsigned int val;
> >
> > regmap_read(mux->regmap, mux->reg, &val);
> > @@ -35,17 +34,18 @@ static u8 clk_cpumux_get_parent(struct clk_hw *hw)
> > val >>= mux->shift;
> > val &= mux->mask;
> >
> > - if (val >= num_parents)
> > - return -EINVAL;
> > -
>
> Yeah we really need to fix the get_parent() op to return a
> clk_hw pointer instead. Time for another migration plan...
>
Agreed
Using clk_hw pointer as returned type is better
otherwise, core driver strongly depends on raw value from hardware
which easily breaks core driver's logic
> > return val;
> > }
> >
> > static int clk_cpumux_set_parent(struct clk_hw *hw, u8 index)
> > {
> > struct mtk_clk_cpumux *mux = to_mtk_clk_cpumux(hw);
> > + int num_parents = clk_hw_get_num_parents(hw);
> > u32 mask, val;
> >
> > + if (index >= num_parents)
> > + return -EINVAL;
>
> When would we call this function with an invalid index? The
> framework should be making sure to only call it with an index
> that's valid. So perhaps this hunk can be left out?
>
O.K. the hunk will be left out
core driver handles everything well when the appropriate number of
parents is registered.
>