On Mon, 2009-06-01 at 01:45 +0900, Atsushi Nemoto wrote:
> On Fri, 29 May 2009 16:29:07 -0700, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > I assume that the MIPS patch "[PATCH] TXx9: Add TX4939 RNG support"
> > depends upon this patch?
>
> To build kernel or driver, no dependencies. To use this device
> actually, both patches are needed.
>
> > > +static u64 read_rng(void __iomem *base, unsigned int offset)
> > > +{
> > > + /* Caller must disable interrupts */
> > > + return ____raw_readq(base + offset);
> > > +}
> >
> > What is the reasoning behind the local_irq_disable() requirement?
> >
> > Because I'm wondering whether this is safe on SMP?
>
> As Ralf replied, These local_irq_disable stuff are just for 64-bit
> access on 32-bit kernel. Maybe something like this is preferred?
>
> static void ____raw_io_start(void)
> {
> #ifndef CONFIG_64BIT
> /* some comments... */
> local_irq_enable();
> #endif
> }
>
> static void ____raw_io_end(void)
> {
> #ifndef CONFIG_64BIT
> /* see above */
> local_irq_disable();
> #endif
> }
>
> For SMP concurrent access, these rountines are protected by mutex in
> rng-core. Also this SoC does not support SMP. There should be no
> problem here.
>
> > > + for (i = 0; i < 20; i++) {
> ...
> > > + udelay(1);
> > > + }
> > > + return rngdev->data_avail;
> > > +}
> >
> > The mysterious udelay() needs a comment, because there is no way in
> > which the reader can otherwise work out why it is there.
>
> Well, this comments can be applied most RNG drivers ;)
>
> Anyway, I will add some comment here. I take this loop (20 loops with
> udelay) from other drivers and changed to udelay(1) because the
> datasheed states "90 bus clock cycles by default" for generation
> (typically 450ns for this SoC).
>
> > > +static int tx4939_rng_data_read(struct hwrng *rng, u32 *buffer)
> > > +{
> > > + struct tx4939_rng *rngdev = container_of(rng, struct tx4939_rng, rng);
> > > +
> > > + rngdev->data_avail--;
> > > + *buffer = *((u32 *)&rngdev->databuf + rngdev->data_avail);
> > > + return sizeof(u32);
> > > +}
> >
> > Concurrent callers can corrupt rngdev->data_avail ?
>
> This is protected by rng_mutex in rng-core.
>
> > > + /* Start RNG */
> > > + write_rng(TX4939_RNG_RCSR_ST, rngdev->base, TX4939_RNG_RCSR);
> > > + local_irq_enable();
> > > + /* drop first two results */
> >
> > The comment doesn't provide the reason for doing this?
>
> >From the datasheet:
>
> The quality of the random numbers generated immediately after
> reset can be insufficient. Therefore, do not use random
> numbers obtained from the first and second generations; use
> the ones from the third or subsequent generation.
Does the datasheet say anything about -how- the random numbers are
produced? Most physical sources that I'm aware of don't have this sort
of issue. But some pseudo-RNGs do. So this looks a little worrisome.
--
http://selenic.com : development and support for Mercurial and Linux
