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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux