From patchwork Wed Jun 13 16:15:34 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Beno=C3=AEt_Th=C3=A9baudeau?= X-Patchwork-Id: 164712 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 25E0AB6FE0 for ; Thu, 14 Jun 2012 02:15:42 +1000 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SeqAV-0003PT-Op; Wed, 13 Jun 2012 16:11:41 +0000 Received: from casper.infradead.org ([2001:770:15f::2]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1SeqAT-0003OO-7i for linux-arm-kernel@merlin.infradead.org; Wed, 13 Jun 2012 16:11:37 +0000 Received: from zose-mta15.web4all.fr ([176.31.217.11]) by casper.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SeqAI-0008Sh-GD for linux-arm-kernel@lists.infradead.org; Wed, 13 Jun 2012 16:11:35 +0000 Received: from localhost (localhost [127.0.0.1]) by zose-mta15.web4all.fr (Postfix) with ESMTP id 55BA42C2E0; Wed, 13 Jun 2012 18:13:10 +0200 (CEST) X-Virus-Scanned: amavisd-new at zose1.web4all.fr Received: from zose-mta15.web4all.fr ([127.0.0.1]) by localhost (zose-mta15.web4all.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JufUxI4kGTWV; Wed, 13 Jun 2012 18:13:09 +0200 (CEST) Received: from zose-store12.web4all.fr (zose-store-12.w4a.fr [178.33.204.48]) by zose-mta15.web4all.fr (Postfix) with ESMTP id 370892C18E; Wed, 13 Jun 2012 18:13:09 +0200 (CEST) Date: Wed, 13 Jun 2012 18:15:34 +0200 (CEST) From: =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?= To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Message-ID: <449445897.2590473.1339604134205.JavaMail.root@advansee.com> In-Reply-To: <20120613063650.GB32436@pengutronix.de> Subject: Re: [PATCH] ARM: mxc-rnga: fix data_present API MIME-Version: 1.0 X-Originating-IP: [88.188.188.98] X-Mailer: Zimbra 7.2.0_GA_2669 (ZimbraWebClient - FF3.0 (Win)/7.2.0_GA_2669) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20120613_171126_689346_0E2E633B X-CRM114-Status: GOOD ( 26.80 ) X-Spam-Score: -1.9 (-) X-Spam-Report: SpamAssassin version 3.3.2 on casper.infradead.org summary: Content analysis details: (-1.9 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: linux-arm-kernel@lists.infradead.org, Alan Carvalho de Assis , Herbert Xu , Sascha Hauer , Matt Mackall X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org 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)