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.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
