Quoting Timur Tabi (2018-06-21 08:17:55)
> @@ -96,11 +110,39 @@ static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
>
> /* read random data from hardware */
> do {
> - val = readl_relaxed(rng->base + PRNG_STATUS);
> - if (!(val & PRNG_STATUS_DATA_AVAIL))
> - break;
> + spin_lock(&rng->lock);
> +
> + /*
> + * First check the status bit. If 'wait' is true, then wait
> + * up to TIMEOUT microseconds for data to be available.
> + */
> + if (wait) {
> + int ret;
Please don't do local variables like this. Put them at the top of the
function.
> +
> + ret = readl_poll_timeout_atomic(rng->base + PRNG_STATUS,
> + val, val & PRNG_STATUS_DATA_AVAIL, 0, TIMEOUT);
> + if (ret) {
> + /* Timed out */
> + spin_unlock(&rng->lock);
> + break;
> + }
> + } else {
> + val = readl_relaxed(rng->base + PRNG_STATUS);
> + if (!(val & PRNG_STATUS_DATA_AVAIL)) {
> + spin_unlock(&rng->lock);
> + break;
> + }
> + }
>
> val = readl_relaxed(rng->base + PRNG_DATA_OUT);
> + spin_unlock(&rng->lock);
Maybe this should be written as:
spin_lock()
if (wait) {
has_data = readl_poll_timeout_atomic(...) == 0;
} else {
val = readl_relaxed(rng->base + PRNG_STATUS);
has_data = val & PRNG_STATUS_DATA_AVAIL;
}
if (has_data)
val = readl_relaxed(rng->base + PRNG_DATA_OUT);
spin_unlock();
if (!has_data)
break;
> +
> + /*
> + * Zero is technically a valid random number, but it's also
> + * the value returned if the PRNG is not enabled properly.
> + * To avoid accidentally returning all zeros, treat it as
> + * invalid and just return what we've already read.
> + */
> if (!val)
> break;
Is this a related change? Looks like a nice comment that isn't related
to locking.
