[PATCH] hwrng: stm32 - fix build warning

[PATCH] hwrng: stm32 - fix build warning

[Sudip Mukherjee]

We have been getting build warning about:
drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
					uninitialized in this function

On checking the code it turns out that sr can never be used
uninitialized as sr is getting initialized in the while loop and while
loop will always execute as the minimum value of max can be 32.
So just initialize sr to 0 while declaring it to silence the compiler.

Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
---

build log at:
https://travis-ci.org/sudipm-mukherjee/parport/jobs/132180906

 drivers/char/hw_random/stm32-rng.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
index 92a8106..0533370 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct stm32_rng_private *priv =
 	    container_of(rng, struct stm32_rng_private, rng);
-	u32 sr;
+	u32 sr = 0;
 	int retval = 0;
 
 	pm_runtime_get_sync((struct device *) priv->rng.priv);
-- 
1.9.1

[PATCH] hwrng: stm32 - fix build warning

[Arnd Bergmann]

On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
> We have been getting build warning about:
> drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
> drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
> 					uninitialized in this function
> 
> On checking the code it turns out that sr can never be used
> uninitialized as sr is getting initialized in the while loop and while
> loop will always execute as the minimum value of max can be 32.
> So just initialize sr to 0 while declaring it to silence the compiler.
> 
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> ---

I notice that you are using a really old compiler. While this warning
seems to be valid in the sense that the compiler should figure out that
the variable might be used uninitialized, please update your toolchain
before reporting other such problems, as gcc-4.6 had a lot more false
positives that newer ones (5.x or 6.x) have.

> 
> build log at:
> https://travis-ci.org/sudipm-mukherjee/parport/jobs/132180906
> 
>  drivers/char/hw_random/stm32-rng.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
> index 92a8106..0533370 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>  {
>  	struct stm32_rng_private *priv =
>  	    container_of(rng, struct stm32_rng_private, rng);
> -	u32 sr;
> +	u32 sr = 0;
>  	int retval = 0;
>  
>  	pm_runtime_get_sync((struct device *) priv->rng.priv);

Does this work as well?

diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..5c836b0afa40 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 		max -= sizeof(u32);
 	}
 
-	if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
+	if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
 		      "bad RNG status - %x\n", sr))
 		writel_relaxed(0, priv->base + RNG_SR);
 
I think it would be nicer to not add a bogus initialization.

	Arnd

[PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-23 22:35 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>> We have been getting build warning about:
>> drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
>> drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
>>                                       uninitialized in this function
>>
>> On checking the code it turns out that sr can never be used
>> uninitialized as sr is getting initialized in the while loop and while
>> loop will always execute as the minimum value of max can be 32.
>> So just initialize sr to 0 while declaring it to silence the compiler.
>>
>> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>> ---
>
> I notice that you are using a really old compiler. While this warning
> seems to be valid in the sense that the compiler should figure out that
> the variable might be used uninitialized, please update your toolchain
> before reporting other such problems, as gcc-4.6 had a lot more false
> positives that newer ones (5.x or 6.x) have.
>
>>
>> build log at:
>> https://travis-ci.org/sudipm-mukherjee/parport/jobs/132180906
>>
>>  drivers/char/hw_random/stm32-rng.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
>> index 92a8106..0533370 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>>  {
>>       struct stm32_rng_private *priv =
>>           container_of(rng, struct stm32_rng_private, rng);
>> -     u32 sr;
>> +     u32 sr = 0;
>>       int retval = 0;
>>
>>       pm_runtime_get_sync((struct device *) priv->rng.priv);
>
> Does this work as well?
>
> diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..5c836b0afa40 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>                 max -= sizeof(u32);
>         }
>
> -       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> +       if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
>                       "bad RNG status - %x\n", sr))
>                 writel_relaxed(0, priv->base + RNG_SR);
>
> I think it would be nicer to not add a bogus initialization.
Hmm, no sure this nicer.
The while loop can break before retval is incremented when sr value is
not expected (sr != RNG_SR_DRDY).
In that case, we certainly want to print sr value.

Maybe the better way is just to initialize sr with status register content?

diff --git a/drivers/char/hw_random/stm32-rng.c
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..07a6659d0fe6 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -57,8 +57,8 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)

        pm_runtime_get_sync((struct device *) priv->rng.priv);

