[PATCH 1/4] crypto: sun8i-ce-cipher - fix error handling in sun8i_ce_cipher_prepare()

[PATCH 1/4] crypto: sun8i-ce-cipher - fix error handling in sun8i_ce_cipher_prepare()

[Ovidiu Panait]

Fix two DMA cleanup issues on the error path in sun8i_ce_cipher_prepare():

1] If dma_map_sg() fails for areq->dst, the device driver would try to free
   DMA memory it has not allocated in the first place. To fix this, on the
   "theend_sgs" error path, call dma unmap only if the corresponding dma
   map was successful.

2] If the dma_map_single() call for the IV fails, the device driver would
   try to free an invalid DMA memory address on the "theend_iv" path:
   ------------[ cut here ]------------
   DMA-API: sun8i-ce 1904000.crypto: device driver tries to free an invalid DMA memory address
   WARNING: CPU: 2 PID: 69 at kernel/dma/debug.c:968 check_unmap+0x123c/0x1b90
   Modules linked in: skcipher_example(O+)
   CPU: 2 UID: 0 PID: 69 Comm: 1904000.crypto- Tainted: G           O        6.15.0-rc3+ #24 PREEMPT
   Tainted: [O]=OOT_MODULE
   Hardware name: OrangePi Zero2 (DT)
   pc : check_unmap+0x123c/0x1b90
   lr : check_unmap+0x123c/0x1b90
   ...
   Call trace:
    check_unmap+0x123c/0x1b90 (P)
    debug_dma_unmap_page+0xac/0xc0
    dma_unmap_page_attrs+0x1f4/0x5fc
    sun8i_ce_cipher_do_one+0x1bd4/0x1f40
    crypto_pump_work+0x334/0x6e0
    kthread_worker_fn+0x21c/0x438
    kthread+0x374/0x664
    ret_from_fork+0x10/0x20
   ---[ end trace 0000000000000000 ]---

To fix this, check for !dma_mapping_error() before calling
dma_unmap_single() on the "theend_iv" path.

Fixes: 06f751b61329 ("crypto: allwinner - Add sun8i-ce Crypto Engine")
Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index 19b7fb4a93e8..05f67661553c 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -275,13 +275,16 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 	} else {
 		if (nr_sgs > 0)
 			dma_unmap_sg(ce->dev, areq->src, ns, DMA_TO_DEVICE);
-		dma_unmap_sg(ce->dev, areq->dst, nd, DMA_FROM_DEVICE);
+
+		if (nr_sgd > 0)
+			dma_unmap_sg(ce->dev, areq->dst, nd, DMA_FROM_DEVICE);
 	}
 
 theend_iv:
 	if (areq->iv && ivsize > 0) {
-		if (rctx->addr_iv)
+		if (!dma_mapping_error(ce->dev, rctx->addr_iv))
 			dma_unmap_single(ce->dev, rctx->addr_iv, rctx->ivlen, DMA_TO_DEVICE);
+
 		offset = areq->cryptlen - ivsize;
 		if (rctx->op_dir & CE_DECRYPTION) {
 			memcpy(areq->iv, chan->backup_iv, ivsize);
-- 
2.48.1

[PATCH 2/4] crypto: sun8i-ce-cipher - use crypto_skcipher_driver_name()

[Ovidiu Panait]

Use crypto_skcipher_driver_name() helper from <crypto/skcipher.h>, instead
of accessing struct crypto_alg directly.

Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index 05f67661553c..f03a8fa7bfa2 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
 #include <crypto/scatterwalk.h>
+#include <crypto/skcipher.h>
 #include <crypto/internal/des.h>
 #include <crypto/internal/skcipher.h>
 #include "sun8i-ce.h"
@@ -438,7 +439,7 @@ int sun8i_ce_cipher_init(struct crypto_tfm *tfm)
 				    crypto_skcipher_reqsize(op->fallback_tfm));
 
 	memcpy(algt->fbname,
-	       crypto_tfm_alg_driver_name(crypto_skcipher_tfm(op->fallback_tfm)),
+	       crypto_skcipher_driver_name(op->fallback_tfm),
 	       CRYPTO_MAX_ALG_NAME);
 
 	err = pm_runtime_get_sync(op->ce->dev);
-- 
2.48.1

[PATCH 3/4] crypto: sun8i-ce-cipher - use IS_ENABLED() checks for debugfs stats

[Ovidiu Panait]

Add IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG) checks before the
fallback counter updates to make sure the code is not included when
debugfs statistics support is not enabled.

