[PATCH] clk: mediatek: fixed static checker warning in clk_cpumux_get_parent call

[PATCH] clk: mediatek: fixed static checker warning in clk_cpumux_get_parent call

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

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 <dan.carpenter@oracle.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 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

[PATCH] clk: mediatek: fixed static checker warning in clk_cpumux_get_parent call

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

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 <dan.carpenter@oracle.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 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

Re: [PATCH] clk: mediatek: fixed static checker warning in clk_cpumux_get_parent call

[Stephen Boyd]

On 07/11, sean.wang@mediatek.com 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

Re: [PATCH] clk: mediatek: fixed static checker warning in clk_cpumux_get_parent call

[Sean Wang]

On Wed, 2017-07-12 at 16:17 -0700, Stephen Boyd wrote:
> On 07/11, sean.wang@mediatek.com 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.







> 

Re: [PATCH] clk: mediatek: fixed static checker warning in clk_cpumux_get_parent call

[Sean Wang]

On Wed, 2017-07-12 at 16:17 -0700, Stephen Boyd wrote:
> On 07/11, sean.wang@mediatek.com 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.







>