Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1405526 > unrolled thread

[PATCH] hwrng: stm32 - fix build warning

Started bySudip Mukherjee <sudipm.mukherjee@gmail.com>
First post2016-05-23 19:20 +0200
Last post2016-05-27 15:40 +0200
Articles 13 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] hwrng: stm32 - fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-05-23 19:20 +0200
    Re: [PATCH] hwrng: stm32 - fix build warning Arnd Bergmann <arnd@arndb.de> - 2016-05-23 22:40 +0200
      Re: [PATCH] hwrng: stm32 - fix build warning Maxime Coquelin <mcoquelin.stm32@gmail.com> - 2016-05-24 10:00 +0200
        Re: [PATCH] hwrng: stm32 - fix build warning Arnd Bergmann <arnd@arndb.de> - 2016-05-24 10:40 +0200
          Re: [PATCH] hwrng: stm32 - fix build warning Maxime Coquelin <mcoquelin.stm32@gmail.com> - 2016-05-24 11:00 +0200
            Re: [PATCH] hwrng: stm32 - fix build warning Arnd Bergmann <arnd@arndb.de> - 2016-05-24 11:00 +0200
              Re: [PATCH] hwrng: stm32 - fix build warning Maxime Coquelin <mcoquelin.stm32@gmail.com> - 2016-05-24 11:30 +0200
                Re: [PATCH] hwrng: stm32 - fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-05-25 08:40 +0200
            Re: [PATCH] hwrng: stm32 - fix build warning Daniel Thompson <daniel.thompson@linaro.org> - 2016-05-24 12:10 +0200
              Re: [PATCH] hwrng: stm32 - fix build warning Maxime Coquelin <mcoquelin.stm32@gmail.com> - 2016-05-24 13:00 +0200
      Re: [PATCH] hwrng: stm32 - fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-05-25 08:40 +0200
        Re: [PATCH] hwrng: stm32 - fix build warning Arnd Bergmann <arnd@arndb.de> - 2016-05-25 12:10 +0200
          Re: [PATCH] hwrng: stm32 - fix build warning Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-05-27 15:40 +0200

#1405526 — [PATCH] hwrng: stm32 - fix build warning

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-05-23 19:20 +0200
Subject[PATCH] hwrng: stm32 - fix build warning
Message-ID<rC1Gi-gw-13@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1405615

FromArnd Bergmann <arnd@arndb.de>
Date2016-05-23 22:40 +0200
Message-ID<rC4NQ-2ay-27@gated-at.bofh.it>
In reply to#1405526
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

[toc] | [prev] | [next] | [standalone]

#1405920

FromMaxime Coquelin <mcoquelin.stm32@gmail.com>
Date2016-05-24 10:00 +0200
Message-ID<rCfpV-sX-35@gated-at.bofh.it>
In reply to#1405615
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

[toc] | [prev] | [next] | [standalone]

#1405944

FromArnd Bergmann <arnd@arndb.de>
Date2016-05-24 10:40 +0200
Message-ID<rCg2C-Ve-9@gated-at.bofh.it>
In reply to#1405920
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);

[toc] | [prev] | [next] | [standalone]

#1405955

FromMaxime Coquelin <mcoquelin.stm32@gmail.com>
Date2016-05-24 11:00 +0200
Message-ID<rCgm3-12f-29@gated-at.bofh.it>
In reply to#1405944
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

[toc] | [prev] | [next] | [standalone]

#1405957

FromArnd Bergmann <arnd@arndb.de>
Date2016-05-24 11:00 +0200
Message-ID<rCgm3-12f-37@gated-at.bofh.it>
In reply to#1405955
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

[toc] | [prev] | [next] | [standalone]

#1405985

FromMaxime Coquelin <mcoquelin.stm32@gmail.com>
Date2016-05-24 11:30 +0200
Message-ID<rCgOZ-1re-5@gated-at.bofh.it>
In reply to#1405957
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

[toc] | [prev] | [next] | [standalone]

#1406679

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-05-25 08:40 +0200
Message-ID<rCAE2-646-13@gated-at.bofh.it>
In reply to#1405985
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

[toc] | [prev] | [next] | [standalone]

#1406023

FromDaniel Thompson <daniel.thompson@linaro.org>
Date2016-05-24 12:10 +0200
Message-ID<rChrI-1Tg-9@gated-at.bofh.it>
In reply to#1405955
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.

[toc] | [prev] | [next] | [standalone]

#1406058

FromMaxime Coquelin <mcoquelin.stm32@gmail.com>
Date2016-05-24 13:00 +0200
Message-ID<rCie6-2bW-25@gated-at.bofh.it>
In reply to#1406023
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

[toc] | [prev] | [next] | [standalone]

#1406677

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-05-25 08:40 +0200
Message-ID<rCAE2-646-19@gated-at.bofh.it>
In reply to#1405615
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

[toc] | [prev] | [next] | [standalone]

#1406804

FromArnd Bergmann <arnd@arndb.de>
Date2016-05-25 12:10 +0200
Message-ID<rCDVf-89T-5@gated-at.bofh.it>
In reply to#1406677
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

[toc] | [prev] | [next] | [standalone]

#1408081

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2016-05-27 15:40 +0200
Message-ID<rDq9C-41V-77@gated-at.bofh.it>
In reply to#1406804
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web