+       sr = readl_relaxed(priv->base + RNG_SR);
        while (max > sizeof(u32)) {
-               sr = readl_relaxed(priv->base + RNG_SR);
                if (!sr && wait) {
                        unsigned int timeout = RNG_TIMEOUT;

@@ -77,6 +77,8 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)
                retval += sizeof(u32);
                data += sizeof(u32);
                max -= sizeof(u32);
+
+               sr = readl_relaxed(priv->base + RNG_SR);
        }

        if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),


Regards,
Maxime

[PATCH] hwrng: stm32 - fix build warning

[Arnd Bergmann]

On Tuesday, May 24, 2016 9:59:41 AM CEST Maxime Coquelin wrote:
> 2016-05-23 22:35 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> > On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
> >> diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
> >> index 92a8106..0533370 100644
> >> --- a/drivers/char/hw_random/stm32-rng.c
> >> +++ b/drivers/char/hw_random/stm32-rng.c
> >> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> >>  {
> >>       struct stm32_rng_private *priv =
> >>           container_of(rng, struct stm32_rng_private, rng);
> >> -     u32 sr;
> >> +     u32 sr = 0;
> >>       int retval = 0;
> >>
> >>       pm_runtime_get_sync((struct device *) priv->rng.priv);
> >
> > Does this work as well?
> >
> > diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
> > index 92a810648bd0..5c836b0afa40 100644
> > --- a/drivers/char/hw_random/stm32-rng.c
> > +++ b/drivers/char/hw_random/stm32-rng.c
> > @@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> >                 max -= sizeof(u32);
> >         }
> >
> > -       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> > +       if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
> >                       "bad RNG status - %x\n", sr))
> >                 writel_relaxed(0, priv->base + RNG_SR);
> >
> > I think it would be nicer to not add a bogus initialization.
> Hmm, no sure this nicer.
> The while loop can break before retval is incremented when sr value is
> not expected (sr != RNG_SR_DRDY).
> In that case, we certainly want to print sr value.

Ah, you are right.

> Maybe the better way is just to initialize sr with status register content?

>        pm_runtime_get_sync((struct device *) priv->rng.priv);
>
>+       sr = readl_relaxed(priv->base + RNG_SR);
>        while (max > sizeof(u32)) {
>-               sr = readl_relaxed(priv->base + RNG_SR);
>                if (!sr && wait) {
>                        unsigned int timeout = RNG_TIMEOUT;


I think that introduces a bug: you really want to read the status
register on each loop iteration.

How about moving the error handling into the loop itself?

	Arnd


diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..fceacd809462 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -59,6 +59,10 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 
 	while (max > sizeof(u32)) {
 		sr = readl_relaxed(priv->base + RNG_SR);
+		if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
+			      "bad RNG status - %x\n", sr))
+			writel_relaxed(0, priv->base + RNG_SR);
+
 		if (!sr && wait) {
 			unsigned int timeout = RNG_TIMEOUT;
 
@@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 		max -= sizeof(u32);
 	}
 
-	if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
-		      "bad RNG status - %x\n", sr))
-		writel_relaxed(0, priv->base + RNG_SR);
-
 	pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
 	pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
 

[PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 10:32 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Tuesday, May 24, 2016 9:59:41 AM CEST Maxime Coquelin wrote:
>> 2016-05-23 22:35 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> > On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>> >> diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
>> >> index 92a8106..0533370 100644
>> >> --- a/drivers/char/hw_random/stm32-rng.c
>> >> +++ b/drivers/char/hw_random/stm32-rng.c
>> >> @@ -52,7 +52,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>> >>  {
>> >>       struct stm32_rng_private *priv =
>> >>           container_of(rng, struct stm32_rng_private, rng);
>> >> -     u32 sr;
>> >> +     u32 sr = 0;
>> >>       int retval = 0;
>> >>
>> >>       pm_runtime_get_sync((struct device *) priv->rng.priv);
>> >
>> > Does this work as well?
>> >
>> > diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
>> > index 92a810648bd0..5c836b0afa40 100644
>> > --- a/drivers/char/hw_random/stm32-rng.c
>> > +++ b/drivers/char/hw_random/stm32-rng.c
>> > @@ -79,7 +79,7 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>> >                 max -= sizeof(u32);
>> >         }
>> >
>> > -       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> > +       if (WARN_ONCE(retval > 0 && (sr & (RNG_SR_SEIS | RNG_SR_CEIS)),
>> >                       "bad RNG status - %x\n", sr))
>> >                 writel_relaxed(0, priv->base + RNG_SR);
>> >
>> > I think it would be nicer to not add a bogus initialization.
>> Hmm, no sure this nicer.
>> The while loop can break before retval is incremented when sr value is
>> not expected (sr != RNG_SR_DRDY).
>> In that case, we certainly want to print sr value.
>
> Ah, you are right.
>
>> Maybe the better way is just to initialize sr with status register content?
>
>>        pm_runtime_get_sync((struct device *) priv->rng.priv);
>>
>>+       sr = readl_relaxed(priv->base + RNG_SR);
>>        while (max > sizeof(u32)) {
>>-               sr = readl_relaxed(priv->base + RNG_SR);
>>                if (!sr && wait) {
>>                        unsigned int timeout = RNG_TIMEOUT;
>
>
> I think that introduces a bug: you really want to read the status
> register on each loop iteration.
Actually, I read the status again at the end of the loop.
But my implementation isn't good anyway, because I read the status
register one time more every time.

>
> How about moving the error handling into the loop itself?
That would be better, indeed, but there is one problem with your below proposal:
>
>         Arnd
>
>
> diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..fceacd809462 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -59,6 +59,10 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>
>         while (max > sizeof(u32)) {
>                 sr = readl_relaxed(priv->base + RNG_SR);
> +               if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> +                             "bad RNG status - %x\n", sr))
> +                       writel_relaxed(0, priv->base + RNG_SR);
> +
The error handling should be moved after the last status register read.

>                 if (!sr && wait) {
>                         unsigned int timeout = RNG_TIMEOUT;
>
> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>                 max -= sizeof(u32);
>         }
>
> -       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> -                     "bad RNG status - %x\n", sr))
> -               writel_relaxed(0, priv->base + RNG_SR);
> -
>         pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
>         pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);


diff --git a/drivers/char/hw_random/stm32-rng.c
b/drivers/char/hw_random/stm32-rng.c
index 92a810648bd0..2a0fc90e4dc3 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)
                        } while (!sr && --timeout);
                }

+               if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
+                               "bad RNG status - %x\n", sr))
+                       writel_relaxed(0, priv->base + RNG_SR);
+
                /* If error detected or data not ready... */
                if (sr != RNG_SR_DRDY)
                        break;
@@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)
                max -= sizeof(u32);
        }

-       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
-                     "bad RNG status - %x\n", sr))
-               writel_relaxed(0, priv->base + RNG_SR);
-
        pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
        pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);

Thanks,
Maxime

[PATCH] hwrng: stm32 - fix build warning

[Arnd Bergmann]

On Tuesday, May 24, 2016 10:50:17 AM CEST Maxime Coquelin wrote:
> diff --git a/drivers/char/hw_random/stm32-rng.c
> b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..2a0fc90e4dc3 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
> *data, size_t max, bool wait)
>                         } while (!sr && --timeout);
>                 }
> 
> +               if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> +                               "bad RNG status - %x\n", sr))
> +                       writel_relaxed(0, priv->base + RNG_SR);
> +
>                 /* If error detected or data not ready... */
>                 if (sr != RNG_SR_DRDY)
>                         break;
> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void
> *data, size_t max, bool wait)
>                 max -= sizeof(u32);
>         }
> 
> -       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> -                     "bad RNG status - %x\n", sr))
> -               writel_relaxed(0, priv->base + RNG_SR);
> -
>         pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
>         pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
> 
> Thanks,
> 

Yes, that looks good to me.

	Arnd

[PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 10:58 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
> On Tuesday, May 24, 2016 10:50:17 AM CEST Maxime Coquelin wrote:
>> diff --git a/drivers/char/hw_random/stm32-rng.c
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a810648bd0..2a0fc90e4dc3 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>>                         } while (!sr && --timeout);
>>                 }
>>
>> +               if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> +                               "bad RNG status - %x\n", sr))
>> +                       writel_relaxed(0, priv->base + RNG_SR);
>> +
>>                 /* If error detected or data not ready... */
>>                 if (sr != RNG_SR_DRDY)
>>                         break;
>> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>>                 max -= sizeof(u32);
>>         }
>>
>> -       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> -                     "bad RNG status - %x\n", sr))
>> -               writel_relaxed(0, priv->base + RNG_SR);
>> -
>>         pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
>>         pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
>>
>> Thanks,
>>
>
> Yes, that looks good to me.