Also, drop the existing ifdef guards, since 'struct sun8i_ce_alg_template'
is always defined, even with CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG disabled.

Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
---
 .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 46 ++++++++++++-------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index f03a8fa7bfa2..433cd18f0b5b 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -34,22 +34,30 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
 
 	if (sg_nents_for_len(areq->src, areq->cryptlen) > MAX_SG ||
 	    sg_nents_for_len(areq->dst, areq->cryptlen) > MAX_SG) {
-		algt->stat_fb_maxsg++;
+		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+			algt->stat_fb_maxsg++;
+
 		return true;
 	}
 
 	if (areq->cryptlen < crypto_skcipher_ivsize(tfm)) {
-		algt->stat_fb_leniv++;
+		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+			algt->stat_fb_leniv++;
+
 		return true;
 	}
 
 	if (areq->cryptlen == 0) {
-		algt->stat_fb_len0++;
+		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+			algt->stat_fb_len0++;
+
 		return true;
 	}
 
 	if (areq->cryptlen % 16) {
-		algt->stat_fb_mod16++;
+		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+			algt->stat_fb_mod16++;
+
 		return true;
 	}
 
@@ -57,12 +65,16 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
 	sg = areq->src;
 	while (sg) {
 		if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
-			algt->stat_fb_srcali++;
+			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+				algt->stat_fb_srcali++;
+
 			return true;
 		}
 		todo = min(len, sg->length);
 		if (todo % 4) {
-			algt->stat_fb_srclen++;
+			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+				algt->stat_fb_srclen++;
+
 			return true;
 		}
 		len -= todo;
@@ -73,12 +85,16 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
 	sg = areq->dst;
 	while (sg) {
 		if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
-			algt->stat_fb_dstali++;
+			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+				algt->stat_fb_dstali++;
+
 			return true;
 		}
 		todo = min(len, sg->length);
 		if (todo % 4) {
-			algt->stat_fb_dstlen++;
+			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+				algt->stat_fb_dstlen++;
+
 			return true;
 		}
 		len -= todo;
@@ -101,9 +117,7 @@ static int sun8i_ce_cipher_fallback(struct skcipher_request *areq)
 		algt = container_of(alg, struct sun8i_ce_alg_template,
 				    alg.skcipher.base);
 
-#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
 		algt->stat_fb++;
-#endif
 	}
 
 	skcipher_request_set_tfm(&rctx->fallback_req, op->fallback_tfm);
@@ -147,9 +161,8 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 		rctx->op_dir, areq->iv, crypto_skcipher_ivsize(tfm),
 		op->keylen);
 
-#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
-	algt->stat_req++;
-#endif
+	if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+		algt->stat_req++;
 
 	flow = rctx->flow;
 
@@ -438,9 +451,10 @@ int sun8i_ce_cipher_init(struct crypto_tfm *tfm)
 	crypto_skcipher_set_reqsize(sktfm, sizeof(struct sun8i_cipher_req_ctx) +
 				    crypto_skcipher_reqsize(op->fallback_tfm));
 
-	memcpy(algt->fbname,
-	       crypto_skcipher_driver_name(op->fallback_tfm),
-	       CRYPTO_MAX_ALG_NAME);
+	if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
+		memcpy(algt->fbname,
+		       crypto_skcipher_driver_name(op->fallback_tfm),
+		       CRYPTO_MAX_ALG_NAME);
 
 	err = pm_runtime_get_sync(op->ce->dev);
 	if (err < 0)
-- 
2.48.1

[PATCH 4/4] crypto: sun8i-ce-cipher - use pm_runtime_resume_and_get()

[Ovidiu Panait]

Replace pm_runtime_get_sync() usage with pm_runtime_resume_and_get() to
simplify error handling.

This is recommended in the documentation of pm_runtime_get_sync():
/**
 * pm_runtime_get_sync - Bump up usage counter of a device and resume it.
...
 * Consider using pm_runtime_resume_and_get() instead of it, especially
 * if its return value is checked by the caller, as this is likely to result
 * in cleaner code.
...
 */

Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index 433cd18f0b5b..471eec743f1c 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -456,13 +456,12 @@ int sun8i_ce_cipher_init(struct crypto_tfm *tfm)
 		       crypto_skcipher_driver_name(op->fallback_tfm),
 		       CRYPTO_MAX_ALG_NAME);
 
-	err = pm_runtime_get_sync(op->ce->dev);
+	err = pm_runtime_resume_and_get(op->ce->dev);
 	if (err < 0)
 		goto error_pm;
 
 	return 0;
 error_pm:
-	pm_runtime_put_noidle(op->ce->dev);
 	crypto_free_skcipher(op->fallback_tfm);
 	return err;
 }
-- 
2.48.1

Re: [PATCH 2/4] crypto: sun8i-ce-cipher - use crypto_skcipher_driver_name()

[Corentin Labbe]

Le Fri, Apr 25, 2025 at 03:45:15PM +0300, Ovidiu Panait a écrit :
> Use crypto_skcipher_driver_name() helper from <crypto/skcipher.h>, instead
> of accessing struct crypto_alg directly.
> 
> Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
> ---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> index 05f67661553c..f03a8fa7bfa2 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/pm_runtime.h>
>  #include <crypto/scatterwalk.h>
> +#include <crypto/skcipher.h>
>  #include <crypto/internal/des.h>
>  #include <crypto/internal/skcipher.h>
>  #include "sun8i-ce.h"
> @@ -438,7 +439,7 @@ int sun8i_ce_cipher_init(struct crypto_tfm *tfm)
>  				    crypto_skcipher_reqsize(op->fallback_tfm));
>  
>  	memcpy(algt->fbname,
> -	       crypto_tfm_alg_driver_name(crypto_skcipher_tfm(op->fallback_tfm)),
> +	       crypto_skcipher_driver_name(op->fallback_tfm),
>  	       CRYPTO_MAX_ALG_NAME);
>  
>  	err = pm_runtime_get_sync(op->ce->dev);
> -- 
> 2.48.1
> 

Acked-by: Corentin LABBE <clabbe.montjoie@gmail.com>
Tested-by: Corentin LABBE <clabbe.montjoie@gmail.com>

Thanks

Re: [PATCH 4/4] crypto: sun8i-ce-cipher - use pm_runtime_resume_and_get()

[Corentin Labbe]

Le Fri, Apr 25, 2025 at 03:45:17PM +0300, Ovidiu Panait a écrit :
> Replace pm_runtime_get_sync() usage with pm_runtime_resume_and_get() to
> simplify error handling.
> 
> This is recommended in the documentation of pm_runtime_get_sync():
> /**
>  * pm_runtime_get_sync - Bump up usage counter of a device and resume it.
> ...
>  * Consider using pm_runtime_resume_and_get() instead of it, especially
>  * if its return value is checked by the caller, as this is likely to result
>  * in cleaner code.
> ...
>  */
> 

Tested-by: Corentin LABBE <clabbe.montjoie@gmail.com>
Acked-by: Corentin LABBE <clabbe.montjoie@gmail.com>

Thanks
Regards

Re: [PATCH 3/4] crypto: sun8i-ce-cipher - use IS_ENABLED() checks for debugfs stats

[Corentin Labbe]

Le Fri, Apr 25, 2025 at 03:45:16PM +0300, Ovidiu Panait a écrit :
> Add IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG) checks before the
> fallback counter updates to make sure the code is not included when
> debugfs statistics support is not enabled.
> 
> Also, drop the existing ifdef guards, since 'struct sun8i_ce_alg_template'
> is always defined, even with CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG disabled.
> 
> Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
> ---
>  .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 46 ++++++++++++-------
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> index f03a8fa7bfa2..433cd18f0b5b 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> @@ -34,22 +34,30 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
>  
>  	if (sg_nents_for_len(areq->src, areq->cryptlen) > MAX_SG ||
>  	    sg_nents_for_len(areq->dst, areq->cryptlen) > MAX_SG) {
> -		algt->stat_fb_maxsg++;
> +		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +			algt->stat_fb_maxsg++;
> +
>  		return true;
>  	}
>  
>  	if (areq->cryptlen < crypto_skcipher_ivsize(tfm)) {
> -		algt->stat_fb_leniv++;
> +		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +			algt->stat_fb_leniv++;
> +
>  		return true;
>  	}
>  
>  	if (areq->cryptlen == 0) {
> -		algt->stat_fb_len0++;
> +		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +			algt->stat_fb_len0++;
> +
>  		return true;
>  	}
>  
>  	if (areq->cryptlen % 16) {
> -		algt->stat_fb_mod16++;
> +		if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +			algt->stat_fb_mod16++;
> +
>  		return true;
>  	}
>  
> @@ -57,12 +65,16 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
>  	sg = areq->src;
>  	while (sg) {
>  		if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
> -			algt->stat_fb_srcali++;
> +			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +				algt->stat_fb_srcali++;
> +
>  			return true;
>  		}
>  		todo = min(len, sg->length);
>  		if (todo % 4) {
> -			algt->stat_fb_srclen++;
> +			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +				algt->stat_fb_srclen++;
> +
>  			return true;
>  		}
>  		len -= todo;
> @@ -73,12 +85,16 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
>  	sg = areq->dst;
>  	while (sg) {
>  		if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
> -			algt->stat_fb_dstali++;
> +			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +				algt->stat_fb_dstali++;
> +
>  			return true;
>  		}
>  		todo = min(len, sg->length);
>  		if (todo % 4) {
> -			algt->stat_fb_dstlen++;
> +			if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG))
> +				algt->stat_fb_dstlen++;
> +
>  			return true;
>  		}
>  		len -= todo;
> @@ -101,9 +117,7 @@ static int sun8i_ce_cipher_fallback(struct skcipher_request *areq)
>  		algt = container_of(alg, struct sun8i_ce_alg_template,
>  				    alg.skcipher.base);
>  
> -#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
>  		algt->stat_fb++;
> -#endif

