On Tue, Dec 12, 2017 at 11:30 AM, Łukasz Stelmach
<l.stelmach@xxxxxxxxxxx> wrote:
> 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.
I understand. With the fix for first line (cc):
Reviewed-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
Best regards,
Krzysztof