Thanks!
Sudip, do you want to send the patch, or I manage to do it?

Maxime

[PATCH] hwrng: stm32 - fix build warning

[Daniel Thompson]

On 24/05/16 09:50, Maxime Coquelin wrote:
> diff --git a/drivers/char/hw_random/stm32-rng.c
> b/drivers/char/hw_random/stm32-rng.c
> index 92a810648bd0..2a0fc90e4dc3 100644
> --- a/drivers/char/hw_random/stm32-rng.c
> +++ b/drivers/char/hw_random/stm32-rng.c
> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
> *data, size_t max, bool wait)
>                         } while (!sr && --timeout);
>                 }
>
> +               if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
> +                               "bad RNG status - %x\n", sr))
> +                       writel_relaxed(0, priv->base + RNG_SR);
> +
>                 /* If error detected or data not ready... */
>                 if (sr != RNG_SR_DRDY)
>                         break;

Minor quibble but I might prefer that the error handling/recovery 
actually be put on the error path itself (included in the if (sr != 
RNG_SR_DRDY) ).


Daniel.

[PATCH] hwrng: stm32 - fix build warning

[Maxime Coquelin]

2016-05-24 12:09 GMT+02:00 Daniel Thompson <daniel.thompson@linaro.org>:
> On 24/05/16 09:50, Maxime Coquelin wrote:
>>
>> diff --git a/drivers/char/hw_random/stm32-rng.c
>> b/drivers/char/hw_random/stm32-rng.c
>> index 92a810648bd0..2a0fc90e4dc3 100644
>> --- a/drivers/char/hw_random/stm32-rng.c
>> +++ b/drivers/char/hw_random/stm32-rng.c
>> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>>                         } while (!sr && --timeout);
>>                 }
>>
>> +               if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>> +                               "bad RNG status - %x\n", sr))
>> +                       writel_relaxed(0, priv->base + RNG_SR);
>> +
>>                 /* If error detected or data not ready... */
>>                 if (sr != RNG_SR_DRDY)
>>                         break;
>
>
> Minor quibble but I might prefer that the error handling/recovery actually
> be put on the error path itself (included in the if (sr != RNG_SR_DRDY) ).
Yes, it would be better.

Regards,
Maxime

[PATCH] hwrng: stm32 - fix build warning

[Sudip Mukherjee]

On Tuesday 24 May 2016 02:50 PM, Maxime Coquelin wrote:
> 2016-05-24 10:58 GMT+02:00 Arnd Bergmann <arnd@arndb.de>:
>> On Tuesday, May 24, 2016 10:50:17 AM CEST Maxime Coquelin wrote:
>>> diff --git a/drivers/char/hw_random/stm32-rng.c
>>> b/drivers/char/hw_random/stm32-rng.c
>>> index 92a810648bd0..2a0fc90e4dc3 100644
>>> --- a/drivers/char/hw_random/stm32-rng.c
>>> +++ b/drivers/char/hw_random/stm32-rng.c
>>> @@ -68,6 +68,10 @@ static int stm32_rng_read(struct hwrng *rng, void
>>> *data, size_t max, bool wait)
>>>                          } while (!sr && --timeout);
>>>                  }
>>>
>>> +               if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>>> +                               "bad RNG status - %x\n", sr))
>>> +                       writel_relaxed(0, priv->base + RNG_SR);
>>> +
>>>                  /* If error detected or data not ready... */
>>>                  if (sr != RNG_SR_DRDY)
>>>                          break;
>>> @@ -79,10 +83,6 @@ static int stm32_rng_read(struct hwrng *rng, void
>>> *data, size_t max, bool wait)
>>>                  max -= sizeof(u32);
>>>          }
>>>
>>> -       if (WARN_ONCE(sr & (RNG_SR_SEIS | RNG_SR_CEIS),
>>> -                     "bad RNG status - %x\n", sr))
>>> -               writel_relaxed(0, priv->base + RNG_SR);
>>> -
>>>          pm_runtime_mark_last_busy((struct device *) priv->rng.priv);
>>>          pm_runtime_put_sync_autosuspend((struct device *) priv->rng.priv);
>>>
>>> Thanks,
>>>
>>
>> Yes, that looks good to me.
>
> Thanks!
> Sudip, do you want to send the patch, or I manage to do it?

