2024-01-16 09:47:01

by Gaurav Jain

[permalink] [raw]
Subject: [PATCH] crypto: caam/hash - fix asynchronous hash

ahash_alg->setkey is updated to ahash_nosetkey in ahash.c
so updating the handling of setkey in caam driver.

Fixes: 2f1f34c1bf7b ("crypto: ahash - optimize performance when wrapping shash")
Signed-off-by: Gaurav Jain <[email protected]>
---
drivers/crypto/caam/caamalg_qi2.c | 4 ++--
drivers/crypto/caam/caamhash.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/caam/caamalg_qi2.c b/drivers/crypto/caam/caamalg_qi2.c
index a148ff1f0872..93a400e286b4 100644
--- a/drivers/crypto/caam/caamalg_qi2.c
+++ b/drivers/crypto/caam/caamalg_qi2.c
@@ -4571,7 +4571,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)

ctx->dev = caam_hash->dev;

- if (alg->setkey) {
+ if (crypto_hash_alg_has_setkey(halg)) {
ctx->adata.key_dma = dma_map_single_attrs(ctx->dev, ctx->key,
ARRAY_SIZE(ctx->key),
DMA_TO_DEVICE,
@@ -4611,7 +4611,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
* For keyed hash algorithms shared descriptors
* will be created later in setkey() callback
*/
- return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
+ return crypto_hash_alg_has_setkey(halg) ? 0 : ahash_set_sh_desc(ahash);
}

static void caam_hash_cra_exit(struct crypto_tfm *tfm)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 290c8500c247..4d50356b593c 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1804,7 +1804,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
} else {
if (priv->era >= 6) {
ctx->dir = DMA_BIDIRECTIONAL;
- ctx->key_dir = alg->setkey ? DMA_TO_DEVICE : DMA_NONE;
+ ctx->key_dir = crypto_hash_alg_has_setkey(halg) ? DMA_TO_DEVICE : DMA_NONE;
} else {
ctx->dir = DMA_TO_DEVICE;
ctx->key_dir = DMA_NONE;
@@ -1862,7 +1862,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
* For keyed hash algorithms shared descriptors
* will be created later in setkey() callback
*/
- return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
+ return crypto_hash_alg_has_setkey(halg) ? 0 : ahash_set_sh_desc(ahash);
}

static void caam_hash_cra_exit(struct crypto_tfm *tfm)
--
2.25.1



2024-01-17 04:33:17

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH] crypto: caam/hash - fix asynchronous hash

On Tue, Jan 16, 2024 at 03:14:05PM +0530, Gaurav Jain wrote:
> ahash_alg->setkey is updated to ahash_nosetkey in ahash.c
> so updating the handling of setkey in caam driver.
>
> Fixes: 2f1f34c1bf7b ("crypto: ahash - optimize performance when wrapping shash")
> Signed-off-by: Gaurav Jain <[email protected]>
> ---
> drivers/crypto/caam/caamalg_qi2.c | 4 ++--
> drivers/crypto/caam/caamhash.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg_qi2.c b/drivers/crypto/caam/caamalg_qi2.c
> index a148ff1f0872..93a400e286b4 100644
> --- a/drivers/crypto/caam/caamalg_qi2.c
> +++ b/drivers/crypto/caam/caamalg_qi2.c
> @@ -4571,7 +4571,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
>
> ctx->dev = caam_hash->dev;
>
> - if (alg->setkey) {
> + if (crypto_hash_alg_has_setkey(halg)) {
> ctx->adata.key_dma = dma_map_single_attrs(ctx->dev, ctx->key,
> ARRAY_SIZE(ctx->key),
> DMA_TO_DEVICE,
> @@ -4611,7 +4611,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
> * For keyed hash algorithms shared descriptors
> * will be created later in setkey() callback
> */
> - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> + return crypto_hash_alg_has_setkey(halg) ? 0 : ahash_set_sh_desc(ahash);
> }
>
> static void caam_hash_cra_exit(struct crypto_tfm *tfm)
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> index 290c8500c247..4d50356b593c 100644
> --- a/drivers/crypto/caam/caamhash.c
> +++ b/drivers/crypto/caam/caamhash.c
> @@ -1804,7 +1804,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
> } else {
> if (priv->era >= 6) {
> ctx->dir = DMA_BIDIRECTIONAL;
> - ctx->key_dir = alg->setkey ? DMA_TO_DEVICE : DMA_NONE;
> + ctx->key_dir = crypto_hash_alg_has_setkey(halg) ? DMA_TO_DEVICE : DMA_NONE;
> } else {
> ctx->dir = DMA_TO_DEVICE;
> ctx->key_dir = DMA_NONE;
> @@ -1862,7 +1862,7 @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
> * For keyed hash algorithms shared descriptors
> * will be created later in setkey() callback
> */
> - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> + return crypto_hash_alg_has_setkey(halg) ? 0 : ahash_set_sh_desc(ahash);
> }
>

Thanks. Did you also consider putting something in struct caam_hash_alg (the
struct in which this driver embeds its ahash_alg structure) that indicates
whether the algorithm is an HMAC or not? Other drivers use that solution.

- Eric

2024-01-17 10:48:51

by Gaurav Jain

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH] crypto: caam/hash - fix asynchronous hash



> -----Original Message-----
> From: Eric Biggers <[email protected]>
> Sent: Wednesday, January 17, 2024 10:03 AM
> To: Gaurav Jain <[email protected]>
> Cc: Herbert Xu <[email protected]>; David S . Miller
> <[email protected]>; Horia Geanta <[email protected]>; Pankaj
> Gupta <[email protected]>; Varun Sethi <[email protected]>; Meenakshi
> Aggarwal <[email protected]>; Aisheng Dong
> <[email protected]>; Silvano Di Ninno <[email protected]>; linux-
> [email protected]; [email protected]; dl-linux-imx <linux-
> [email protected]>
> Subject: [EXT] Re: [PATCH] crypto: caam/hash - fix asynchronous hash
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> On Tue, Jan 16, 2024 at 03:14:05PM +0530, Gaurav Jain wrote:
> > ahash_alg->setkey is updated to ahash_nosetkey in ahash.c so updating
> > the handling of setkey in caam driver.
> >
> > Fixes: 2f1f34c1bf7b ("crypto: ahash - optimize performance when
> > wrapping shash")
> > Signed-off-by: Gaurav Jain <[email protected]>
> > ---
> > drivers/crypto/caam/caamalg_qi2.c | 4 ++--
> > drivers/crypto/caam/caamhash.c | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/crypto/caam/caamalg_qi2.c
> > b/drivers/crypto/caam/caamalg_qi2.c
> > index a148ff1f0872..93a400e286b4 100644
> > --- a/drivers/crypto/caam/caamalg_qi2.c
> > +++ b/drivers/crypto/caam/caamalg_qi2.c
> > @@ -4571,7 +4571,7 @@ static int caam_hash_cra_init(struct crypto_tfm
> > *tfm)
> >
> > ctx->dev = caam_hash->dev;
> >
> > - if (alg->setkey) {
> > + if (crypto_hash_alg_has_setkey(halg)) {
> > ctx->adata.key_dma = dma_map_single_attrs(ctx->dev, ctx->key,
> > ARRAY_SIZE(ctx->key),
> > DMA_TO_DEVICE,
> > @@ -4611,7 +4611,7 @@ static int caam_hash_cra_init(struct crypto_tfm
> *tfm)
> > * For keyed hash algorithms shared descriptors
> > * will be created later in setkey() callback
> > */
> > - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> > + return crypto_hash_alg_has_setkey(halg) ? 0 :
> > + ahash_set_sh_desc(ahash);
> > }
> >
> > static void caam_hash_cra_exit(struct crypto_tfm *tfm) diff --git
> > a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> > index 290c8500c247..4d50356b593c 100644
> > --- a/drivers/crypto/caam/caamhash.c
> > +++ b/drivers/crypto/caam/caamhash.c
> > @@ -1804,7 +1804,7 @@ static int caam_hash_cra_init(struct crypto_tfm
> *tfm)
> > } else {
> > if (priv->era >= 6) {
> > ctx->dir = DMA_BIDIRECTIONAL;
> > - ctx->key_dir = alg->setkey ? DMA_TO_DEVICE : DMA_NONE;
> > + ctx->key_dir = crypto_hash_alg_has_setkey(halg)
> > + ? DMA_TO_DEVICE : DMA_NONE;
> > } else {
> > ctx->dir = DMA_TO_DEVICE;
> > ctx->key_dir = DMA_NONE; @@ -1862,7 +1862,7 @@
> > static int caam_hash_cra_init(struct crypto_tfm *tfm)
> > * For keyed hash algorithms shared descriptors
> > * will be created later in setkey() callback
> > */
> > - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> > + return crypto_hash_alg_has_setkey(halg) ? 0 :
> > + ahash_set_sh_desc(ahash);
> > }
> >
>
> Thanks. Did you also consider putting something in struct caam_hash_alg (the
> struct in which this driver embeds its ahash_alg structure) that indicates whether
> the algorithm is an HMAC or not? Other drivers use that solution.

Crypto/ahash.c has this API to check the setkey so I used to differentiate between HMAC & only hash.
Let me know if this change is not sufficient, will add the flag in caam_hash_alg.

Gaurav
>
> - Eric

2024-01-18 03:43:57

by Eric Biggers

[permalink] [raw]
Subject: Re: [EXT] Re: [PATCH] crypto: caam/hash - fix asynchronous hash

On Wed, Jan 17, 2024 at 10:48:39AM +0000, Gaurav Jain wrote:
>
>
> > -----Original Message-----
> > From: Eric Biggers <[email protected]>
> > Sent: Wednesday, January 17, 2024 10:03 AM
> > To: Gaurav Jain <[email protected]>
> > Cc: Herbert Xu <[email protected]>; David S . Miller
> > <[email protected]>; Horia Geanta <[email protected]>; Pankaj
> > Gupta <[email protected]>; Varun Sethi <[email protected]>; Meenakshi
> > Aggarwal <[email protected]>; Aisheng Dong
> > <[email protected]>; Silvano Di Ninno <[email protected]>; linux-
> > [email protected]; [email protected]; dl-linux-imx <linux-
> > [email protected]>
> > Subject: [EXT] Re: [PATCH] crypto: caam/hash - fix asynchronous hash
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report this
> > email' button
> >
> >
> > On Tue, Jan 16, 2024 at 03:14:05PM +0530, Gaurav Jain wrote:
> > > ahash_alg->setkey is updated to ahash_nosetkey in ahash.c so updating
> > > the handling of setkey in caam driver.
> > >
> > > Fixes: 2f1f34c1bf7b ("crypto: ahash - optimize performance when
> > > wrapping shash")
> > > Signed-off-by: Gaurav Jain <[email protected]>
> > > ---
> > > drivers/crypto/caam/caamalg_qi2.c | 4 ++--
> > > drivers/crypto/caam/caamhash.c | 4 ++--
> > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/crypto/caam/caamalg_qi2.c
> > > b/drivers/crypto/caam/caamalg_qi2.c
> > > index a148ff1f0872..93a400e286b4 100644
> > > --- a/drivers/crypto/caam/caamalg_qi2.c
> > > +++ b/drivers/crypto/caam/caamalg_qi2.c
> > > @@ -4571,7 +4571,7 @@ static int caam_hash_cra_init(struct crypto_tfm
> > > *tfm)
> > >
> > > ctx->dev = caam_hash->dev;
> > >
> > > - if (alg->setkey) {
> > > + if (crypto_hash_alg_has_setkey(halg)) {
> > > ctx->adata.key_dma = dma_map_single_attrs(ctx->dev, ctx->key,
> > > ARRAY_SIZE(ctx->key),
> > > DMA_TO_DEVICE,
> > > @@ -4611,7 +4611,7 @@ static int caam_hash_cra_init(struct crypto_tfm
> > *tfm)
> > > * For keyed hash algorithms shared descriptors
> > > * will be created later in setkey() callback
> > > */
> > > - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> > > + return crypto_hash_alg_has_setkey(halg) ? 0 :
> > > + ahash_set_sh_desc(ahash);
> > > }
> > >
> > > static void caam_hash_cra_exit(struct crypto_tfm *tfm) diff --git
> > > a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> > > index 290c8500c247..4d50356b593c 100644
> > > --- a/drivers/crypto/caam/caamhash.c
> > > +++ b/drivers/crypto/caam/caamhash.c
> > > @@ -1804,7 +1804,7 @@ static int caam_hash_cra_init(struct crypto_tfm
> > *tfm)
> > > } else {
> > > if (priv->era >= 6) {
> > > ctx->dir = DMA_BIDIRECTIONAL;
> > > - ctx->key_dir = alg->setkey ? DMA_TO_DEVICE : DMA_NONE;
> > > + ctx->key_dir = crypto_hash_alg_has_setkey(halg)
> > > + ? DMA_TO_DEVICE : DMA_NONE;
> > > } else {
> > > ctx->dir = DMA_TO_DEVICE;
> > > ctx->key_dir = DMA_NONE; @@ -1862,7 +1862,7 @@
> > > static int caam_hash_cra_init(struct crypto_tfm *tfm)
> > > * For keyed hash algorithms shared descriptors
> > > * will be created later in setkey() callback
> > > */
> > > - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> > > + return crypto_hash_alg_has_setkey(halg) ? 0 :
> > > + ahash_set_sh_desc(ahash);
> > > }
> > >
> >
> > Thanks. Did you also consider putting something in struct caam_hash_alg (the
> > struct in which this driver embeds its ahash_alg structure) that indicates whether
> > the algorithm is an HMAC or not? Other drivers use that solution.
>
> Crypto/ahash.c has this API to check the setkey so I used to differentiate between HMAC & only hash.
> Let me know if this change is not sufficient, will add the flag in caam_hash_alg.
>

Currently crypto_hash_alg_has_setkey() isn't used outside of crypto/ahash.c.
Since there's an alternative that matches what the other drivers do, I think I'd
prefer that you did that. We can then make crypto_hash_alg_has_setkey() a
static function, private to crypto/ahash.c.

- Eric

2024-01-18 09:28:48

by Gaurav Jain

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH] crypto: caam/hash - fix asynchronous hash



> -----Original Message-----
> From: Eric Biggers <[email protected]>
> Sent: Thursday, January 18, 2024 9:14 AM
> To: Gaurav Jain <[email protected]>
> Cc: Herbert Xu <[email protected]>; David S . Miller
> <[email protected]>; Horia Geanta <[email protected]>; Pankaj
> Gupta <[email protected]>; Varun Sethi <[email protected]>; Meenakshi
> Aggarwal <[email protected]>; Aisheng Dong
> <[email protected]>; Silvano Di Ninno <[email protected]>; linux-
> [email protected]; [email protected]; dl-linux-imx <linux-
> [email protected]>
> Subject: Re: [EXT] Re: [PATCH] crypto: caam/hash - fix asynchronous hash
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> On Wed, Jan 17, 2024 at 10:48:39AM +0000, Gaurav Jain wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eric Biggers <[email protected]>
> > > Sent: Wednesday, January 17, 2024 10:03 AM
> > > To: Gaurav Jain <[email protected]>
> > > Cc: Herbert Xu <[email protected]>; David S . Miller
> > > <[email protected]>; Horia Geanta <[email protected]>; Pankaj
> > > Gupta <[email protected]>; Varun Sethi <[email protected]>;
> > > Meenakshi Aggarwal <[email protected]>; Aisheng Dong
> > > <[email protected]>; Silvano Di Ninno <[email protected]>;
> > > linux- [email protected]; [email protected];
> > > dl-linux-imx <linux- [email protected]>
> > > Subject: [EXT] Re: [PATCH] crypto: caam/hash - fix asynchronous hash
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments. When in doubt, report the message
> > > using the 'Report this email' button
> > >
> > >
> > > On Tue, Jan 16, 2024 at 03:14:05PM +0530, Gaurav Jain wrote:
> > > > ahash_alg->setkey is updated to ahash_nosetkey in ahash.c so
> > > > updating the handling of setkey in caam driver.
> > > >
> > > > Fixes: 2f1f34c1bf7b ("crypto: ahash - optimize performance when
> > > > wrapping shash")
> > > > Signed-off-by: Gaurav Jain <[email protected]>
> > > > ---
> > > > drivers/crypto/caam/caamalg_qi2.c | 4 ++--
> > > > drivers/crypto/caam/caamhash.c | 4 ++--
> > > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/crypto/caam/caamalg_qi2.c
> > > > b/drivers/crypto/caam/caamalg_qi2.c
> > > > index a148ff1f0872..93a400e286b4 100644
> > > > --- a/drivers/crypto/caam/caamalg_qi2.c
> > > > +++ b/drivers/crypto/caam/caamalg_qi2.c
> > > > @@ -4571,7 +4571,7 @@ static int caam_hash_cra_init(struct
> > > > crypto_tfm
> > > > *tfm)
> > > >
> > > > ctx->dev = caam_hash->dev;
> > > >
> > > > - if (alg->setkey) {
> > > > + if (crypto_hash_alg_has_setkey(halg)) {
> > > > ctx->adata.key_dma = dma_map_single_attrs(ctx->dev, ctx->key,
> > > > ARRAY_SIZE(ctx->key),
> > > >
> > > > DMA_TO_DEVICE, @@ -4611,7 +4611,7 @@ static int
> > > > caam_hash_cra_init(struct crypto_tfm
> > > *tfm)
> > > > * For keyed hash algorithms shared descriptors
> > > > * will be created later in setkey() callback
> > > > */
> > > > - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> > > > + return crypto_hash_alg_has_setkey(halg) ? 0 :
> > > > + ahash_set_sh_desc(ahash);
> > > > }
> > > >
> > > > static void caam_hash_cra_exit(struct crypto_tfm *tfm) diff --git
> > > > a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> > > > index 290c8500c247..4d50356b593c 100644
> > > > --- a/drivers/crypto/caam/caamhash.c
> > > > +++ b/drivers/crypto/caam/caamhash.c
> > > > @@ -1804,7 +1804,7 @@ static int caam_hash_cra_init(struct
> > > > crypto_tfm
> > > *tfm)
> > > > } else {
> > > > if (priv->era >= 6) {
> > > > ctx->dir = DMA_BIDIRECTIONAL;
> > > > - ctx->key_dir = alg->setkey ? DMA_TO_DEVICE : DMA_NONE;
> > > > + ctx->key_dir =
> > > > + crypto_hash_alg_has_setkey(halg) ? DMA_TO_DEVICE : DMA_NONE;
> > > > } else {
> > > > ctx->dir = DMA_TO_DEVICE;
> > > > ctx->key_dir = DMA_NONE; @@ -1862,7 +1862,7
> > > > @@ static int caam_hash_cra_init(struct crypto_tfm *tfm)
> > > > * For keyed hash algorithms shared descriptors
> > > > * will be created later in setkey() callback
> > > > */
> > > > - return alg->setkey ? 0 : ahash_set_sh_desc(ahash);
> > > > + return crypto_hash_alg_has_setkey(halg) ? 0 :
> > > > + ahash_set_sh_desc(ahash);
> > > > }
> > > >
> > >
> > > Thanks. Did you also consider putting something in struct
> > > caam_hash_alg (the struct in which this driver embeds its ahash_alg
> > > structure) that indicates whether the algorithm is an HMAC or not? Other
> drivers use that solution.
> >
> > Crypto/ahash.c has this API to check the setkey so I used to differentiate
> between HMAC & only hash.
> > Let me know if this change is not sufficient, will add the flag in caam_hash_alg.
> >
>
> Currently crypto_hash_alg_has_setkey() isn't used outside of crypto/ahash.c.
> Since there's an alternative that matches what the other drivers do, I think I'd
> prefer that you did that. We can then make crypto_hash_alg_has_setkey() a
> static function, private to crypto/ahash.c.

Understood. Changes done in v2.

Thanks
Gaurav
>
> - Eric