From patchwork Wed Jun 13 16:15:34 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: ARM: mxc-rnga: fix data_present API Date: Wed, 13 Jun 2012 06:15:34 -0000 From: =?utf-8?q?Beno=C3=AEt_Th=C3=A9baudeau?= X-Patchwork-Id: 164712 Message-Id: <449445897.2590473.1339604134205.JavaMail.root@advansee.com> To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Cc: linux-arm-kernel@lists.infradead.org, Alan Carvalho de Assis , Herbert Xu , Sascha Hauer , Matt Mackall 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 Cc: Herbert Xu Cc: Sascha Hauer Cc: Alan Carvalho de Assis Cc: Signed-off-by: Benoît Thébaudeau --- .../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 #include #include +#include #include /* 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)