Hello

You put IS_ENABLED everywhere, but here you remove it, why ?
I think you forgot it.

Thanks
Regards

Re: [PATCH 3/4] crypto: sun8i-ce-cipher - use IS_ENABLED() checks for debugfs stats

[Ovidiu Panait]

Hi,

On 4/25/25 4:30 PM, Corentin Labbe wrote:
> Le Fri, Apr 25, 2025 at 03:45:16PM +0300, Ovidiu Panait a écrit :
>> Add IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG) checks before the
>> fallback counter updates to make sure the code is not included when
>> debugfs statistics support is not enabled.
>>
>> Also, drop the existing ifdef guards, since 'struct sun8i_ce_alg_template'
>> is always defined, even with CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG disabled.
>>
>> Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
>> ---
>>  .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 46 ++++++++++++-------
>>  1 file changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> index f03a8fa7bfa2..433cd18f0b5b 100644
>> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> @@ -34,22 +34,30 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
>>  
...
>> @@ -101,9 +117,7 @@ static int sun8i_ce_cipher_fallback(struct skcipher_request *areq)
>>  		algt = container_of(alg, struct sun8i_ce_alg_template,
>>  				    alg.skcipher.base);
>>  
>> -#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
>>  		algt->stat_fb++;
>> -#endif
> 
> Hello
> 
> You put IS_ENABLED everywhere, but here you remove it, why ?
> I think you forgot it.
> 

This is already part of an IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG)
block, so no need to add an extra IS_ENABLED() check here. Just the
ifdef was dropped, as it was not really necessary.

Original code:

	if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG)) {
		struct skcipher_alg *alg = crypto_skcipher_alg(tfm);
		struct sun8i_ce_alg_template *algt __maybe_unused;

		algt = container_of(alg, struct sun8i_ce_alg_template,
				    alg.skcipher.base);

#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
		algt->stat_fb++;
#endif
	}

Thanks,
Ovidiu


> Thanks
> Regards
> 

Re: [PATCH 3/4] crypto: sun8i-ce-cipher - use IS_ENABLED() checks for debugfs stats

[Corentin Labbe]

