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 > --- > 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 != &crypto_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&D 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 > Reviewed-by: Stephan Müller > Reviewed-by: PrasannaKumar Muralidharan Reviewed-by: Bartlomiej Zolnierkiewicz Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
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&D 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&D 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&D Institute Poland Samsung Electronics
Re: [PATCH 3/4] crypto: s5p-sss - Document the struct s5p_aes_dev
On Friday, March 17, 2017 04:49:21 PM Krzysztof Kozlowski wrote: > Add kernel-doc to s5p_aes_dev structure. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Bartlomiej Zolnierkiewicz Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
Re: [PATCH 2/4] crypto: s5p-sss - Remove unused variant field from state container
On Friday, March 17, 2017 04:49:20 PM Krzysztof Kozlowski wrote: > The driver uses type of device (variant) only during probe so there is > no need to store it for later. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Bartlomiej Zolnierkiewicz Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D 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: # v4.10.x > Fixes: 28b62b145868 ("crypto: s5p-sss - Fix spinlock recursion on LRW(AES)") > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Bartlomiej Zolnierkiewicz Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D 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(&dev->lock, flags); > + mutex_lock(&dev->lock); > backlog = crypto_get_backlog(&dev->queue); > async_req = crypto_dequeue_request(&dev->queue); > > if (!async_req) { > dev->busy = false; > - spin_unlock_irqrestore(&dev->lock, flags); > + mutex_unlock(&dev->lock); > return; > } > - spin_unlock_irqrestore(&dev->lock, flags); > + mutex_unlock(&dev->lock); Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics