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

[Åukasz Stelmach]

It was <2017-12-11 pon 16:03>, when Krzysztof Kozlowski wrote:
> 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...

Moving locks/unlocks to exynos_rng_get_random() means taking a lock to
retrieve 20 bytes. It doesn't scale at all. I really wanted to avoid it,
because the performance loss is quite noticable in such case. That is
why I put the lock around the loop in exynos_rng_generatr(). As a
consequence I had to move locks out of exynos_rng_set_seed() too.

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


-- 
Åukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Attachment: signature.asc
Description: PGP signature