[PATCH 1/2] drivers: clk: st: warn on iomap failure

[PATCH 1/2] drivers: clk: st: warn on iomap failure

[Nicholas Mc Guire]

 While the return value of clkgen_get_register_base() is being checked
at the call site, there is no indication of failure cause thus making
diagnosis of the issues hard. The WARN_ON() allows to determine the
cause of failure.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem found by an experimental coccinelle script

Patch was compile tested with: multi_v7_defconfig (implies
CONFIG_ARCH_STI=y)

Patch is against 4.18-rc4 (localversion-next is next-20180713)

 drivers/clk/st/clkgen-pll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index cbb5184..aeb30ab 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -647,6 +647,7 @@ static void __iomem * __init clkgen_get_register_base(
 		return NULL;
 
 	reg = of_iomap(pnode, 0);
+	WARN_ON(!reg);
 
 	of_node_put(pnode);
 	return reg;
-- 
2.1.4

[PATCH 2/2] drivers: clk: st: address sparse warnings

[Nicholas Mc Guire]

 Refactoring of code to make it more readable and at the same time make
sparse happy again.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

sparse complained about:
drivers/clk/st/clkgen-pll.c:225:12: warning: context imbalance in 'clkgen_pll_enable' - different lock contexts for basic block
drivers/clk/st/clkgen-pll.c:267:9: warning: context imbalance in 'clkgen_pll_disable' - different lock contexts for basic block
drivers/clk/st/clkgen-pll.c:413:9: warning: context imbalance in 'set_rate_stm_pll3200c32' - different lock contexts for basic block
drivers/clk/st/clkgen-pll.c:570:9: warning: context imbalance in 'set_rate_stm_pll4600c28' - different lock contexts for basic block

Which are technically false positives as the pll->lock which is being
checked is determined at configuration time, thus the locks are balanced.
Never the less the refactored code seems more readable and was 
commented to make it clear.

Patch was compile tested with: multi_v7_defconfig (implies
CONFIG_ARCH_STI=y)

Patch is against 4.18-rc4 (localversion-next is next-20180713)

 drivers/clk/st/clkgen-pll.c | 51 +++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index 7a7106dc..cbb5184 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -228,13 +228,13 @@ static int clkgen_pll_enable(struct clk_hw *hw)
 	unsigned long flags = 0;
 	int ret = 0;
 
-	if (pll->lock)
+	if (pll->lock) {
+		/* stih418 and stih407 */
 		spin_lock_irqsave(pll->lock, flags);
-
-	ret = __clkgen_pll_enable(hw);
-
-	if (pll->lock)
+		ret = __clkgen_pll_enable(hw);
 		spin_unlock_irqrestore(pll->lock, flags);
+	} else
+		ret = __clkgen_pll_enable(hw);
 
 	return ret;
 }
@@ -259,13 +259,13 @@ static void clkgen_pll_disable(struct clk_hw *hw)
 	struct clkgen_pll *pll = to_clkgen_pll(hw);
 	unsigned long flags = 0;
 
