diff mbox

clk: mediatek: fixed static checker warning in clk_cpumux_get_parent call

Message ID 59fd4aa684632f1d9d2e7573a95bc42850e81690.1499701486.git.sean.wang@mediatek.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Sean Wang July 11, 2017, 2:43 a.m. UTC 
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(-)

Comments

Stephen Boyd July 12, 2017, 11:17 p.m. UTC | #1 
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?
Sean Wang July 13, 2017, 3:15 a.m. UTC | #2 
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.







> 


--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;