[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

Timur Tabi
2018-06-22T13:11:07Z

On 6/22/18 12:36 AM, Stephen Boyd wrote:
> 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.

Ok.

>
>> +
>> + 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;

That would work, but I don't really see this as better, just different.

>> + /*
>> + * 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.

It's slightly related in that the locking is needed to reduce the number
of times we read zero from the DATA_OUT register.