Re: [patch 2/9] Add new generic HW RNG core

[Michael Buesch <mb@bu3sch.de>]

I will send patches for the following stuff, on top
of your current -mm, which has my previous patches applied.

On Tuesday 16 May 2006 00:02, you wrote:
> Michael Buesch <mb@bu3sch.de> wrote:
> > +static inline
> > +int hwrng_init(struct hwrng *rng)
> > +static inline
> > +void hwrng_cleanup(struct hwrng *rng)
> > +static inline
> > +int hwrng_data_present(struct hwrng *rng)
> > +static inline
> > +int hwrng_data_read(struct hwrng *rng, u32 *data)
> 
> Lose the newlines, please.

No problem.

> What's going on with the need_resched() tricks in there?  (Unobvious, needs
> a comment).  From my reading, it'll cause a caller to this function to hang
> for arbitrary periods of time if something if causing heavy scheduling
> pressure.

Yeah, it was like this in the old RNG driver. I would also like to
drop it, as I don't see a real advantage from it. But I don't
like to drop the hwrng_data_present() polling bit of it, because...

> What's the polling of hwrng_data_present() doing in here?  (Unobvious,
> needs a comment).

Some HW RNG might require some time between data_reads to gather
new entropy. The short polling here makes sure we don't return too
early from the syscall. So it reduces syscall overhead. Imagine a
device which needs 20usecs between reads to gather new entropy.
The first read will succeed and probably only return one byte of
entropy. The next attempt to data_present in the while loop will
fail and the syscall will return with one byte read. userspace will
need to call the syscall for every byte (in this case).

We could, of course, add a variable to struct hwrng that indicates the
average needed time a device needs to gather new entropy in most cases.
So we could poll for this time instead of the rather random 200usecs. 

> > +static ssize_t hwrng_attr_current_store(struct class_device *class,
> > +					const char *buf, size_t len)
> > +{
> > +	int err;
> > +	struct hwrng *rng;
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> 
> Are the sysfs permissions not adequate?

Will drop this.

> > +MODULE_AUTHOR("The Linux Kernel team");
> 
> Mutter.  Might as well remove this.

Sure. Leftover from the old driver.

> A MAINTAINERS record would be nice.

Yes, sir.