On 21-06-18, 10:17, Timur Tabi wrote:
> The hwrng.read callback includes a boolean parameter called 'wait'
> which indicates whether the function should block and wait for
> more data.
>
> When 'wait' is true, the driver spins on the DATA_AVAIL bit or until
> a reasonable timeout. The timeout can occur if there is a heavy load
> on reading the PRNG.
>
> The same code also needs a spinlock to protect against race conditions.
>
> If multiple cores hammer on the PRNG, it's possible for a race
> condition to occur between reading the status register and
> reading the data register. Add a spinlock to protect against
> that.
>
> 1. Core 1 reads status register, shows data is available.
> 2. Core 2 also reads status register, same result
> 3. Core 2 reads data register, depleting all entropy
> 4. Core 1 reads data register, which returns 0
>
> Signed-off-by: Timur Tabi <ti...@codeaurora.org>
> ---
> drivers/char/hw_random/msm-rng.c | 57
> +++++++++++++++++++++++++++++++++++-----
> 1 file changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/char/hw_random/msm-rng.c
> b/drivers/char/hw_random/msm-rng.c
> index 841fee845ec9..44580588b938 100644
> --- a/drivers/char/hw_random/msm-rng.c
> +++ b/drivers/char/hw_random/msm-rng.c
> @@ -15,9 +15,11 @@
> #include <linux/err.h>
> #include <linux/hw_random.h>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>
> /* Device specific register offsets */
> #define PRNG_DATA_OUT 0x0000
> @@ -35,10 +37,22 @@
> #define MAX_HW_FIFO_SIZE (MAX_HW_FIFO_DEPTH * 4)
> #define WORD_SZ 4
>
> +/*
> + * Normally, this would be the maximum time it takes to refill the FIFO,
> + * after a read. Under heavy load, tests show that this delay is either
> + * below 50us or above 2200us. The higher value is probably what happens
> + * when entropy is completely depleted.
> + *
> + * Since we don't want to wait 2ms in a spinlock, set the timeout to the
> + * lower value. Under extreme situations, that timeout can extend to 100us.
> + */
> +#define TIMEOUT 50
> +
> struct msm_rng {
> void __iomem *base;
> struct clk *clk;
> struct hwrng hwrng;
> + spinlock_t lock;
> };
>
> #define to_msm_rng(p) container_of(p, struct msm_rng, hwrng)
> @@ -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;
> +
> + 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);
> +
> + /*
> + * 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;
>
> @@ -148,10 +190,11 @@ static int msm_rng_probe(struct platform_device *pdev)
> if (IS_ERR(rng->clk))
> return PTR_ERR(rng->clk);
>
> - rng->hwrng.name = KBUILD_MODNAME,
> - rng->hwrng.init = msm_rng_init,
> - rng->hwrng.cleanup = msm_rng_cleanup,
> - rng->hwrng.read = msm_rng_read,
> + rng->hwrng.name = KBUILD_MODNAME;
> + rng->hwrng.init = msm_rng_init;
> + rng->hwrng.cleanup = msm_rng_cleanup;
> + rng->hwrng.read = msm_rng_read;
this should be a separate patch
--
~Vinod
