Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver
On Friday, March 24, 2017 07:19:34 PM Krzysztof Kozlowski wrote: > On Fri, Mar 24, 2017 at 05:11:25PM +0100, Bartlomiej Zolnierkiewicz wrote: > > On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote: > > > I really do not like global or file-scope variables. I do not like > > > drivers using them. Actually I hate them. > > > > > > From time to time I encounter a driver which was designed with that > > > approach - static fields and hidden assumption that there will be only > > > one instance. Usually that assumption is really hidden... > > > > > > ... and then it happens that I want to use two instances which of course > > > fails. > > > > > > This code serves as a clear documentation for this assumption - only one > > > instance is allowed. You can look at it as a self-documenting > > > requirement. > > > > For me it looks as needless case of defensive programming and when > > I see the code like this it always raises questions about the real > > intentions of the code. I find it puzzling and not helpful. > > I do not understand what might be puzzling about check for static > file-scope value. It is of course subjective, but for me that looks > pretty self-explanatory. The check should never happen given that ->probe will not happen twice. However it seems that this is possible now with DT platform devices and incorrect DTB. > > > And I think the probe might be called twice, for example in case of > > > mistake in DTB. > > > > Even if this is possible resource allocation code in the driver will > > take take care of handling it just fine, > > Indeed, the devm_ioremap_resource() solves the case. I can drop the > check then. Looking on this a bit more it seems that devm_ioremap_resource() will not cover all mistakes (using compatible by mistake in some other DTB node). Leave the check, I take my objection back. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R Institute Poland Samsung Electronics
Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver
On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote:
> On Fri, Mar 24, 2017 at 04:26:47PM +0100, Bartlomiej Zolnierkiewicz wrote:
> >
> > Hi,
> >
> > Firstly, thanks for working on this.
> >
> > The patch looks fine overall for me, some review comments below.
> >
> > On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote:
> > > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
> > > This is a driver for pseudo random number generator block which on
> > > Exynos4 chipsets must be seeded with some value. On newer Exynos5420
> > > chipsets it might seed itself from true random number generator block
> > > but this is not implemented yet.
> > >
> > > New driver is a complete rework to use the crypto ALGAPI instead of
> > > hw_random API. Rationale for the change:
> > > 1. hw_random interface is for true RNG devices.
> > > 2. The old driver was seeding itself with jiffies which is not a
> > >reliable source for randomness.
> > > 3. Device generates five random numbers in each pass but old driver was
> > >returning only one thus its performance was reduced.
> > >
> > > Compatibility with DeviceTree bindings is preserved.
> > >
> > > New driver does not use runtime power management but manually enables
> > > and disables the clock when needed. This is preferred approach because
> > > using runtime PM just to toggle clock is huge overhead. Another
> >
> > I'm not entirely convinced that the new approach is better.
> >
> > With the old approach exynos_rng_generate() can be called more
> > than once before PM autosuspend kicks in and thus clk_prepare_enable()/
> > clk_disable()_unprepare() operations will be done only once. This
> > would give better performance on the "burst" operations.
> >
> > [ The above assumes that clock operations are more costly than
> > going through PM core to check the current device state. ]
>
> I agree that we loose the "burst" mode but:
> 1. At least on Exynso4 SSS is part of TOP power domain so it will not
>help to reduce any more power consumption (on Exynos5422 it is
>mentioned in G2D... which seems incorrect).
> 2. I think the overhead of clk operations is much smaller. These are only
>two locks (prepare mutex + enable spin), simple tree traversal and
>play with few SFRs.
>
>The power domain code in comparison to that is huge and complicated
>with inter-device links and dependencies. Also the actual runtime PM
>suspend would anyway fall back at then end to clk prepare/enable
>locks and paths.
>
>We've been talking about this a lot with Marek Szyprowski (cc'ed) and
>he was always (AFAIR) against attempts of runtime power
>management of a single clock...
OK, thanks for explanation.
> > > +static int exynos_rng_probe(struct platform_device *pdev)
> > > +{
> > > + struct exynos_rng_dev *rng;
> > > + struct resource *res;
> > > + int ret;
> > > +
> > > + if (exynos_rng_dev)
> > > + return -EEXIST;
> >
> > How this condition could ever happen?
> >
> > The probe function will never be called twice.
>
> I really do not like global or file-scope variables. I do not like
> drivers using them. Actually I hate them.
>
> From time to time I encounter a driver which was designed with that
> approach - static fields and hidden assumption that there will be only
> one instance. Usually that assumption is really hidden...
>
> ... and then it happens that I want to use two instances which of course
> fails.
>
> This code serves as a clear documentation for this assumption - only one
> instance is allowed. You can look at it as a self-documenting
> requirement.
For me it looks as needless case of defensive programming and when
I see the code like this it always raises questions about the real
intentions of the code. I find it puzzling and not helpful.
> And I think the probe might be called twice, for example in case of
> mistake in DTB.
Even if this is possible resource allocation code in the driver will
take take care of handling it just fine,
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver
Hi,
Firstly, thanks for working on this.
The patch looks fine overall for me, some review comments below.
On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote:
> Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
> This is a driver for pseudo random number generator block which on
> Exynos4 chipsets must be seeded with some value. On newer Exynos5420
> chipsets it might seed itself from true random number generator block
> but this is not implemented yet.
>
> New driver is a complete rework to use the crypto ALGAPI instead of
> hw_random API. Rationale for the change:
> 1. hw_random interface is for true RNG devices.
> 2. The old driver was seeding itself with jiffies which is not a
>reliable source for randomness.
> 3. Device generates five random numbers in each pass but old driver was
>returning only one thus its performance was reduced.
>
> Compatibility with DeviceTree bindings is preserved.
>
> New driver does not use runtime power management but manually enables
> and disables the clock when needed. This is preferred approach because
> using runtime PM just to toggle clock is huge overhead. Another
I'm not entirely convinced that the new approach is better.
With the old approach exynos_rng_generate() can be called more
than once before PM autosuspend kicks in and thus clk_prepare_enable()/
clk_disable()_unprepare() operations will be done only once. This
would give better performance on the "burst" operations.
[ The above assumes that clock operations are more costly than
going through PM core to check the current device state. ]
> +static int exynos_rng_get_random(struct exynos_rng_dev *rng,
> + u8 *dst, unsigned int dlen,
> + unsigned int *read)
> +{
> + int retry = 100;
I know that this is copied verbatim from the old driver but please
use define for the maximum number of retries.
> +static int exynos_rng_probe(struct platform_device *pdev)
> +{
> + struct exynos_rng_dev *rng;
> + struct resource *res;
> + int ret;
> +
> + if (exynos_rng_dev)
> + return -EEXIST;
How this condition could ever happen?
The probe function will never be called twice.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
Re: [PATCH 4/4] crypto: s5p-sss - Use mutex instead of spinlock
Hi,
On Friday, March 17, 2017 04:49:22 PM Krzysztof Kozlowski wrote:
> Driver uses threaded interrupt handler so there is no real need for
> using spinlocks for synchronization. Mutexes would do fine and are
> friendlier for overall system preemptivness and real-time behavior.
Are you sure that this conversion is safe? This driver also uses
a tasklet and tasklets run in the interrupt context.
> @@ -667,18 +666,17 @@ static void s5p_tasklet_cb(unsigned long data)
> struct s5p_aes_dev *dev = (struct s5p_aes_dev *)data;
> struct crypto_async_request *async_req, *backlog;
> struct s5p_aes_reqctx *reqctx;
> - unsigned long flags;
>
> - spin_lock_irqsave(>lock, flags);
> + mutex_lock(>lock);
> backlog = crypto_get_backlog(>queue);
> async_req = crypto_dequeue_request(>queue);
>
> if (!async_req) {
> dev->busy = false;
> - spin_unlock_irqrestore(>lock, flags);
> + mutex_unlock(>lock);
> return;
> }
> - spin_unlock_irqrestore(>lock, flags);
> + mutex_unlock(>lock);
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
Re: [PATCH 1/4] crypto: s5p-sss - Close possible race for completed requests
On Friday, March 17, 2017 04:49:19 PM Krzysztof Kozlowski wrote:
> Driver is capable of handling only one request at a time and it stores
> it in its state container struct s5p_aes_dev. This stored request must be
> protected between concurrent invocations (e.g. completing current
> request and scheduling new one). Combination of lock and "busy" field
> is used for that purpose.
>
> When "busy" field is true, the driver will not accept new request thus
> it will not overwrite currently handled data.
>
> However commit 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion
> on LRW(AES)") moved some of the write to "busy" field out of a lock
> protected critical section. This might lead to potential race between
> completing current request and scheduling a new one. Effectively the
> request completion might try to operate on new crypto request.
>
> Cc: <sta...@vger.kernel.org> # v4.10.x
> Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)")
> Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnier...@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
Re: [PATCH v4 2/2] crypto: hw_random - Add new Exynos RNG driver
Hi, On Saturday, April 08, 2017 03:32:45 PM Krzysztof Kozlowski wrote: > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one. > This is a driver for pseudo random number generator block which on > Exynos4 chipsets must be seeded with some value. On newer Exynos5420 > chipsets it might seed itself from true random number generator block > but this is not implemented yet. > > New driver is a complete rework to use the crypto ALGAPI instead of > hw_random API. Rationale for the change: > 1. hw_random interface is for true RNG devices. > 2. The old driver was seeding itself with jiffies which is not a >reliable source for randomness. > 3. Device generates five random 32-bit numbers in each pass but old >driver was returning only one 32-bit number thus its performance was >reduced. > > Compatibility with DeviceTree bindings is preserved. > > New driver does not use runtime power management but manually enables > and disables the clock when needed. This is preferred approach because > using runtime PM just to toggle clock is huge overhead. > > Another difference is reseeding itself with generated random data > periodically and during resuming from system suspend (previously driver > was re-seeding itself again with jiffies). > > Signed-off-by: Krzysztof Kozlowski <k...@kernel.org> > Reviewed-by: Stephan Müller <smuel...@chronox.de> > Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnier...@samsung.com> Best regards, -- Bartlomiej Zolnierkiewicz Samsung R Institute Poland Samsung Electronics
Re: [RFT PATCH] crypto: ahash.c: Require export/import in ahash
On Tuesday, January 16, 2018 11:35:44 AM Kamil Konieczny wrote:
> Export and import were optional in async hash. As drivers were rewritten,
> they become mandatory now, so correct init of ahash transformation.
>
> Signed-off-by: Kamil Konieczny <k.koniec...@partner.samsung.com>
> ---
> Tested with crypto run-time self test on Odroid-U3 with Exynos 4412 CPU,
> with insmod s5p-sss.ko
> Please test with other crypto hash drivers.
Testing all existing ahash drivers is impossible so the code audit
should be done instead. From the quick look there are 3 hash drivers
left that still don't implement ->import/->export methods:
drivers/crypto/mxs-dcp.c
drivers/crypto/n2_core.c
drivers/crypto/ux500/hash/hash_core.c
It seems that after this patch they will OOPS, currently they now
return errors on ->import/->export attempts.
Please verify this and if necessary add dummy ->import/->export
implementations to affected drivers + contact their maintainers
(or authors if there is no maintainer assigned) to make them
aware of the problem (maybe some drivers should be removed now?).
> crypto/ahash.c | 18 ++
> 1 file changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index 3a35d67de7d9..7a8906d5af53 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -434,16 +434,6 @@ static int ahash_def_finup(struct ahash_request *req)
> return ahash_def_finup_finish1(req, err);
> }
>
> -static int ahash_no_export(struct ahash_request *req, void *out)
> -{
> - return -ENOSYS;
> -}
> -
> -static int ahash_no_import(struct ahash_request *req, const void *in)
> -{
> - return -ENOSYS;
> -}
> -
> static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
> {
> struct crypto_ahash *hash = __crypto_ahash_cast(tfm);
> @@ -451,8 +441,8 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
>
> hash->setkey = ahash_nosetkey;
> hash->has_setkey = false;
> - hash->export = ahash_no_export;
> - hash->import = ahash_no_import;
> + hash->export = alg->export;
> + hash->import = alg->import;
>
> if (tfm->__crt_alg->cra_type != _ahash_type)
> return crypto_init_shash_ops_async(tfm);
> @@ -467,10 +457,6 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
> hash->setkey = alg->setkey;
> hash->has_setkey = true;
> }
> - if (alg->export)
> - hash->export = alg->export;
> - if (alg->import)
> - hash->import = alg->import;
>
> return 0;
> }
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
