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
