| Message ID | 20171211140623.7673-5-l.stelmach@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Dec 11, 2017 at 3:06 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote: > Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > 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@samsung.com> > --- > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@samsung.com> wrote: >> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> >> 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@samsung.com> >> --- >> 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
On Tue, Dec 12, 2017 at 11:30 AM, Łukasz Stelmach <l.stelmach@samsung.com> 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@samsung.com> wrote: >>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >>> >>> 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@samsung.com> >>> --- >>> 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@kernel.org> Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); 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;
Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> 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@samsung.com> --- drivers/crypto/exynos-rng.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)