-	if (pll->lock)
+	if (pll->lock) {
+		/* stih418 and stih407 */
 		spin_lock_irqsave(pll->lock, flags);
-
-	__clkgen_pll_disable(hw);
-
-	if (pll->lock)
+		__clkgen_pll_disable(hw);
 		spin_unlock_irqrestore(pll->lock, flags);
+	} else
+		__clkgen_pll_disable(hw);
 }
 
 static int clk_pll3200c32_get_params(unsigned long input, unsigned long output,
@@ -400,15 +400,18 @@ static int set_rate_stm_pll3200c32(struct clk_hw *hw, unsigned long rate,
 
 	__clkgen_pll_disable(hw);
 
-	if (pll->lock)
+	if (pll->lock) {
+		/* stih407 and stih418 */
 		spin_lock_irqsave(pll->lock, flags);
-
-	CLKGEN_WRITE(pll, ndiv, pll->ndiv);
-	CLKGEN_WRITE(pll, idf, pll->idf);
-	CLKGEN_WRITE(pll, cp, pll->cp);
-
-	if (pll->lock)
+		CLKGEN_WRITE(pll, ndiv, pll->ndiv);
+		CLKGEN_WRITE(pll, idf, pll->idf);
+		CLKGEN_WRITE(pll, cp, pll->cp);
 		spin_unlock_irqrestore(pll->lock, flags);
+	} else {
+		CLKGEN_WRITE(pll, ndiv, pll->ndiv);
+		CLKGEN_WRITE(pll, idf, pll->idf);
+		CLKGEN_WRITE(pll, cp, pll->cp);
+	}
 
 	__clkgen_pll_enable(hw);
 
@@ -558,14 +561,16 @@ static int set_rate_stm_pll4600c28(struct clk_hw *hw, unsigned long rate,
 
 	__clkgen_pll_disable(hw);
 
-	if (pll->lock)
+	if (pll->lock) {
+		/* stih407 and stih418 */
 		spin_lock_irqsave(pll->lock, flags);
-
-	CLKGEN_WRITE(pll, ndiv, pll->ndiv);
-	CLKGEN_WRITE(pll, idf, pll->idf);
-
-	if (pll->lock)
+		CLKGEN_WRITE(pll, ndiv, pll->ndiv);
+		CLKGEN_WRITE(pll, idf, pll->idf);
 		spin_unlock_irqrestore(pll->lock, flags);
+	} else {
+		CLKGEN_WRITE(pll, ndiv, pll->ndiv);
+		CLKGEN_WRITE(pll, idf, pll->idf);
+	}
 
 	__clkgen_pll_enable(hw);
 
-- 
2.1.4

Re: [PATCH 2/2] drivers: clk: st: address sparse warnings

[Stephen Boyd]

Quoting Nicholas Mc Guire (2018-07-15 03:18:24)
>  Refactoring of code to make it more readable and at the same time make
> sparse happy again.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> sparse complained about:
> drivers/clk/st/clkgen-pll.c:225:12: warning: context imbalance in 'clkgen_pll_enable' - different lock contexts for basic block
> drivers/clk/st/clkgen-pll.c:267:9: warning: context imbalance in 'clkgen_pll_disable' - different lock contexts for basic block
> drivers/clk/st/clkgen-pll.c:413:9: warning: context imbalance in 'set_rate_stm_pll3200c32' - different lock contexts for basic block
> drivers/clk/st/clkgen-pll.c:570:9: warning: context imbalance in 'set_rate_stm_pll4600c28' - different lock contexts for basic block
> 
> Which are technically false positives as the pll->lock which is being
> checked is determined at configuration time, thus the locks are balanced.
> Never the less the refactored code seems more readable and was 
> commented to make it clear.

This stuff above should go into the commit text.

> 
> Patch was compile tested with: multi_v7_defconfig (implies
> CONFIG_ARCH_STI=y)
> 
> Patch is against 4.18-rc4 (localversion-next is next-20180713)
> 
>  drivers/clk/st/clkgen-pll.c | 51 +++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)

I believe this driver is in stasis mode. Not sure anyone is testing this
right now. Please Cc people who have worked on this driver, like Gabriel
Fernandez.
 
> 
> diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
> index 7a7106dc..cbb5184 100644
> --- a/drivers/clk/st/clkgen-pll.c
> +++ b/drivers/clk/st/clkgen-pll.c
> @@ -228,13 +228,13 @@ static int clkgen_pll_enable(struct clk_hw *hw)
>         unsigned long flags = 0;
>         int ret = 0;
>  
> -       if (pll->lock)
> +       if (pll->lock) {
> +               /* stih418 and stih407 */
>                 spin_lock_irqsave(pll->lock, flags);
> -
> -       ret = __clkgen_pll_enable(hw);
> -
> -       if (pll->lock)
> +               ret = __clkgen_pll_enable(hw);
>                 spin_unlock_irqrestore(pll->lock, flags);
> +       } else
> +               ret = __clkgen_pll_enable(hw);
>  
>         return ret;
>  }
> @@ -259,13 +259,13 @@ static void clkgen_pll_disable(struct clk_hw *hw)
>         struct clkgen_pll *pll = to_clkgen_pll(hw);
>         unsigned long flags = 0;
>  
> -       if (pll->lock)
> +       if (pll->lock) {
> +               /* stih418 and stih407 */
>                 spin_lock_irqsave(pll->lock, flags);
> -
> -       __clkgen_pll_disable(hw);
> -
> -       if (pll->lock)
> +               __clkgen_pll_disable(hw);
>                 spin_unlock_irqrestore(pll->lock, flags);
> +       } else
> +               __clkgen_pll_disable(hw);
>  }
>  
>  static int clk_pll3200c32_get_params(unsigned long input, unsigned long output,
> @@ -400,15 +400,18 @@ static int set_rate_stm_pll3200c32(struct clk_hw *hw, unsigned long rate,
>  
>         __clkgen_pll_disable(hw);
>  
> -       if (pll->lock)
> +       if (pll->lock) {
> +               /* stih407 and stih418 */
>                 spin_lock_irqsave(pll->lock, flags);
> -
> -       CLKGEN_WRITE(pll, ndiv, pll->ndiv);
> -       CLKGEN_WRITE(pll, idf, pll->idf);
> -       CLKGEN_WRITE(pll, cp, pll->cp);
> -
> -       if (pll->lock)
> +               CLKGEN_WRITE(pll, ndiv, pll->ndiv);
> +               CLKGEN_WRITE(pll, idf, pll->idf);
> +               CLKGEN_WRITE(pll, cp, pll->cp);
>                 spin_unlock_irqrestore(pll->lock, flags);
> +       } else {
> +               CLKGEN_WRITE(pll, ndiv, pll->ndiv);
> +               CLKGEN_WRITE(pll, idf, pll->idf);
> +               CLKGEN_WRITE(pll, cp, pll->cp);
> +       }

Please don't duplicate this code. The sparse warnings can be fixed by
adding a fake lock acquire to the else of the if condition. We do this
in drivers/clk/clk.c so you should be able to copy it from there.

>  

Re: [PATCH 1/2] drivers: clk: st: warn on iomap failure

[Stephen Boyd]

Quoting Nicholas Mc Guire (2018-07-15 03:18:23)
>  While the return value of clkgen_get_register_base() is being checked
> at the call site, there is no indication of failure cause thus making
> diagnosis of the issues hard. The WARN_ON() allows to determine the
> cause of failure.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> Problem found by an experimental coccinelle script
> 
> Patch was compile tested with: multi_v7_defconfig (implies
> CONFIG_ARCH_STI=y)
> 
> Patch is against 4.18-rc4 (localversion-next is next-20180713)
> 
>  drivers/clk/st/clkgen-pll.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
> index cbb5184..aeb30ab 100644
> --- a/drivers/clk/st/clkgen-pll.c
> +++ b/drivers/clk/st/clkgen-pll.c
> @@ -647,6 +647,7 @@ static void __iomem * __init clkgen_get_register_base(
>                 return NULL;
>  
>         reg = of_iomap(pnode, 0);
> +       WARN_ON(!reg);
>  
>         of_node_put(pnode);
>         return reg;

Shouldn't the caller blow up on NULL pointer access? This patch doesn't
seem useful, sorry.

Re: [PATCH 1/2] drivers: clk: st: warn on iomap failure

[Nicholas Mc Guire]

On Wed, Jul 25, 2018 at 01:41:38PM -0700, Stephen Boyd wrote:
> Quoting Nicholas Mc Guire (2018-07-15 03:18:23)
> >  While the return value of clkgen_get_register_base() is being checked
> > at the call site, there is no indication of failure cause thus making
> > diagnosis of the issues hard. The WARN_ON() allows to determine the
> > cause of failure.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > Problem found by an experimental coccinelle script
> > 
> > Patch was compile tested with: multi_v7_defconfig (implies
> > CONFIG_ARCH_STI=y)
> > 
> > Patch is against 4.18-rc4 (localversion-next is next-20180713)
> > 
> >  drivers/clk/st/clkgen-pll.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
> > index cbb5184..aeb30ab 100644
> > --- a/drivers/clk/st/clkgen-pll.c
> > +++ b/drivers/clk/st/clkgen-pll.c
> > @@ -647,6 +647,7 @@ static void __iomem * __init clkgen_get_register_base(
> >                 return NULL;
> >  
> >         reg = of_iomap(pnode, 0);
> > +       WARN_ON(!reg);
> >  
> >         of_node_put(pnode);
> >         return reg;
> 
> Shouldn't the caller blow up on NULL pointer access? This patch doesn't
> seem useful, sorry.
>
if you look at the call chain then there is a check for !NULL along
the way - but never any information - no pr_*/printk or the like so
ultimately you would get a failure but not know where that failure came
from - the intent of the WARN_ON() is to allow you to locate the trigger
event. Blowing up with a BUG_ON() is not necessary as the call chain
does check for !NULL along the way.

thx!
hofrat

Re: [PATCH 2/2] drivers: clk: st: address sparse warnings

[Nicholas Mc Guire]

On Wed, Jul 25, 2018 at 01:40:53PM -0700, Stephen Boyd wrote:
> Quoting Nicholas Mc Guire (2018-07-15 03:18:24)
> >  Refactoring of code to make it more readable and at the same time make
> > sparse happy again.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > sparse complained about:
> > drivers/clk/st/clkgen-pll.c:225:12: warning: context imbalance in 'clkgen_pll_enable' - different lock contexts for basic block
> > drivers/clk/st/clkgen-pll.c:267:9: warning: context imbalance in 'clkgen_pll_disable' - different lock contexts for basic block
> > drivers/clk/st/clkgen-pll.c:413:9: warning: context imbalance in 'set_rate_stm_pll3200c32' - different lock contexts for basic block
> > drivers/clk/st/clkgen-pll.c:570:9: warning: context imbalance in 'set_rate_stm_pll4600c28' - different lock contexts for basic block
> > 
> > Which are technically false positives as the pll->lock which is being
> > checked is determined at configuration time, thus the locks are balanced.
> > Never the less the refactored code seems more readable and was 
> > commented to make it clear.
> 
> This stuff above should go into the commit text.

ok - then I´ll resend that with that moved into the commit message.

> 
> > 
> > Patch was compile tested with: multi_v7_defconfig (implies
> > CONFIG_ARCH_STI=y)
> > 
> > Patch is against 4.18-rc4 (localversion-next is next-20180713)
> > 
> >  drivers/clk/st/clkgen-pll.c | 51 +++++++++++++++++++++++++--------------------
> >  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> I believe this driver is in stasis mode. Not sure anyone is testing this
> right now. Please Cc people who have worked on this driver, like Gabriel
> Fernandez.
>  
> > 
> > diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
> > index 7a7106dc..cbb5184 100644
> > --- a/drivers/clk/st/clkgen-pll.c
> > +++ b/drivers/clk/st/clkgen-pll.c
> > @@ -228,13 +228,13 @@ static int clkgen_pll_enable(struct clk_hw *hw)
> >         unsigned long flags = 0;
> >         int ret = 0;
> >  
> > -       if (pll->lock)
> > +       if (pll->lock) {
> > +               /* stih418 and stih407 */
> >                 spin_lock_irqsave(pll->lock, flags);
> > -
> > -       ret = __clkgen_pll_enable(hw);
> > -
> > -       if (pll->lock)
> > +               ret = __clkgen_pll_enable(hw);
> >                 spin_unlock_irqrestore(pll->lock, flags);
> > +       } else
> > +               ret = __clkgen_pll_enable(hw);
> >  
> >         return ret;
> >  }
> > @@ -259,13 +259,13 @@ static void clkgen_pll_disable(struct clk_hw *hw)
> >         struct clkgen_pll *pll = to_clkgen_pll(hw);
> >         unsigned long flags = 0;
> >  
> > -       if (pll->lock)
> > +       if (pll->lock) {
> > +               /* stih418 and stih407 */
> >                 spin_lock_irqsave(pll->lock, flags);
> > -
> > -       __clkgen_pll_disable(hw);
> > -
> > -       if (pll->lock)
> > +               __clkgen_pll_disable(hw);
> >                 spin_unlock_irqrestore(pll->lock, flags);
> > +       } else
> > +               __clkgen_pll_disable(hw);
> >  }
> >  
> >  static int clk_pll3200c32_get_params(unsigned long input, unsigned long output,
> > @@ -400,15 +400,18 @@ static int set_rate_stm_pll3200c32(struct clk_hw *hw, unsigned long rate,
> >  
> >         __clkgen_pll_disable(hw);
> >  
> > -       if (pll->lock)
> > +       if (pll->lock) {
> > +               /* stih407 and stih418 */
> >                 spin_lock_irqsave(pll->lock, flags);
> > -
> > -       CLKGEN_WRITE(pll, ndiv, pll->ndiv);
> > -       CLKGEN_WRITE(pll, idf, pll->idf);
> > -       CLKGEN_WRITE(pll, cp, pll->cp);
> > -
> > -       if (pll->lock)
> > +               CLKGEN_WRITE(pll, ndiv, pll->ndiv);
> > +               CLKGEN_WRITE(pll, idf, pll->idf);
> > +               CLKGEN_WRITE(pll, cp, pll->cp);
> >                 spin_unlock_irqrestore(pll->lock, flags);
> > +       } else {
> > +               CLKGEN_WRITE(pll, ndiv, pll->ndiv);
> > +               CLKGEN_WRITE(pll, idf, pll->idf);
> > +               CLKGEN_WRITE(pll, cp, pll->cp);
> > +       }
> 
> Please don't duplicate this code. The sparse warnings can be fixed by
> adding a fake lock acquire to the else of the if condition. We do this
> in drivers/clk/clk.c so you should be able to copy it from there.

the duplication is not a mater of the sparse warning only - the inetnt was
to improve readability - atleast for the one-line cases it seems more 
readable to have it this way than to have the two lock checks - notably
as you can then comment what the difference here really is.

I agree that for this case with the three lines in the body it is not
that reasonable - this was simply a matter of consistency as the other lock
checks were moved into a if-else construct for readability and commenting.

thx!
hofrat