Data valid gets cleared by reading the ISR (status register) and NOT from
reading ODATA (data register). A new data word can become available between
checking ISR and reading ODATA, causing us to reuse the same data word next
time atmel_trng_read() gets called, if that happens before the following
data word is ready.
With this fixed, rngtest no longer complains of 'Continous run' errors.
Before:
rngtest -c 1000 < /dev/hwrng
rngtest 3
Copyright (c) 2004 by Henrique de Moraes Holschuh
This is free software; see the source for copying conditions. There is NO warr.
rngtest: starting FIPS tests...
rngtest: bits received from input: 20000032
rngtest: FIPS 140-2 successes: 923
rngtest: FIPS 140-2 failures: 77
rngtest: FIPS 140-2(2001-10-10) Monobit: 0
rngtest: FIPS 140-2(2001-10-10) Poker: 0
rngtest: FIPS 140-2(2001-10-10) Runs: 1
rngtest: FIPS 140-2(2001-10-10) Long run: 0
rngtest: FIPS 140-2(2001-10-10) Continuous run: 76
rngtest: input channel speed: (min=721.402; avg=46003.510; max=49321.338)Kibitss
rngtest: FIPS tests speed: (min=11.442; avg=12.714; max=12.801)Mibits/s
rngtest: Program run time: 1931860 microseconds
After:
rngtest -c 1000 < /dev/hwrng
rngtest 3
Copyright (c) 2004 by Henrique de Moraes Holschuh
This is free software; see the source for copying conditions. There is NO warr.
rngtest: starting FIPS tests...
rngtest: bits received from input: 20000032
rngtest: FIPS 140-2 successes: 1000
rngtest: FIPS 140-2 failures: 0
rngtest: FIPS 140-2(2001-10-10) Monobit: 0
rngtest: FIPS 140-2(2001-10-10) Poker: 0
rngtest: FIPS 140-2(2001-10-10) Runs: 0
rngtest: FIPS 140-2(2001-10-10) Long run: 0
rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
rngtest: input channel speed: (min=777.518; avg=36988.482; max=43115.342)Kibitss
rngtest: FIPS tests speed: (min=11.951; avg=12.715; max=12.887)Mibits/s
rngtest: Program run time: 2035543 microseconds
Cc: [email protected]
Signed-off-by: Peter Korsgaard <[email protected]>
Reported-by: George Pontis <[email protected]>
---
drivers/char/hw_random/atmel-rng.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/char/hw_random/atmel-rng.c b/drivers/char/hw_random/atmel-rng.c
index d7ab920..731c904 100644
--- a/drivers/char/hw_random/atmel-rng.c
+++ b/drivers/char/hw_random/atmel-rng.c
@@ -36,6 +36,13 @@ static int atmel_trng_read(struct hwrng *rng, void *buf, size_t max,
/* data ready? */
if (readl(trng->base + TRNG_ISR) & 1) {
*data = readl(trng->base + TRNG_ODATA);
+ /*
+ ensure data ready is only set again AFTER the next data
+ word is ready in case it got set between checking ISR
+ and reading ODATA, so we don't risk re-reading the
+ same word
+ */
+ readl(trng->base + TRNG_ISR);
return 4;
} else
return 0;
--
1.7.10
On 05/25/2012 11:12 AM, Peter Korsgaard :
> Data valid gets cleared by reading the ISR (status register) and NOT from
> reading ODATA (data register). A new data word can become available between
> checking ISR and reading ODATA, causing us to reuse the same data word next
> time atmel_trng_read() gets called, if that happens before the following
> data word is ready.
Hi Peter, thanks for finding this...
> With this fixed, rngtest no longer complains of 'Continous run' errors.
> Before:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 923
> rngtest: FIPS 140-2 failures: 77
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 1
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 76
> rngtest: input channel speed: (min=721.402; avg=46003.510; max=49321.338)Kibitss
> rngtest: FIPS tests speed: (min=11.442; avg=12.714; max=12.801)Mibits/s
> rngtest: Program run time: 1931860 microseconds
>
> After:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 1000
> rngtest: FIPS 140-2 failures: 0
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 0
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> rngtest: input channel speed: (min=777.518; avg=36988.482; max=43115.342)Kibitss
> rngtest: FIPS tests speed: (min=11.951; avg=12.715; max=12.887)Mibits/s
> rngtest: Program run time: 2035543 microseconds
>
> Cc: [email protected]
> Signed-off-by: Peter Korsgaard <[email protected]>
> Reported-by: George Pontis <[email protected]>
> ---
> drivers/char/hw_random/atmel-rng.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/char/hw_random/atmel-rng.c b/drivers/char/hw_random/atmel-rng.c
> index d7ab920..731c904 100644
> --- a/drivers/char/hw_random/atmel-rng.c
> +++ b/drivers/char/hw_random/atmel-rng.c
> @@ -36,6 +36,13 @@ static int atmel_trng_read(struct hwrng *rng, void *buf, size_t max,
> /* data ready? */
> if (readl(trng->base + TRNG_ISR) & 1) {
> *data = readl(trng->base + TRNG_ODATA);
> + /*
> + ensure data ready is only set again AFTER the next data
> + word is ready in case it got set between checking ISR
> + and reading ODATA, so we don't risk re-reading the
> + same word
> + */
> + readl(trng->base + TRNG_ISR);
> return 4;
> } else
> return 0;
What about a single read to ISR like this:
tmp = readl(trng->base + TRNG_ODATA);
if (readl(trng->base + TRNG_ISR) & 1) {
*data = tmp;
return 4;
} else {
return 0;
}
But it is true that there is always 2 reads in case of data not
available, instead of just one: I cannot figure out which solution is
the fastest: your thoughts?
Bye,
--
Nicolas Ferre
>>>>> "Nicolas" == Nicolas Ferre <[email protected]> writes:
Hi,
Nicolas> What about a single read to ISR like this:
Nicolas> tmp = readl(trng->base + TRNG_ODATA);
Nicolas> if (readl(trng->base + TRNG_ISR) & 1) {
Nicolas> *data = tmp;
Nicolas> return 4;
Nicolas> } else {
Nicolas> return 0;
Nicolas> }
No, that won't work as you then have another race. Data might not be
ready when you read ODATA, but then become ready just in time for when
you read ISR, so you end up using stale data.
It all would have been easier if the ready bit would get cleared on
reads from ODATA instead of/as well as from ISR, but that's
unfortunately not the case.
--
Bye, Peter Korsgaard
On 05/25/2012 12:10 PM, Peter Korsgaard :
>>>>>> "Nicolas" == Nicolas Ferre <[email protected]> writes:
>
> Hi,
>
> Nicolas> What about a single read to ISR like this:
>
> Nicolas> tmp = readl(trng->base + TRNG_ODATA);
> Nicolas> if (readl(trng->base + TRNG_ISR) & 1) {
> Nicolas> *data = tmp;
> Nicolas> return 4;
> Nicolas> } else {
> Nicolas> return 0;
> Nicolas> }
>
> No, that won't work as you then have another race. Data might not be
> ready when you read ODATA, but then become ready just in time for when
> you read ISR, so you end up using stale data.
Yes, sure.
> It all would have been easier if the ready bit would get cleared on
> reads from ODATA instead of/as well as from ISR, but that's
> unfortunately not the case.
I will talk about that idea to my friends designers and see if we can
improve things, in the future...
Thanks, bye,
--
Nicolas Ferre
On 05/25/2012 11:12 AM, Peter Korsgaard :
> Data valid gets cleared by reading the ISR (status register) and NOT from
> reading ODATA (data register). A new data word can become available between
> checking ISR and reading ODATA, causing us to reuse the same data word next
> time atmel_trng_read() gets called, if that happens before the following
> data word is ready.
>
> With this fixed, rngtest no longer complains of 'Continous run' errors.
> Before:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 923
> rngtest: FIPS 140-2 failures: 77
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 1
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 76
> rngtest: input channel speed: (min=721.402; avg=46003.510; max=49321.338)Kibitss
> rngtest: FIPS tests speed: (min=11.442; avg=12.714; max=12.801)Mibits/s
> rngtest: Program run time: 1931860 microseconds
>
> After:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 1000
> rngtest: FIPS 140-2 failures: 0
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 0
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> rngtest: input channel speed: (min=777.518; avg=36988.482; max=43115.342)Kibitss
> rngtest: FIPS tests speed: (min=11.951; avg=12.715; max=12.887)Mibits/s
> rngtest: Program run time: 2035543 microseconds
>
> Cc: [email protected]
> Signed-off-by: Peter Korsgaard <[email protected]>
> Reported-by: George Pontis <[email protected]>
Acked-by: Nicolas Ferre <[email protected]>
Thanks!
> ---
> drivers/char/hw_random/atmel-rng.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/char/hw_random/atmel-rng.c b/drivers/char/hw_random/atmel-rng.c
> index d7ab920..731c904 100644
> --- a/drivers/char/hw_random/atmel-rng.c
> +++ b/drivers/char/hw_random/atmel-rng.c
> @@ -36,6 +36,13 @@ static int atmel_trng_read(struct hwrng *rng, void *buf, size_t max,
> /* data ready? */
> if (readl(trng->base + TRNG_ISR) & 1) {
> *data = readl(trng->base + TRNG_ODATA);
> + /*
> + ensure data ready is only set again AFTER the next data
> + word is ready in case it got set between checking ISR
> + and reading ODATA, so we don't risk re-reading the
> + same word
> + */
> + readl(trng->base + TRNG_ISR);
> return 4;
> } else
> return 0;
--
Nicolas Ferre
On Fri, May 25, 2012 at 11:12:38AM +0200, Peter Korsgaard wrote:
> Data valid gets cleared by reading the ISR (status register) and NOT from
> reading ODATA (data register). A new data word can become available between
> checking ISR and reading ODATA, causing us to reuse the same data word next
> time atmel_trng_read() gets called, if that happens before the following
> data word is ready.
Patch applied crypto. Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt