Re: [PATCH v3] crypto: s5p-sss: Add HASH support for Exynos
On 03.10.2017 21:30, Krzysztof Kozlowski wrote:
> On Tue, Oct 03, 2017 at 04:57:43PM +0200, Kamil Konieczny wrote:
>
[...]
+static struct ahash_alg algs_sha256[] = {
+{
+ .init = s5p_hash_init,
+ .update = s5p_hash_update,
+ .final = s5p_hash_final,
+ .finup = s5p_hash_finup,
+ .digest = s5p_hash_digest,
+ .halg.digestsize= SHA256_DIGEST_SIZE,
+ .halg.base = {
+ .cra_name = "sha256",
+ .cra_driver_name= "exynos-sha256",
+ .cra_priority = 100,
+ .cra_flags = CRYPTO_ALG_TYPE_AHASH |
+CRYPTO_ALG_KERN_DRIVER_ONLY |
+CRYPTO_ALG_ASYNC |
+CRYPTO_ALG_NEED_FALLBACK,
+ .cra_blocksize = HASH_BLOCK_SIZE,
+ .cra_ctxsize= sizeof(struct s5p_hash_ctx),
+ .cra_alignmask = SSS_DMA_ALIGN_MASK,
+ .cra_module = THIS_MODULE,
+ .cra_init = s5p_hash_cra_init,
+ .cra_exit = s5p_hash_cra_exit,
+ }
+}
+
+};
+
+static struct sss_hash_algs_info exynos_hash_algs_info[] = {
>>>
>>> You have warnings in your code. Please be sure that all compiler,
>>> Smatch, Sparse, checkpatch and coccicheck warnings are fixed.
>>>
>>> ../drivers/crypto/s5p-sss.c:1896:34: warning: ‘exynos_hash_algs_info’
>>> defined but not used [-Wunused-variable]
>>> static struct sss_hash_algs_info exynos_hash_algs_info[] = {
>>>
>>> Probably this should be __maybe_unused.
>>
>> You are right, I did not check this with EXYNOS_HASH disabled, I will
>> rewrite it.
>>
>>> Also this should be const. I do not understand why you have to add one
>>> more static variable (which sticks the driver to only one instance...)
>>> and then modify it during runtime. Everything should be stored in device
>>> state container (s5p_aes_dev) - directly or through some other pointers.
>>
>> There is .registered field which is incremented with each algo register.
>> I can move assignes to fields .import, .export and .statesize into struct.
>> When I tried add const, I got compiler warn:
>> drivers/crypto/s5p-sss.c: In function ‘s5p_aes_remove’:
>> drivers/crypto/s5p-sss.c:2397:6: warning: passing argument 1 of
>> ‘crypto_unregister_ahash’ discards ‘const’ qualifier from pointer target
>> type [-Wdiscarded-qualifiers]
>> &hash_algs_i[i].algs_list[j]);
>> so it was not designed to be const (in crypto framework).
>> In AES code the alg struct is also static:
>> static struct crypto_alg algs[] = {
>
> The crypto_alg and ahash_alg must indeed stay non-const but
> sss_hash_algs_info is different. You do not pass it to crypto-core.
It was again from omap driver, which has descriptions for omap2, omap4 and
omap5,
but here I over-complicated this. I will just remove it and simplify register
and deregister hash algs.
>
>> What you mean by 'stick the driver to only one instance' ? In Exynos 4412
>> there
>> is only one SecSS block, in some other Exynos there is SlimSS, but it is not
>> the same (it has lower capabilities and other io addresses), so there should
>> not
>> be two s5p_aes_dev drivers loaded at the same time.
>
> Current driver matches hardware in one-to-one so indeed there cannot be
> two s5p_aes_dev devices. However this might change thus almost every
> driver tries to follow the pattern of state-container passed to device
> (e.g. platform_set_drvdata()). With this approach the code is nicely
> encapsulated and usually much easier to review. Globals (or file-scope
> variables) usually makes code more difficult to maintain.
>
> In this driver this is not entirely possible as some crypto-functions do
> not allow passing driver-supplied opaque pointer. But except this case,
> everywhere else the driver should follow common convention - do not use
> static variables.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
Re: [PATCH v3] crypto: s5p-sss: Add HASH support for Exynos
On Tue, Oct 03, 2017 at 04:57:43PM +0200, Kamil Konieczny wrote:
> >> [...]
> >> +static struct ahash_alg algs_sha256[] = {
> >> +{
> >> + .init = s5p_hash_init,
> >> + .update = s5p_hash_update,
> >> + .final = s5p_hash_final,
> >> + .finup = s5p_hash_finup,
> >> + .digest = s5p_hash_digest,
> >> + .halg.digestsize= SHA256_DIGEST_SIZE,
> >> + .halg.base = {
> >> + .cra_name = "sha256",
> >> + .cra_driver_name= "exynos-sha256",
> >> + .cra_priority = 100,
> >> + .cra_flags = CRYPTO_ALG_TYPE_AHASH |
> >> +CRYPTO_ALG_KERN_DRIVER_ONLY |
> >> +CRYPTO_ALG_ASYNC |
> >> +CRYPTO_ALG_NEED_FALLBACK,
> >> + .cra_blocksize = HASH_BLOCK_SIZE,
> >> + .cra_ctxsize= sizeof(struct s5p_hash_ctx),
> >> + .cra_alignmask = SSS_DMA_ALIGN_MASK,
> >> + .cra_module = THIS_MODULE,
> >> + .cra_init = s5p_hash_cra_init,
> >> + .cra_exit = s5p_hash_cra_exit,
> >> + }
> >> +}
> >> +
> >> +};
> >> +
> >> +static struct sss_hash_algs_info exynos_hash_algs_info[] = {
> >
> > You have warnings in your code. Please be sure that all compiler,
> > Smatch, Sparse, checkpatch and coccicheck warnings are fixed.
> >
> > ../drivers/crypto/s5p-sss.c:1896:34: warning: ‘exynos_hash_algs_info’
> > defined but not used [-Wunused-variable]
> > static struct sss_hash_algs_info exynos_hash_algs_info[] = {
> >
> > Probably this should be __maybe_unused.
>
> You are right, I did not check this with EXYNOS_HASH disabled, I will
> rewrite it.
>
> > Also this should be const. I do not understand why you have to add one
> > more static variable (which sticks the driver to only one instance...)
> > and then modify it during runtime. Everything should be stored in device
> > state container (s5p_aes_dev) - directly or through some other pointers.
>
> There is .registered field which is incremented with each algo register.
> I can move assignes to fields .import, .export and .statesize into struct.
> When I tried add const, I got compiler warn:
> drivers/crypto/s5p-sss.c: In function ‘s5p_aes_remove’:
> drivers/crypto/s5p-sss.c:2397:6: warning: passing argument 1 of
> ‘crypto_unregister_ahash’ discards ‘const’ qualifier from pointer target type
> [-Wdiscarded-qualifiers]
> &hash_algs_i[i].algs_list[j]);
> so it was not designed to be const (in crypto framework).
> In AES code the alg struct is also static:
> static struct crypto_alg algs[] = {
The crypto_alg and ahash_alg must indeed stay non-const but
sss_hash_algs_info is different. You do not pass it to crypto-core.
> What you mean by 'stick the driver to only one instance' ? In Exynos 4412
> there
> is only one SecSS block, in some other Exynos there is SlimSS, but it is not
> the same (it has lower capabilities and other io addresses), so there should
> not
> be two s5p_aes_dev drivers loaded at the same time.
Current driver matches hardware in one-to-one so indeed there cannot be
two s5p_aes_dev devices. However this might change thus almost every
driver tries to follow the pattern of state-container passed to device
(e.g. platform_set_drvdata()). With this approach the code is nicely
encapsulated and usually much easier to review. Globals (or file-scope
variables) usually makes code more difficult to maintain.
In this driver this is not entirely possible as some crypto-functions do
not allow passing driver-supplied opaque pointer. But except this case,
everywhere else the driver should follow common convention - do not use
static variables.
Best regards,
Krzysztof
Re: [PATCH v3] crypto: s5p-sss: Add HASH support for Exynos
On 30.09.2017 21:50, Krzysztof Kozlowski wrote:
> On Wed, Sep 27, 2017 at 02:25:50PM +0200, Kamil Konieczny wrote:
>> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
>> It uses the crypto framework asynchronous hash api.
>> It is based on omap-sham.c driver.
>> S5P has some HW differencies and is not implemented.
>>
>> Modifications in s5p-sss:
>>[...]
>> +/*
>> + * HASH bit numbers, used by device, setting in dev->hash_flags with
>> + * functions set_bit(), clear_bit() or tested with test_bit() or BIT(),
>> + * to keep HASH state BUSY or FREE, or to signal state from irq_handler
>> + * to hash_tasklet. SGS keep track of allocated memory for scatterlist
>> + */
>> +#define HASH_FLAGS_BUSY 0
>> +#define HASH_FLAGS_FINAL1
>> +#define HASH_FLAGS_DMA_ACTIVE 2
>> +#define HASH_FLAGS_OUTPUT_READY 3
>> +#define HASH_FLAGS_DMA_READY4
>> +#define HASH_FLAGS_SGS_COPIED 5
>> +#define HASH_FLAGS_SGS_ALLOCED 6
>> +
>> +/*
>> + * HASH bit numbers used in request context
>> + * FINUP mark last hash operation
>> + */
>> +#define HASH_FLAGS_FINUP7
>> +#define HASH_FLAGS_ERROR8
>
> I spent some time on s5p_hash_finish_req() and other code around flags,
> confused by two different flags (ctx->flags, device->hash_flags) and
> different API used to play with them next to each other (once test_bit,
> line later just |=).
>
> This is just confusing. AFAIU, you use only two bits in ctx->flags, so
> just convert it to two bools. This will remove the confuse:
> 1. between the defines before and here,
> 2. around mixing xxx_bit() and regular |= operations.
>
Good point, I will rewrite them into two bool vars.
>> +
>> +/* HASH op codes */
>> +#define HASH_OP_UPDATE 1
>> +#define HASH_OP_FINAL 2
>> +
>> +/* HASH HW constants */
>> +#define BUFLEN HASH_BLOCK_SIZE
>> +
>> +#define SSS_DMA_ALIGN 16
>> +#define SSS_ALIGNED __attribute__((aligned(SSS_DMA_ALIGN)))
>> +#define SSS_DMA_ALIGN_MASK (SSS_DMA_ALIGN - 1)
>
> No changes here... I asked for making this consistent with current code
> so please bring a patch which introduces new macro to existing code and
> then re-use it for new code.
>
> Dropping inconsistent code and then promising "I will fix it up later"
> does not work.
This align stuff was again taken from omap driver. AES code does not need
any DMA_ALIGN, it sets ".cra_align = 0x0f", but it is not needed, it can be
simple zero 0x00, as AES operate on blocks of 64 bytes long, and SecSS DMA
engine can operate on non-aligned addresses. I am not sure how CPU will
handle HASH corner case at last update, when input data will point to last
byte of page, length will be 1, and FeedControl will round up length to 8,
so DMA will try (?) reading bytes after page.
To sum it up, AES code needs cleanup ".cra_align = 0", but I do not think
it is so critical to make it before HASH.
You have good point here, as the names are confusing, so I will remove
SSS_ALIGNED from HASH struct definitions, and change SSS_DMA_ALIGN into
SSS_HASH_DMA_LEN_ALIGN, and SSS_DMA_ALIGN_MASK into SSS_HASH_DMA_ALIGN_MASK.
>> [...]
>> * @lock: Lock for protecting both access to device hardware registers
>> - * and fields related to current request (including the busy
>> field).
>> + * and fields related to current request (including the busy
>> + * field).
>
> Why wrapping this line?
Sorry, I will remove this cleanup (line is 81 bytes long).
>> + * @res:Resources for hash.
>> + * @io_hash_base: Per-variant offset for HASH block IO memory.
>> + * @hash_lock: Lock for protecting hash_req, hash_queue and hash_flags
>> + * variable.
>> + * @hash_tasklet: New HASH request scheduling job.
>> + * @xmit_buf: Buffer for current HASH request transfer into SSS block.
>> + * @hash_flags: Flags for current HASH op.
>> + * @hash_queue: Async hash queue.
>> + * @hash_req: Current request sending to SSS HASH block.
>> + * @hash_sg_iter: Scatterlist transferred through DMA into SSS HASH block.
>> + * @hash_sg_cnt: Counter for hash_sg_iter.
>> + *
>> +/**
>> + * s5p_hash_rx - get next hash_sg_iter
>> + * @dev:device
>> + *
>> + * Return:
>> + * 2if there is no more data and it is UPDATE op
>> + * 1if new receiving (input) data is ready and can be written to
>> + * device
>
> Why wrapping so early?
OK, I will reformat comments up to 80 bytes per line, this one and following
ones.
>>[...]
>> +
>> +if (final) {
>> +/* number of bytes for last part */
>> +low = length; high = 0;
>
> No multiple assignments in one line.
>
>> +/* total number of bits prev hashed */
>> +tmplen = ctx->digcnt * 8;
>> +prelow = (u32)tmplen;
>> +prehigh = (u32)(tmplen >> 32);
>> +} else {
>> +prelow = 0; prehigh = 0;
>> +low = 0; high = BIT(31);
>
> No multiple ass
Re: [PATCH v3] crypto: s5p-sss: Add HASH support for Exynos
On Wed, Sep 27, 2017 at 02:25:50PM +0200, Kamil Konieczny wrote: > Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW. > It uses the crypto framework asynchronous hash api. > It is based on omap-sham.c driver. > S5P has some HW differencies and is not implemented. > > Modifications in s5p-sss: > > - Add hash supporting structures and functions. > > - Modify irq handler to handle both aes and hash signals. > > - Resize resource end in probe if EXYNOS_HASH is enabled in > Kconfig. > > - Add new copyright line and new author. > > - Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6 > with crypto run-time self test testmgr > and with tcrypt module with: modprobe tcrypt sec=1 mode=N > where N=402, 403, 404 (MD5, SHA1, SHA256). > > Modifications in drivers/crypto/Kconfig: > > - Add new CRYPTO_DEV_EXYNOS_HASH, depend on !EXYNOS_RNG > and CRYPTO_DEV_S5P > > - Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH > as they are nedded for fallback. > > Signed-off-by: Kamil Konieczny > --- > version 3: > - many fixes suggested by Krzysztof Kozlowski: comments, uppercases in const, > remove unused defines, remove unused variable bs, constify aes_variant, > remove global var use_hash, remove WARN_ON, improve hash_import(), > change goto label into 'out' in s5p_hash_handle_queue(), reorder variable > declarations, add spinlock to protect clearing HASH_FLAGS_BUSY > - simplify code: replace one-line functions s5p_hash_update_req(), > s5p_hash_final_req() with call to s5p_hash_xmit_dma(), and delete them > - replace call to s5p_hash_hw_init() into s5p_ahash_dma_init() and delete it > - fix clearing shash flag CRYPTO_TFM_REQ_MAY_SLEEP > - fix s5p_hash_set_flow() Thanks for the changes, looks better. > > version 2: > - change patch format so number of lines drops > - change in Kconfig as suggested by Krzysztof Kozlowski, add > EXYNOS_HASH subsection > - change #ifndef EXYNOS_RNG into #ifdef CRYPTO_DEV_EXYNOS_HASH > - remove style fixups in aes, as they should go in separate patch > - remove FLOW_LOG, FLOW_DUMP macros and its uses > - remove #if 0 ... endif > - remove unused function hash_wait and its defines > - fix compiler warning in dev_dbg > - remove some comments > - other minor fixes in comments > > drivers/crypto/Kconfig | 14 + > drivers/crypto/s5p-sss.c | 1507 > +- > 2 files changed, 1509 insertions(+), 12 deletions(-) > > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig > index fe33c199fc1a..01cf07ce34c5 100644 > --- a/drivers/crypto/Kconfig > +++ b/drivers/crypto/Kconfig > @@ -439,6 +439,20 @@ config CRYPTO_DEV_S5P > Select this to offload Samsung S5PV210 or S5PC110, Exynos from AES > algorithms execution. > > +config CRYPTO_DEV_EXYNOS_HASH > + bool "Support for Samsung Exynos HASH accelerator" > + depends on CRYPTO_DEV_S5P > + depends on !CRYPTO_DEV_EXYNOS_RNG && CRYPTO_DEV_EXYNOS_RNG!=m > + select CRYPTO_SHA1 > + select CRYPTO_MD5 > + select CRYPTO_SHA256 > + help > + Select this to offload Exynos from HASH MD5/SHA1/SHA256. > + This will select software SHA1, MD5 and SHA256 as they are > + needed for small and zero-size messages. > + HASH algorithms will be disabled if EXYNOS_RNG > + is enabled due to hw conflict. > + > config CRYPTO_DEV_NX > bool "Support for IBM PowerPC Nest (NX) cryptographic acceleration" > depends on PPC64 > diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c > index 7ac657f46d15..e801ec4bfd8e 100644 > --- a/drivers/crypto/s5p-sss.c > +++ b/drivers/crypto/s5p-sss.c > @@ -1,18 +1,21 @@ > /* > * Cryptographic API. > * > - * Support for Samsung S5PV210 HW acceleration. > + * Support for Samsung S5PV210 and Exynos HW acceleration. > * > * Copyright (C) 2011 NetUP Inc. All rights reserved. > + * Copyright (c) 2017 Samsung Electronics Co., Ltd. All rights reserved. > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > published > * by the Free Software Foundation. > * > + * Hash part based on omap-sham.c driver. > */ > > #include > #include > +#include > #include > #include > #include > @@ -30,28 +33,41 @@ > #include > #include > > +#include > +#include > +#include > +#include > + > #define _SBF(s, v) ((v) << (s)) > > /* Feed control registers */ > #define SSS_REG_FCINTSTAT 0x > +#define SSS_FCINTSTAT_HPARTINT BIT(7) > +#define SSS_FCINTSTAT_HDONEINT BIT(5) > #define SSS_FCINTSTAT_BRDMAINT BIT(3) > #define SSS_FCINTSTAT_BTDMAINT BIT(2) > #define SSS_FCINTSTAT_HRDMAINT BIT(1) > #define SSS_FCINTSTAT_PKDMAINT BIT(0) > > #define SSS_REG_FCINTENSET 0x0004 > +#define SSS_FCINTENSET_HPARTINTENSET BIT(7) > +#defi
[PATCH v3] crypto: s5p-sss: Add HASH support for Exynos
Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW. It uses the crypto framework asynchronous hash api. It is based on omap-sham.c driver. S5P has some HW differencies and is not implemented. Modifications in s5p-sss: - Add hash supporting structures and functions. - Modify irq handler to handle both aes and hash signals. - Resize resource end in probe if EXYNOS_HASH is enabled in Kconfig. - Add new copyright line and new author. - Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6 with crypto run-time self test testmgr and with tcrypt module with: modprobe tcrypt sec=1 mode=N where N=402, 403, 404 (MD5, SHA1, SHA256). Modifications in drivers/crypto/Kconfig: - Add new CRYPTO_DEV_EXYNOS_HASH, depend on !EXYNOS_RNG and CRYPTO_DEV_S5P - Select sw algorithms MD5, SHA1 and SHA256 in EXYNOS_HASH as they are nedded for fallback. Signed-off-by: Kamil Konieczny --- version 3: - many fixes suggested by Krzysztof Kozlowski: comments, uppercases in const, remove unused defines, remove unused variable bs, constify aes_variant, remove global var use_hash, remove WARN_ON, improve hash_import(), change goto label into 'out' in s5p_hash_handle_queue(), reorder variable declarations, add spinlock to protect clearing HASH_FLAGS_BUSY - simplify code: replace one-line functions s5p_hash_update_req(), s5p_hash_final_req() with call to s5p_hash_xmit_dma(), and delete them - replace call to s5p_hash_hw_init() into s5p_ahash_dma_init() and delete it - fix clearing shash flag CRYPTO_TFM_REQ_MAY_SLEEP - fix s5p_hash_set_flow() version 2: - change patch format so number of lines drops - change in Kconfig as suggested by Krzysztof Kozlowski, add EXYNOS_HASH subsection - change #ifndef EXYNOS_RNG into #ifdef CRYPTO_DEV_EXYNOS_HASH - remove style fixups in aes, as they should go in separate patch - remove FLOW_LOG, FLOW_DUMP macros and its uses - remove #if 0 ... endif - remove unused function hash_wait and its defines - fix compiler warning in dev_dbg - remove some comments - other minor fixes in comments drivers/crypto/Kconfig | 14 + drivers/crypto/s5p-sss.c | 1507 +- 2 files changed, 1509 insertions(+), 12 deletions(-) diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index fe33c199fc1a..01cf07ce34c5 100644 --- a/drivers/crypto/Kconfig +++ b/drivers/crypto/Kconfig @@ -439,6 +439,20 @@ config CRYPTO_DEV_S5P Select this to offload Samsung S5PV210 or S5PC110, Exynos from AES algorithms execution. +config CRYPTO_DEV_EXYNOS_HASH + bool "Support for Samsung Exynos HASH accelerator" + depends on CRYPTO_DEV_S5P + depends on !CRYPTO_DEV_EXYNOS_RNG && CRYPTO_DEV_EXYNOS_RNG!=m + select CRYPTO_SHA1 + select CRYPTO_MD5 + select CRYPTO_SHA256 + help + Select this to offload Exynos from HASH MD5/SHA1/SHA256. + This will select software SHA1, MD5 and SHA256 as they are + needed for small and zero-size messages. + HASH algorithms will be disabled if EXYNOS_RNG + is enabled due to hw conflict. + config CRYPTO_DEV_NX bool "Support for IBM PowerPC Nest (NX) cryptographic acceleration" depends on PPC64 diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c index 7ac657f46d15..e801ec4bfd8e 100644 --- a/drivers/crypto/s5p-sss.c +++ b/drivers/crypto/s5p-sss.c @@ -1,18 +1,21 @@ /* * Cryptographic API. * - * Support for Samsung S5PV210 HW acceleration. + * Support for Samsung S5PV210 and Exynos HW acceleration. * * Copyright (C) 2011 NetUP Inc. All rights reserved. + * Copyright (c) 2017 Samsung Electronics Co., Ltd. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as published * by the Free Software Foundation. * + * Hash part based on omap-sham.c driver. */ #include #include +#include #include #include #include @@ -30,28 +33,41 @@ #include #include +#include +#include +#include +#include + #define _SBF(s, v) ((v) << (s)) /* Feed control registers */ #define SSS_REG_FCINTSTAT 0x +#define SSS_FCINTSTAT_HPARTINT BIT(7) +#define SSS_FCINTSTAT_HDONEINT BIT(5) #define SSS_FCINTSTAT_BRDMAINT BIT(3) #define SSS_FCINTSTAT_BTDMAINT BIT(2) #define SSS_FCINTSTAT_HRDMAINT BIT(1) #define SSS_FCINTSTAT_PKDMAINT BIT(0) #define SSS_REG_FCINTENSET 0x0004 +#define SSS_FCINTENSET_HPARTINTENSET BIT(7) +#define SSS_FCINTENSET_HDONEINTENSET BIT(5) #define SSS_FCINTENSET_BRDMAINTENSETBIT(3) #define SSS_FCINTENSET_BTDMAINTENSETBIT(2) #define SSS_FCINTENSET_HRDMAINTENSETBIT(1) #define SSS_FCINTENSET_PKDMAINTENSETBIT(0) #define SSS_REG_FCINTENCLR 0x0008 +#define SSS_FCINTENCLR_HPARTINTENCLR BIT(7) +#define SSS_FCINTENCLR_HDO
