* [PATCH] ARM: mxc-rnga: fix data_present API
@ 2012-06-12 21:07 Benoît Thébaudeau
2012-06-12 21:12 ` Fabio Estevam
2012-06-13 6:36 ` Uwe Kleine-König
0 siblings, 2 replies; 6+ messages in thread
From: Benoît Thébaudeau @ 2012-06-12 21:07 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Alan Carvalho de Assis <acassis@gmail.com>
Cc: <linux-arm-kernel@lists.infradead.org>
Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
---
.../drivers/char/hw_random/mxc-rnga.c | 22 +++++++++++++-------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
index 187c6be..2924253 100644
--- linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c
+++ linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
@@ -24,6 +24,7 @@
#include <linux/ioport.h>
#include <linux/platform_device.h>
#include <linux/hw_random.h>
+#include <linux/delay.h>
#include <linux/io.h>
/* RNGA Registers */
@@ -60,16 +61,21 @@
static struct platform_device *rng_dev;
-static int mxc_rnga_data_present(struct hwrng *rng)
+static int mxc_rnga_data_present(struct hwrng *rng, int wait)
{
- int level;
void __iomem *rng_base = (void __iomem *)rng->priv;
-
- /* how many random numbers is in FIFO? [0-16] */
- level = ((__raw_readl(rng_base + RNGA_STATUS) &
- RNGA_STATUS_LEVEL_MASK) >> 8);
-
- return level > 0 ? 1 : 0;
+ int level, data, i;
+
+ for (i = 0; i < 20; i++) {
+ /* how many random numbers is in FIFO? [0-16] */
+ level = ((__raw_readl(rng_base + RNGA_STATUS) &
+ RNGA_STATUS_LEVEL_MASK) >> 8);
+ data = level > 0 ? 1 : 0;
+ if (data || !wait)
+ break;
+ udelay(10);
+ }
+ return data;
}
static int mxc_rnga_data_read(struct hwrng *rng, u32 * data)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] ARM: mxc-rnga: fix data_present API
2012-06-12 21:07 [PATCH] ARM: mxc-rnga: fix data_present API Benoît Thébaudeau
@ 2012-06-12 21:12 ` Fabio Estevam
2012-06-12 21:25 ` Benoît Thébaudeau
2012-06-13 6:36 ` Uwe Kleine-König
1 sibling, 1 reply; 6+ messages in thread
From: Fabio Estevam @ 2012-06-12 21:12 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 12, 2012 at 6:07 PM, Beno?t Th?baudeau
<benoit.thebaudeau@advansee.com> wrote:
> +
> + ? ? ? for (i = 0; i < 20; i++) {
Where does this magic 20 comes from?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: mxc-rnga: fix data_present API
2012-06-12 21:12 ` Fabio Estevam
@ 2012-06-12 21:25 ` Benoît Thébaudeau
0 siblings, 0 replies; 6+ messages in thread
From: Benoît Thébaudeau @ 2012-06-12 21:25 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 12, 2012 at 11:12 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Jun 12, 2012 at 6:07 PM, Beno?t Th?baudeau
> <benoit.thebaudeau@advansee.com> wrote:
> > +
> > + ? ? ? for (i = 0; i < 20; i++) {
>
> Where does this magic 20 comes from?
>From the other hw_random drivers. Most use this value, and it works fine on i.MX
too.
Regards,
Beno?t
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: mxc-rnga: fix data_present API
2012-06-12 21:07 [PATCH] ARM: mxc-rnga: fix data_present API Benoît Thébaudeau
2012-06-12 21:12 ` Fabio Estevam
@ 2012-06-13 6:36 ` Uwe Kleine-König
2012-06-13 16:15 ` Benoît Thébaudeau
1 sibling, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2012-06-13 6:36 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
your changelog is very sparse. Maybe point out the commit that changed
the API without fixing its users?
On Tue, Jun 12, 2012 at 11:07:47PM +0200, Beno?t Th?baudeau wrote:
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Alan Carvalho de Assis <acassis@gmail.com>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> ---
> .../drivers/char/hw_random/mxc-rnga.c | 22 +++++++++++++-------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
> index 187c6be..2924253 100644
> --- linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c
> +++ linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
> @@ -24,6 +24,7 @@
> #include <linux/ioport.h>
> #include <linux/platform_device.h>
> #include <linux/hw_random.h>
> +#include <linux/delay.h>
> #include <linux/io.h>
>
> /* RNGA Registers */
> @@ -60,16 +61,21 @@
>
> static struct platform_device *rng_dev;
>
> -static int mxc_rnga_data_present(struct hwrng *rng)
> +static int mxc_rnga_data_present(struct hwrng *rng, int wait)
> {
> - int level;
> void __iomem *rng_base = (void __iomem *)rng->priv;
> -
> - /* how many random numbers is in FIFO? [0-16] */
> - level = ((__raw_readl(rng_base + RNGA_STATUS) &
> - RNGA_STATUS_LEVEL_MASK) >> 8);
> -
> - return level > 0 ? 1 : 0;
> + int level, data, i;
level is only used in the for loop, so it should be declared there, too.
> +
> + for (i = 0; i < 20; i++) {
> + /* how many random numbers is in FIFO? [0-16] */
As you touch this comment can you fix the grammar, too? (i.e. s/is/are/)
> + level = ((__raw_readl(rng_base + RNGA_STATUS) &
> + RNGA_STATUS_LEVEL_MASK) >> 8);
> + data = level > 0 ? 1 : 0;
This statement is equivalent to:
data = level > 0;
so IMHO there is no need for the data variable.
> + if (data || !wait)
> + break;
> + udelay(10);
Apart from the magic 20 that Fabio already pointed out, these 10 us are
magic, too. What is the requirement when wait evaluates to true? Are you
allowed to sleep? If so, maybe better do that?
> + }
> + return data;
> }
>
> static int mxc_rnga_data_read(struct hwrng *rng, u32 * data)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: mxc-rnga: fix data_present API
2012-06-13 6:36 ` Uwe Kleine-König
@ 2012-06-13 16:15 ` Benoît Thébaudeau
2012-06-27 6:48 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Benoît Thébaudeau @ 2012-06-13 16:15 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Wed, Jun 13, 2012 at 8:36:50AM +0200, Uwe Kleine-K?nig wrote:
> your changelog is very sparse. Maybe point out the commit that
> changed
> the API without fixing its users?
Done below.
> On Tue, Jun 12, 2012 at 11:07:47PM +0200, Beno?t Th?baudeau wrote:
> > + int level, data, i;
> level is only used in the for loop, so it should be declared there,
> too.
Done below.
> > +
> > + for (i = 0; i < 20; i++) {
> > + /* how many random numbers is in FIFO? [0-16] */
> As you touch this comment can you fix the grammar, too? (i.e.
> s/is/are/)
Done below.
> > + level = ((__raw_readl(rng_base + RNGA_STATUS) &
> > + RNGA_STATUS_LEVEL_MASK) >> 8);
> > + data = level > 0 ? 1 : 0;
> This statement is equivalent to:
>
> data = level > 0;
>
> so IMHO there is no need for the data variable.
Done below. I had kept that only to stick more closely to the original code, but
I did not like this either.
> > + if (data || !wait)
> > + break;
> > + udelay(10);
> Apart from the magic 20 that Fabio already pointed out, these 10 us
> are
> magic, too. What is the requirement when wait evaluates to true? Are
> you
> allowed to sleep? If so, maybe better do that?
Both the magic 20 and the magic 10 ?s are simply the missing extension for RNGA
of commit 984e976. They first appear in commit 844dd05, which only says this is
polling time used to let RNG HW gather new entropy while rng_dev_read() is
waiting for it. Hence, at least the 10 ?s should be hardware-specific.
The requirement when wait evaluates to true should be to wait for entropy data
with a time-out. rng_dev_read() seems to run in a "sleep-able" context. The
issue is that there does not seem to be any source of information regarding the
time required by RNGA to gather entropy data, or the time after which it is
useless to expect entropy data. The RNGA chapter of the i.MX31 RM is very light.
IMHO, considering the information we have, it's better to keep this 20 * 10 ?s
behavior copied from the other RNG drivers since it works, it gives some time to
RNGA to gather new entropy, and at worst it wastes 0.2 ms each time. Should it
sleep for 10 ?s?
See my reworked patch below.
Regards,
Beno?t
[PATCH] ARM: mxc-rnga: fix data_present API
Commit 45001e9, which added support for RNGA, ignored the previous commit
984e976, which changed the data_present API.
Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Alan Carvalho de Assis <acassis@gmail.com>
Cc: <linux-arm-kernel@lists.infradead.org>
Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
---
.../drivers/char/hw_random/mxc-rnga.c | 21 ++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git linux-next-HEAD-aab6028.orig/drivers/char/hw_random/mxc-rnga.c linux-next-HEAD-aab6028/drivers/char/hw_random/mxc-rnga.c
index 187c6be..85074de 100644
--- linux-next-HEAD-aab6028.orig/drivers/char/hw_random/mxc-rnga.c
+++ linux-next-HEAD-aab6028/drivers/char/hw_random/mxc-rnga.c
@@ -24,6 +24,7 @@
#include <linux/ioport.h>
#include <linux/platform_device.h>
#include <linux/hw_random.h>
+#include <linux/delay.h>
#include <linux/io.h>
/* RNGA Registers */
@@ -60,16 +61,20 @@
static struct platform_device *rng_dev;
-static int mxc_rnga_data_present(struct hwrng *rng)
+static int mxc_rnga_data_present(struct hwrng *rng, int wait)
{
- int level;
void __iomem *rng_base = (void __iomem *)rng->priv;
-
- /* how many random numbers is in FIFO? [0-16] */
- level = ((__raw_readl(rng_base + RNGA_STATUS) &
- RNGA_STATUS_LEVEL_MASK) >> 8);
-
- return level > 0 ? 1 : 0;
+ int i;
+
+ for (i = 0; i < 20; i++) {
+ /* how many random numbers are in FIFO? [0-16] */
+ int level = (__raw_readl(rng_base + RNGA_STATUS) &
+ RNGA_STATUS_LEVEL_MASK) >> 8;
+ if (level || !wait)
+ return !!level;
+ udelay(10);
+ }
+ return 0;
}
static int mxc_rnga_data_read(struct hwrng *rng, u32 * data)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] ARM: mxc-rnga: fix data_present API
2012-06-13 16:15 ` Benoît Thébaudeau
@ 2012-06-27 6:48 ` Herbert Xu
0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2012-06-27 6:48 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 13, 2012 at 06:15:34PM +0200, Beno?t Th?baudeau wrote:
>
> [PATCH] ARM: mxc-rnga: fix data_present API
>
> Commit 45001e9, which added support for RNGA, ignored the previous commit
> 984e976, which changed the data_present API.
>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Alan Carvalho de Assis <acassis@gmail.com>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
Patch applied.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-27 6:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-12 21:07 [PATCH] ARM: mxc-rnga: fix data_present API Benoît Thébaudeau
2012-06-12 21:12 ` Fabio Estevam
2012-06-12 21:25 ` Benoît Thébaudeau
2012-06-13 6:36 ` Uwe Kleine-König
2012-06-13 16:15 ` Benoît Thébaudeau
2012-06-27 6:48 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).