Le Fri, Apr 25, 2025 at 04:47:35PM +0300, Ovidiu Panait a écrit :
> Hi,
> 
> On 4/25/25 4:30 PM, Corentin Labbe wrote:
> > Le Fri, Apr 25, 2025 at 03:45:16PM +0300, Ovidiu Panait a écrit :
> >> Add IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG) checks before the
> >> fallback counter updates to make sure the code is not included when
> >> debugfs statistics support is not enabled.
> >>
> >> Also, drop the existing ifdef guards, since 'struct sun8i_ce_alg_template'
> >> is always defined, even with CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG disabled.
> >>
> >> Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
> >> ---
> >>  .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 46 ++++++++++++-------
> >>  1 file changed, 30 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> >> index f03a8fa7bfa2..433cd18f0b5b 100644
> >> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> >> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> >> @@ -34,22 +34,30 @@ static int sun8i_ce_cipher_need_fallback(struct skcipher_request *areq)
> >>  
> ...
> >> @@ -101,9 +117,7 @@ static int sun8i_ce_cipher_fallback(struct skcipher_request *areq)
> >>  		algt = container_of(alg, struct sun8i_ce_alg_template,
> >>  				    alg.skcipher.base);
> >>  
> >> -#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
> >>  		algt->stat_fb++;
> >> -#endif
> > 
> > Hello
> > 
> > You put IS_ENABLED everywhere, but here you remove it, why ?
> > I think you forgot it.
> > 
> 
> This is already part of an IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG)
> block, so no need to add an extra IS_ENABLED() check here. Just the
> ifdef was dropped, as it was not really necessary.
> 
> Original code:
> 
> 	if (IS_ENABLED(CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG)) {
> 		struct skcipher_alg *alg = crypto_skcipher_alg(tfm);
> 		struct sun8i_ce_alg_template *algt __maybe_unused;
> 
> 		algt = container_of(alg, struct sun8i_ce_alg_template,
> 				    alg.skcipher.base);
> 
> #ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
> 		algt->stat_fb++;
> #endif
> 	}
> 

Oups sorry didnt check with enough diff context.

So:
Tested-by: Corentin LABBE <clabbe.montjoie@gmail.com>
Acked-by: Corentin LABBE <clabbe.montjoie@gmail.com>

Thanks
Regards

Re: [PATCH 2/4] crypto: sun8i-ce-cipher - use crypto_skcipher_driver_name()

[Herbert Xu]

On Fri, Apr 25, 2025 at 03:45:15PM +0300, Ovidiu Panait wrote:
> Use crypto_skcipher_driver_name() helper from <crypto/skcipher.h>, instead
> of accessing struct crypto_alg directly.
> 
> Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
> ---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> index 05f67661553c..f03a8fa7bfa2 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/pm_runtime.h>
>  #include <crypto/scatterwalk.h>
> +#include <crypto/skcipher.h>
>  #include <crypto/internal/des.h>
>  #include <crypto/internal/skcipher.h>

If you include internal/skcipher.h, there is no need to include
skcipher.h.

There is no need to resend this as I'll just remove this hunk when
applying the patch.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/4] crypto: sun8i-ce-cipher - fix error handling in sun8i_ce_cipher_prepare()

[Herbert Xu]

On Fri, Apr 25, 2025 at 03:45:14PM +0300, Ovidiu Panait wrote:
> Fix two DMA cleanup issues on the error path in sun8i_ce_cipher_prepare():
> 
> 1] If dma_map_sg() fails for areq->dst, the device driver would try to free
>    DMA memory it has not allocated in the first place. To fix this, on the
>    "theend_sgs" error path, call dma unmap only if the corresponding dma
>    map was successful.
> 
> 2] If the dma_map_single() call for the IV fails, the device driver would
>    try to free an invalid DMA memory address on the "theend_iv" path:
>    ------------[ cut here ]------------
>    DMA-API: sun8i-ce 1904000.crypto: device driver tries to free an invalid DMA memory address
>    WARNING: CPU: 2 PID: 69 at kernel/dma/debug.c:968 check_unmap+0x123c/0x1b90
>    Modules linked in: skcipher_example(O+)
>    CPU: 2 UID: 0 PID: 69 Comm: 1904000.crypto- Tainted: G           O        6.15.0-rc3+ #24 PREEMPT
>    Tainted: [O]=OOT_MODULE
>    Hardware name: OrangePi Zero2 (DT)
>    pc : check_unmap+0x123c/0x1b90
>    lr : check_unmap+0x123c/0x1b90
>    ...
>    Call trace:
>     check_unmap+0x123c/0x1b90 (P)
>     debug_dma_unmap_page+0xac/0xc0
>     dma_unmap_page_attrs+0x1f4/0x5fc
>     sun8i_ce_cipher_do_one+0x1bd4/0x1f40
>     crypto_pump_work+0x334/0x6e0
>     kthread_worker_fn+0x21c/0x438
>     kthread+0x374/0x664
>     ret_from_fork+0x10/0x20
>    ---[ end trace 0000000000000000 ]---
> 
> To fix this, check for !dma_mapping_error() before calling
> dma_unmap_single() on the "theend_iv" path.
> 
> Fixes: 06f751b61329 ("crypto: allwinner - Add sun8i-ce Crypto Engine")
> Signed-off-by: Ovidiu Panait <ovidiu.panait.oss@gmail.com>
> ---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt