From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932257AbbBZUQL (ORCPT ); Thu, 26 Feb 2015 15:16:11 -0500 Received: from mout.kundenserver.de ([212.227.126.131]:65141 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753689AbbBZUQI (ORCPT ); Thu, 26 Feb 2015 15:16:08 -0500 From: Arnd Bergmann To: Scott Branden Cc: bcm-kernel-feedback-list@broadcom.com, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Matt Mackall , Herbert Xu , Grant Likely , Ray Jui , Jonathan Richardson , Dmitry Torokhov , Anatol Pomazao , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver Date: Thu, 26 Feb 2015 21:15:49 +0100 Message-ID: <4488852.TuVxKbAYde@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <54EF75F0.7000901@broadcom.com> References: <1424888184-22180-3-git-send-email-sbranden@broadcom.com> <3062825.7b1oYP4yuL@wuerfel> <54EF75F0.7000901@broadcom.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:Myb6VrAxtGNN8YflUBy6A2pxZimibKZWeh6wPookltKkum1voQX EJm+Fa4WdmsvRTkvDV5xvPg+tI+vIuIC8M+4/UfkYHPpjP3b1Cs2v7nvmsr9gWhaFuJnJSA tmN7rp3dap+WjNLG72hROAb+1jnL8t9IApCjklKPArfZsPGw3PpQ1G+0s0zSs2/IURO7jGJ /A42cmpTah9HCjMkr4G6Q== X-UI-Out-Filterresults: notjunk:1; Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday 26 February 2015 11:37:20 Scott Branden wrote: > Response inline. > > On 15-02-25 11:17 AM, Arnd Bergmann wrote: > > On Wednesday 25 February 2015 10:16:24 Scott Branden wrote: > >> This adds a driver for random number generator present on Broadcom > >> IPROC devices. > >> > >> Reviewed-by: Ray Jui > >> Signed-off-by: Scott Branden > > > > The driver looks reasonable overall, I have just one question about > > something that sticks out: > > > >> + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) { > > ... > >> + > >> + /* Are there any random numbers available? */ > >> + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) & > >> + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) { > > ... > >> + } else { > >> + if (!wait) > >> + /* Cannot wait, return immediately */ > >> + return max - num_remaining; > >> + > >> + /* Can wait, give others chance to run */ > >> + cpu_relax(); > >> + } > >> + } > >> + > > > > It looks like you do a busy-loop around cpu_relax here if asked to wait. > > Is this intentional? I would normally expect either cond_resched() or > > some msleep() instead. > > This code was following examples of other open source drivers - bcm2835 > and exynos both use cpu_relax. I'll have to look into this more to > understand. > The majority of the driver apparently use udelay(10) to wait, which is not much better but at least consistent. The cpu_relax() call probably gives better throughput. I don't understand why none of the drivers actually attempts to msleep(), but that may be because the delay is much too long. Can you find out what the expected latency is for new data to become available on your hardware? Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Thu, 26 Feb 2015 21:15:49 +0100 Subject: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver In-Reply-To: <54EF75F0.7000901@broadcom.com> References: <1424888184-22180-3-git-send-email-sbranden@broadcom.com> <3062825.7b1oYP4yuL@wuerfel> <54EF75F0.7000901@broadcom.com> Message-ID: <4488852.TuVxKbAYde@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thursday 26 February 2015 11:37:20 Scott Branden wrote: > Response inline. > > On 15-02-25 11:17 AM, Arnd Bergmann wrote: > > On Wednesday 25 February 2015 10:16:24 Scott Branden wrote: > >> This adds a driver for random number generator present on Broadcom > >> IPROC devices. > >> > >> Reviewed-by: Ray Jui > >> Signed-off-by: Scott Branden > > > > The driver looks reasonable overall, I have just one question about > > something that sticks out: > > > >> + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) { > > ... > >> + > >> + /* Are there any random numbers available? */ > >> + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) & > >> + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) { > > ... > >> + } else { > >> + if (!wait) > >> + /* Cannot wait, return immediately */ > >> + return max - num_remaining; > >> + > >> + /* Can wait, give others chance to run */ > >> + cpu_relax(); > >> + } > >> + } > >> + > > > > It looks like you do a busy-loop around cpu_relax here if asked to wait. > > Is this intentional? I would normally expect either cond_resched() or > > some msleep() instead. > > This code was following examples of other open source drivers - bcm2835 > and exynos both use cpu_relax. I'll have to look into this more to > understand. > The majority of the driver apparently use udelay(10) to wait, which is not much better but at least consistent. The cpu_relax() call probably gives better throughput. I don't understand why none of the drivers actually attempts to msleep(), but that may be because the delay is much too long. Can you find out what the expected latency is for new data to become available on your hardware? Arnd