linux-kernel - Re: [PATCH v2 2/2] hwrng: iproc-rng200

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4488852.TuVxKbAYde@wuerfel>
Date:	Thu, 26 Feb 2015 21:15:49 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Scott Branden <sbranden@...adcom.com>
Cc:	bcm-kernel-feedback-list@...adcom.com,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Matt Mackall <mpm@...enic.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Grant Likely <grant.likely@...aro.org>,
	Ray Jui <rjui@...adcom.com>,
	Jonathan Richardson <jonathar@...adcom.com>,
	Dmitry Torokhov <dtor@...gle.com>,
	Anatol Pomazao <anatol@...gle.com>, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver

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 <rjui@...adcom.com>
> >> Signed-off-by: Scott Branden <sbranden@...adcom.com>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/