Re: [PATCH v2 4/4] crypto: exynos - Introduce mutex to prevent concurrent access to hardware

[Krzysztof Kozlowski]

On Mon, Dec 11, 2017 at 3:06 PM, Åukasz Stelmach <l.stelmach@xxxxxxxxxxx> wrote:
> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>, Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>
> Hardware operations like reading random numbers and setting a seed need
> to be conducted in a single thread. Therefore a mutex is required to
> prevent multiple threads (processes) from accessing the hardware at the
> same time.
>
> The sequence of mutex_lock() and mutex_unlock() in the exynos_rng_reseed()
> function enables switching between different threads waiting for the
> driver to generate random numbers for them.
>
> Signed-off-by: Åukasz Stelmach <l.stelmach@xxxxxxxxxxx>
> ---
>  drivers/crypto/exynos-rng.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
> index c72a838f1932..6209035ca659 100644
> --- a/drivers/crypto/exynos-rng.c
> +++ b/drivers/crypto/exynos-rng.c
> @@ -22,6 +22,7 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>
> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
>         enum exynos_prng_type           type;
>         void __iomem                    *mem;
>         struct clk                      *clk;
> +       struct mutex                    lock;
>         /* Generated numbers stored for seeding during resume */
>         u8                              seed_save[EXYNOS_RNG_SEED_SIZE];
>         unsigned int                    seed_save_len;
> @@ -192,6 +194,10 @@ static void exynos_rng_reseed(struct exynos_rng_dev *rng)
>                 return;
>
>         exynos_rng_set_seed(rng, seed, read);
> +
> +       /* Let others do some of their job. */
> +       mutex_unlock(&rng->lock);
> +       mutex_lock(&rng->lock);
>  }
>
>  static int exynos_rng_generate(struct crypto_rng *tfm,
> @@ -207,6 +213,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>         if (ret)
>                 return ret;
>
> +       mutex_lock(&rng->lock);
>         do {
>                 ret = exynos_rng_get_random(rng, dst, dlen, &read);
>                 if (ret)
> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>
>                 exynos_rng_reseed(rng);
>         } while (dlen > 0);
> +       mutex_unlock(&rng->lock);
>
>         clk_disable_unprepare(rng->clk);
>
> @@ -234,7 +242,9 @@ static int exynos_rng_seed(struct crypto_rng *tfm, const u8 *seed,
>         if (ret)
>                 return ret;
>
> +       mutex_lock(&rng->lock);
>         ret = exynos_rng_set_seed(ctx->rng, seed, slen);
> +       mutex_unlock(&rng->lock);

I think the number of mutex locks/unlock statements can be reduced
(including the mutex unlock+lock pattern) after moving the mutex to
exynos_rng_set_seed() and exynos_rng_get_random() because actually you
want to protect them. This would remove the new code from suspend and
resume path and gave you the fairness.

On the other hand the mutex would be unlocked+locked many times for
large generate() calls...

Best regards,
Krzysztof

>         clk_disable_unprepare(rng->clk);
>
> @@ -284,6 +294,8 @@ static int exynos_rng_probe(struct platform_device *pdev)
>                 return -ENOTSUPP;
>         }
>
> +       mutex_init(&rng->lock);
> +
>         rng->dev = &pdev->dev;
>         rng->clk = devm_clk_get(&pdev->dev, "secss");
>         if (IS_ERR(rng->clk)) {
> @@ -334,9 +346,14 @@ static int __maybe_unused exynos_rng_suspend(struct device *dev)
>         if (ret)
>                 return ret;
>
> +       mutex_lock(&rng->lock);
> +
>         /* Get new random numbers and store them for seeding on resume. */
>         exynos_rng_get_random(rng, rng->seed_save, sizeof(rng->seed_save),
>                               &(rng->seed_save_len));
> +
> +       mutex_unlock(&rng->lock);
> +
>         dev_dbg(rng->dev, "Stored %u bytes for seeding on system resume\n",
>                 rng->seed_save_len);
>
> @@ -359,8 +376,12 @@ static int __maybe_unused exynos_rng_resume(struct device *dev)
>         if (ret)
>                 return ret;
>
> +       mutex_lock(&rng->lock);
> +
>         ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
>
> +       mutex_unlock(&rng->lock);
> +
>         clk_disable_unprepare(rng->clk);
>
>         return ret;
> --
> 2.11.0
>