Maybe you should send it, i have not done anything in reaching its final 
form.

Regards
Sudip

[PATCH] hwrng: stm32 - fix build warning

[Sudip Mukherjee]

On Tuesday 24 May 2016 02:05 AM, Arnd Bergmann wrote:
> On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>> We have been getting build warning about:
>> drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
>> drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
>> 					uninitialized in this function
>>
>> On checking the code it turns out that sr can never be used
>> uninitialized as sr is getting initialized in the while loop and while
>> loop will always execute as the minimum value of max can be 32.
>> So just initialize sr to 0 while declaring it to silence the compiler.
>>
>> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>> ---
>
> I notice that you are using a really old compiler. While this warning
> seems to be valid in the sense that the compiler should figure out that
> the variable might be used uninitialized, please update your toolchain
> before reporting other such problems, as gcc-4.6 had a lot more false
> positives that newer ones (5.x or 6.x) have.

yes, i need to upgrade gcc in my travis bot. But in my local system I am 
having gcc-4.8.4 and there also I am having this error and i am sure 
4.8.4 is still being used by many people.

Regards
Sudip

[PATCH] hwrng: stm32 - fix build warning

[Arnd Bergmann]

On Wednesday, May 25, 2016 7:35:17 AM CEST Sudip Mukherjee wrote:
> On Tuesday 24 May 2016 02:05 AM, Arnd Bergmann wrote:
> > On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
> >> We have been getting build warning about:
> >> drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
> >> drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
> >>                                      uninitialized in this function
> >>
> >> On checking the code it turns out that sr can never be used
> >> uninitialized as sr is getting initialized in the while loop and while
> >> loop will always execute as the minimum value of max can be 32.
> >> So just initialize sr to 0 while declaring it to silence the compiler.
> >>
> >> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> >> ---
> >
> > I notice that you are using a really old compiler. While this warning
> > seems to be valid in the sense that the compiler should figure out that
> > the variable might be used uninitialized, please update your toolchain
> > before reporting other such problems, as gcc-4.6 had a lot more false
> > positives that newer ones (5.x or 6.x) have.
> 
> yes, i need to upgrade gcc in my travis bot. But in my local system I am 
> having gcc-4.8.4 and there also I am having this error and i am sure 
> 4.8.4 is still being used by many people.

Right, the change from gcc-4.8 to 4.9 is what drastically changed hte
maybe-uninitialized warnings, introducing a number of additional warnings
(many of them correct) but removing many others (mostly false positives).
I tend to care only about the ones in 4.9+ for this reason. I haven't
run statistics on this in a while, but I guess we could consider turning
off this warning for 4.8 and earlier (though IIRC the switch to turn it
off only appeared in 4.9).

BTW, regarding your build infrastructure, I'd also recommend building
with 'make -s' to make the output more compact.

	Arnd

[PATCH] hwrng: stm32 - fix build warning

[Sudip Mukherjee]

On Wednesday 25 May 2016 03:36 PM, Arnd Bergmann wrote:
> On Wednesday, May 25, 2016 7:35:17 AM CEST Sudip Mukherjee wrote:
>> On Tuesday 24 May 2016 02:05 AM, Arnd Bergmann wrote:
>>> On Monday, May 23, 2016 6:14:08 PM CEST Sudip Mukherjee wrote:
>>>> We have been getting build warning about:
>>>> drivers/char/hw_random/stm32-rng.c: In function 'stm32_rng_read':
>>>> drivers/char/hw_random/stm32-rng.c:82:19: warning: 'sr' may be used
>>>>                                       uninitialized in this function
>>>>
>>>> On checking the code it turns out that sr can never be used
>>>> uninitialized as sr is getting initialized in the while loop and while
>>>> loop will always execute as the minimum value of max can be 32.
>>>> So just initialize sr to 0 while declaring it to silence the compiler.
>>>>
>>>> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>>>> ---
>>>
>snip>
>
> BTW, regarding your build infrastructure, I'd also recommend building
> with 'make -s' to make the output more compact.

travis is having a timeout and if there is no output from the build 
within a time limit then it will cancel the build. I can use the option 
if i can increase the timeout limit. I will have a look at the options. 
Thanks for the idea.

Regards
Sudip