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

[Krzysztof Kozlowski]

On Tue, Dec 12, 2017 at 11:30 AM, Łukasz Stelmach
 wrote:
> It was <2017-12-11 pon 16:03>, when Krzysztof Kozlowski wrote:
>> On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach  
>> wrote:
>>> Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 
>>> 
>>>
>>> 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 
>>> ---
>>>  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 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>
>>> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
>>> enum exynos_prng_type   type;
>>> void __iomem*mem;
>>> struct clk  *clk;
>>> +   struct mutexlock;
>>> /* Generated numbers stored for seeding during resume */
>>> u8  seed_save[EXYNOS_RNG_SEED_SIZE];
>>> unsigned intseed_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(>lock);
>>> +   mutex_lock(>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(>lock);
>>> do {
>>> ret = exynos_rng_get_random(rng, dst, dlen, );
>>> if (ret)
>>> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>>>
>>> exynos_rng_reseed(rng);
>>> } while (dlen > 0);
>>> +   mutex_unlock(>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(>lock);
>>> ret = exynos_rng_set_seed(ctx->rng, seed, slen);
>>> +   mutex_unlock(>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 

Best regards,
Krzysztof

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  
> wrote:
>> Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 
>> 
>>
>> 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 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>
>> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
>> enum exynos_prng_type   type;
>> void __iomem*mem;
>> struct clk  *clk;
>> +   struct mutexlock;
>> /* Generated numbers stored for seeding during resume */
>> u8  seed_save[EXYNOS_RNG_SEED_SIZE];
>> unsigned intseed_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(>lock);
>> +   mutex_lock(>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(>lock);
>> do {
>> ret = exynos_rng_get_random(rng, dst, dlen, );
>> if (ret)
>> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>>
>> exynos_rng_reseed(rng);
>> } while (dlen > 0);
>> +   mutex_unlock(>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(>lock);
>> ret = exynos_rng_set_seed(ctx->rng, seed, slen);
>> +   mutex_unlock(>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(>lock);
>> +
>> rng->dev = >dev;
>> rng->clk = devm_clk_get(>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(>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(>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(>lock);
>> +
>> ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
>>
>> +   mutex_unlock(>lock);
>> +
>> clk_disable_unprepare(rng->clk);
>>
>> return ret;
>> --
>> 2.11.0


-- 
Łukasz Stelmach
Samsung R Institute Poland
Samsung Electronics


signature.asc
Description: PGP signature

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  wrote:
> Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 
> 
>
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -79,6 +80,7 @@ struct exynos_rng_dev {
> enum exynos_prng_type   type;
> void __iomem*mem;
> struct clk  *clk;
> +   struct mutexlock;
> /* Generated numbers stored for seeding during resume */
> u8  seed_save[EXYNOS_RNG_SEED_SIZE];
> unsigned intseed_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(>lock);
> +   mutex_lock(>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(>lock);
> do {
> ret = exynos_rng_get_random(rng, dst, dlen, );
> if (ret)
> @@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
>
> exynos_rng_reseed(rng);
> } while (dlen > 0);
> +   mutex_unlock(>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(>lock);
> ret = exynos_rng_set_seed(ctx->rng, seed, slen);
> +   mutex_unlock(>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(>lock);
> +
> rng->dev = >dev;
> rng->clk = devm_clk_get(>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(>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(>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(>lock);
> +
> ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
>
> +   mutex_unlock(>lock);
> +
> clk_disable_unprepare(rng->clk);
>
> return ret;
> --
> 2.11.0
>

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

[Łukasz Stelmach]

Cc: Marek Szyprowski , Bartlomiej Zolnierkiewicz 


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 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -79,6 +80,7 @@ struct exynos_rng_dev {
enum exynos_prng_type   type;
void __iomem*mem;
struct clk  *clk;
+   struct mutexlock;
/* Generated numbers stored for seeding during resume */
u8  seed_save[EXYNOS_RNG_SEED_SIZE];
unsigned intseed_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(>lock);
+   mutex_lock(>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(>lock);
do {
ret = exynos_rng_get_random(rng, dst, dlen, );
if (ret)
@@ -217,6 +224,7 @@ static int exynos_rng_generate(struct crypto_rng *tfm,
 
exynos_rng_reseed(rng);
} while (dlen > 0);
+   mutex_unlock(>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(>lock);
ret = exynos_rng_set_seed(ctx->rng, seed, slen);
+   mutex_unlock(>lock);
 
clk_disable_unprepare(rng->clk);
 
@@ -284,6 +294,8 @@ static int exynos_rng_probe(struct platform_device *pdev)
return -ENOTSUPP;
}
 
+   mutex_init(>lock);
+
rng->dev = >dev;
rng->clk = devm_clk_get(>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(>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(>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(>lock);
+
ret = exynos_rng_set_seed(rng, rng->seed_save, rng->seed_save_len);
 
+   mutex_unlock(>lock);
+
clk_disable_unprepare(rng->clk);
 
return ret;
-- 
2.11.0