Messages in this thread | | | Date | Mon, 15 May 2006 15:02:02 -0700 | From | Andrew Morton <> | Subject | Re: [patch 2/9] Add new generic HW RNG core |
| |
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.
> +static int rng_dev_open(struct inode *inode, struct file *filp)
Like that.
> +static ssize_t rng_dev_read(struct file *filp, char __user *buf, > + size_t size, loff_t *offp) > +{ > + int have_data; > + u32 data; > + ssize_t ret = 0; > + int i, err = 0; > + > + while (size) { > + err = -ERESTARTSYS; > + if (mutex_lock_interruptible(&rng_mutex)) > + goto out; > + if (!current_rng) { > + mutex_unlock(&rng_mutex); > + err = -ENODEV; > + goto out; > + } > + have_data = 0; > + if (hwrng_data_present(current_rng)) > + have_data = hwrng_data_read(current_rng, &data); > + mutex_unlock(&rng_mutex); > + > + err = -EFAULT; > + while (have_data && size) { > + if (put_user((u8)data, buf++)) > + goto out; > + size--; > + ret++; > + have_data--; > + data >>= 8; > + } > + > + err = -EAGAIN; > + if (filp->f_flags & O_NONBLOCK) > + goto out; > + > + err = -ERESTARTSYS; > + if (need_resched()) { > + if (schedule_timeout_interruptible(1)) > + goto out; > + } else { > + if (mutex_lock_interruptible(&rng_mutex)) > + goto out; > + if (!current_rng) { > + mutex_unlock(&rng_mutex); > + err = -ENODEV; > + goto out; > + } > + for (i = 0; i < 20; i++) { > + if (hwrng_data_present(current_rng)) > + break; > + udelay(10); > + } > + mutex_unlock(&rng_mutex); > + } > + if (signal_pending(current)) > + goto out; > + } > +out: > + return ret ? : err; > +}
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.
What's the polling of hwrng_data_present() doing in here? (Unobvious, needs a comment).
> +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?
> +MODULE_AUTHOR("The Linux Kernel team");
Mutter. Might as well remove this.
A MAINTAINERS record would be nice.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |