[PATCH 0/9] crypto: qce - refactor the driver

[PATCH 0/9] crypto: qce - refactor the driver

[Bartosz Golaszewski]

This driver will soon be getting more features so show it some 
refactoring love in the meantime. Switching to using a workqueue and 
sleeping locks improves cryptsetup benchmark results for AES encryption.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (9):
      crypto: qce - fix goto jump in error path
      crypto: qce - unregister previously registered algos in error path
      crypto: qce - remove unneeded call to icc_set_bw() in error path
      crypto: qce - shrink code with devres clk helpers
      crypto: qce - convert qce_dma_request() to use devres
      crypto: qce - make qce_register_algs() a managed interface
      crypto: qce - use __free() for a buffer that's always freed
      crypto: qce - convert tasklet to workqueue
      crypto: qce - switch to using a mutex

 drivers/crypto/qce/core.c | 131 ++++++++++++++++------------------------------
 drivers/crypto/qce/core.h |   9 ++--
 drivers/crypto/qce/dma.c  |  22 ++++----
 drivers/crypto/qce/dma.h  |   3 +-
 drivers/crypto/qce/sha.c  |   6 +--
 5 files changed, 68 insertions(+), 103 deletions(-)
---
base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
change-id: 20241128-crypto-qce-refactor-ab58869eec34

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 1/9] crypto: qce - fix goto jump in error path

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

If qce_check_version() fails, we should jump to err_dma as we already
called qce_dma_request() a couple lines before.

Cc: stable@vger.kernel.org
Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index e228a31fe28dc..58ea93220f015 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -247,7 +247,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
 
 	ret = qce_check_version(qce);
 	if (ret)
-		goto err_clks;
+		goto err_dma;
 
 	spin_lock_init(&qce->lock);
 	tasklet_init(&qce->done_tasklet, qce_tasklet_req_done,

-- 
2.45.2

Re: [PATCH 1/9] crypto: qce - fix goto jump in error path

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> If qce_check_version() fails, we should jump to err_dma as we already
> called qce_dma_request() a couple lines before.
> 
> Cc: stable@vger.kernel.org
> Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index e228a31fe28dc..58ea93220f015 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -247,7 +247,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   
>   	ret = qce_check_version(qce);
>   	if (ret)
> -		goto err_clks;
> +		goto err_dma;
>   
>   	spin_lock_init(&qce->lock);
>   	tasklet_init(&qce->done_tasklet, qce_tasklet_req_done,
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 2/9] crypto: qce - unregister previously registered algos in error path

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

If we encounter an error when registering alorithms with the crypto
framework, we just bail out and don't unregister the ones we
successfully registered in prior iterations of the loop.

Add code that goes back over the algos and unregisters them before
returning an error from qce_register_algs().

Cc: stable@vger.kernel.org
Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index 58ea93220f015..848e5e802b92b 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -51,16 +51,19 @@ static void qce_unregister_algs(struct qce_device *qce)
 static int qce_register_algs(struct qce_device *qce)
 {
 	const struct qce_algo_ops *ops;
-	int i, ret = -ENODEV;
+	int i, j, ret = -ENODEV;
 
 	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
 		ops = qce_ops[i];
 		ret = ops->register_algs(qce);
-		if (ret)
-			break;
+		if (ret) {
+			for (j = i - 1; j >= 0; j--)
+				ops->unregister_algs(qce);
+			return ret;
+		}
 	}
 
-	return ret;
+	return 0;
 }
 
 static int qce_handle_request(struct crypto_async_request *async_req)

-- 
2.45.2

Re: [PATCH 2/9] crypto: qce - unregister previously registered algos in error path

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> If we encounter an error when registering alorithms with the crypto
> framework, we just bail out and don't unregister the ones we
> successfully registered in prior iterations of the loop.
> 
> Add code that goes back over the algos and unregisters them before
> returning an error from qce_register_algs().
> 
> Cc: stable@vger.kernel.org
> Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index 58ea93220f015..848e5e802b92b 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -51,16 +51,19 @@ static void qce_unregister_algs(struct qce_device *qce)
>   static int qce_register_algs(struct qce_device *qce)
>   {
>   	const struct qce_algo_ops *ops;
> -	int i, ret = -ENODEV;
> +	int i, j, ret = -ENODEV;
>   
>   	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
>   		ops = qce_ops[i];
>   		ret = ops->register_algs(qce);
> -		if (ret)
> -			break;
> +		if (ret) {
> +			for (j = i - 1; j >= 0; j--)
> +				ops->unregister_algs(qce);
> +			return ret;
> +		}
>   	}
>   
> -	return ret;
> +	return 0;
>   }
>   
>   static int qce_handle_request(struct crypto_async_request *async_req)
> 

Perhaps you can also use the devm trick here aswell ?

Neil

Re: [PATCH 2/9] crypto: qce - unregister previously registered algos in error path

[Bartosz Golaszewski]

On Tue, Dec 3, 2024 at 2:55 PM <neil.armstrong@linaro.org> wrote:
>
> On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > If we encounter an error when registering alorithms with the crypto
> > framework, we just bail out and don't unregister the ones we
> > successfully registered in prior iterations of the loop.
> >
> > Add code that goes back over the algos and unregisters them before
> > returning an error from qce_register_algs().
> >
> > Cc: stable@vger.kernel.org
> > Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >   drivers/crypto/qce/core.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> > index 58ea93220f015..848e5e802b92b 100644
> > --- a/drivers/crypto/qce/core.c
> > +++ b/drivers/crypto/qce/core.c
> > @@ -51,16 +51,19 @@ static void qce_unregister_algs(struct qce_device *qce)
> >   static int qce_register_algs(struct qce_device *qce)
> >   {
> >       const struct qce_algo_ops *ops;
> > -     int i, ret = -ENODEV;
> > +     int i, j, ret = -ENODEV;
> >
> >       for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
> >               ops = qce_ops[i];
> >               ret = ops->register_algs(qce);
> > -             if (ret)
> > -                     break;
> > +             if (ret) {
> > +                     for (j = i - 1; j >= 0; j--)
> > +                             ops->unregister_algs(qce);
> > +                     return ret;
> > +             }
> >       }
> >
> > -     return ret;
> > +     return 0;
> >   }
> >
> >   static int qce_handle_request(struct crypto_async_request *async_req)
> >
>
> Perhaps you can also use the devm trick here aswell ?
>

I wanted to keep this one brief for backporting but I also think that
scheduling a separate action here for every algo would be a bit
overkill. This is quite concise so I'd keep it this way.

Bart

Re: [PATCH 2/9] crypto: qce - unregister previously registered algos in error path

[neil.armstrong]

On 03/12/2024 16:05, Bartosz Golaszewski wrote:
> On Tue, Dec 3, 2024 at 2:55 PM <neil.armstrong@linaro.org> wrote:
>>
>> On 03/12/2024 10:19, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> If we encounter an error when registering alorithms with the crypto
>>> framework, we just bail out and don't unregister the ones we
>>> successfully registered in prior iterations of the loop.
>>>
>>> Add code that goes back over the algos and unregisters them before
>>> returning an error from qce_register_algs().
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> ---
>>>    drivers/crypto/qce/core.c | 11 +++++++----
>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
>>> index 58ea93220f015..848e5e802b92b 100644
>>> --- a/drivers/crypto/qce/core.c
>>> +++ b/drivers/crypto/qce/core.c
>>> @@ -51,16 +51,19 @@ static void qce_unregister_algs(struct qce_device *qce)
>>>    static int qce_register_algs(struct qce_device *qce)
>>>    {
>>>        const struct qce_algo_ops *ops;
>>> -     int i, ret = -ENODEV;
>>> +     int i, j, ret = -ENODEV;
>>>
>>>        for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
>>>                ops = qce_ops[i];
>>>                ret = ops->register_algs(qce);
>>> -             if (ret)
>>> -                     break;
>>> +             if (ret) {
>>> +                     for (j = i - 1; j >= 0; j--)
>>> +                             ops->unregister_algs(qce);
>>> +                     return ret;
>>> +             }
>>>        }
>>>
>>> -     return ret;
>>> +     return 0;
>>>    }
>>>
>>>    static int qce_handle_request(struct crypto_async_request *async_req)
>>>
>>
>> Perhaps you can also use the devm trick here aswell ?
>>
> 
> I wanted to keep this one brief for backporting but I also think that
> scheduling a separate action here for every algo would be a bit
> overkill. This is quite concise so I'd keep it this way.

Sure understandable!

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

> 
> Bart

[PATCH 3/9] crypto: qce - remove unneeded call to icc_set_bw() in error path

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's no need to call icc_set_bw(qce->mem_path, 0, 0); in error path
as this will already be done in the release path of devm_of_icc_get().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index 848e5e802b92b..f9ff1dfc1defe 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -234,7 +234,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
 
 	ret = clk_prepare_enable(qce->core);
 	if (ret)
-		goto err_mem_path_disable;
+		return ret;
 
 	ret = clk_prepare_enable(qce->iface);
 	if (ret)
@@ -274,8 +274,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	clk_disable_unprepare(qce->iface);
 err_clks_core:
 	clk_disable_unprepare(qce->core);
-err_mem_path_disable:
-	icc_set_bw(qce->mem_path, 0, 0);
 
 	return ret;
 }

-- 
2.45.2

Re: [PATCH 3/9] crypto: qce - remove unneeded call to icc_set_bw() in error path

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> There's no need to call icc_set_bw(qce->mem_path, 0, 0); in error path
> as this will already be done in the release path of devm_of_icc_get().
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index 848e5e802b92b..f9ff1dfc1defe 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -234,7 +234,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   
>   	ret = clk_prepare_enable(qce->core);
>   	if (ret)
> -		goto err_mem_path_disable;
> +		return ret;
>   
>   	ret = clk_prepare_enable(qce->iface);
>   	if (ret)
> @@ -274,8 +274,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	clk_disable_unprepare(qce->iface);
>   err_clks_core:
>   	clk_disable_unprepare(qce->core);
> -err_mem_path_disable:
> -	icc_set_bw(qce->mem_path, 0, 0);
>   
>   	return ret;
>   }
> 

Indeed, Good catch

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 4/9] crypto: qce - shrink code with devres clk helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
separately as well as putting the clocks in error path and the remove()
callback.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index f9ff1dfc1defe..cdcddf8f9f02b 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -212,15 +212,15 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	qce->core = devm_clk_get_optional(qce->dev, "core");
+	qce->core = devm_clk_get_optional_enabled(qce->dev, "core");
 	if (IS_ERR(qce->core))
 		return PTR_ERR(qce->core);
 
-	qce->iface = devm_clk_get_optional(qce->dev, "iface");
+	qce->iface = devm_clk_get_optional_enabled(qce->dev, "iface");
 	if (IS_ERR(qce->iface))
 		return PTR_ERR(qce->iface);
 
-	qce->bus = devm_clk_get_optional(qce->dev, "bus");
+	qce->bus = devm_clk_get_optional_enabled(qce->dev, "bus");
 	if (IS_ERR(qce->bus))
 		return PTR_ERR(qce->bus);
 
@@ -232,21 +232,9 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(qce->core);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(qce->iface);
-	if (ret)
-		goto err_clks_core;
-
-	ret = clk_prepare_enable(qce->bus);
-	if (ret)
-		goto err_clks_iface;
-
 	ret = qce_dma_request(qce->dev, &qce->dma);
 	if (ret)
-		goto err_clks;
+		return ret;
 
 	ret = qce_check_version(qce);
 	if (ret)
@@ -268,12 +256,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
 
 err_dma:
 	qce_dma_release(&qce->dma);
-err_clks:
-	clk_disable_unprepare(qce->bus);
-err_clks_iface:
-	clk_disable_unprepare(qce->iface);
-err_clks_core:
-	clk_disable_unprepare(qce->core);
 
 	return ret;
 }
@@ -285,9 +267,6 @@ static void qce_crypto_remove(struct platform_device *pdev)
 	tasklet_kill(&qce->done_tasklet);
 	qce_unregister_algs(qce);
 	qce_dma_release(&qce->dma);
-	clk_disable_unprepare(qce->bus);
-	clk_disable_unprepare(qce->iface);
-	clk_disable_unprepare(qce->core);
 }
 
 static const struct of_device_id qce_crypto_of_match[] = {

-- 
2.45.2

Re: [PATCH 4/9] crypto: qce - shrink code with devres clk helpers

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
> separately as well as putting the clocks in error path and the remove()
> callback.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 29 ++++-------------------------
>   1 file changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index f9ff1dfc1defe..cdcddf8f9f02b 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -212,15 +212,15 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		return ret;
>   
> -	qce->core = devm_clk_get_optional(qce->dev, "core");
> +	qce->core = devm_clk_get_optional_enabled(qce->dev, "core");
>   	if (IS_ERR(qce->core))
>   		return PTR_ERR(qce->core);
>   
> -	qce->iface = devm_clk_get_optional(qce->dev, "iface");
> +	qce->iface = devm_clk_get_optional_enabled(qce->dev, "iface");
>   	if (IS_ERR(qce->iface))
>   		return PTR_ERR(qce->iface);
>   
> -	qce->bus = devm_clk_get_optional(qce->dev, "bus");
> +	qce->bus = devm_clk_get_optional_enabled(qce->dev, "bus");
>   	if (IS_ERR(qce->bus))
>   		return PTR_ERR(qce->bus);
>   
> @@ -232,21 +232,9 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = clk_prepare_enable(qce->core);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(qce->iface);
> -	if (ret)
> -		goto err_clks_core;
> -
> -	ret = clk_prepare_enable(qce->bus);
> -	if (ret)
> -		goto err_clks_iface;
> -
>   	ret = qce_dma_request(qce->dev, &qce->dma);
>   	if (ret)
> -		goto err_clks;
> +		return ret;
>   
>   	ret = qce_check_version(qce);
>   	if (ret)
> @@ -268,12 +256,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   
>   err_dma:
>   	qce_dma_release(&qce->dma);
> -err_clks:
> -	clk_disable_unprepare(qce->bus);
> -err_clks_iface:
> -	clk_disable_unprepare(qce->iface);
> -err_clks_core:
> -	clk_disable_unprepare(qce->core);
>   
>   	return ret;
>   }
> @@ -285,9 +267,6 @@ static void qce_crypto_remove(struct platform_device *pdev)
>   	tasklet_kill(&qce->done_tasklet);
>   	qce_unregister_algs(qce);
>   	qce_dma_release(&qce->dma);
> -	clk_disable_unprepare(qce->bus);
> -	clk_disable_unprepare(qce->iface);
> -	clk_disable_unprepare(qce->core);
>   }
>   
>   static const struct of_device_id qce_crypto_of_match[] = {
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 4/9] crypto: qce - shrink code with devres clk helpers

[Stephan Gerhold]

On Tue, Dec 03, 2024 at 10:19:32AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
> separately as well as putting the clocks in error path and the remove()
> callback.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

FWIW: Ideally, the driver shouldn't keep on the clock all the time in
the first place, since this will prevent reaching deeper low power
states. So while this cleanup is nice, I think it will have to be
reverted again once someone fixes that.

If you're working on refactoring this rarely cared for driver, is there
any chance you could add some form of runtime PM while you're at it? :-)

Thanks,
Stephan

Re: [PATCH 4/9] crypto: qce - shrink code with devres clk helpers

[Bartosz Golaszewski]

On Tue, Dec 3, 2024 at 7:32 PM Stephan Gerhold
<stephan.gerhold@linaro.org> wrote:
>
> On Tue, Dec 03, 2024 at 10:19:32AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
> > separately as well as putting the clocks in error path and the remove()
> > callback.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> FWIW: Ideally, the driver shouldn't keep on the clock all the time in
> the first place, since this will prevent reaching deeper low power
> states. So while this cleanup is nice, I think it will have to be
> reverted again once someone fixes that.
>
> If you're working on refactoring this rarely cared for driver, is there
> any chance you could add some form of runtime PM while you're at it? :-)
>

Sure, I will most likely be doing more work on this in the future, I
can think about it. This patch was about code shrink, not functional
changes so I prefer to keep it as is.

Bart

[PATCH 5/9] crypto: qce - convert qce_dma_request() to use devres

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Make qce_dma_request() into a managed interface. With this we can
simplify the error path in probe() and drop another operations from
remove().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 16 +++-------------
 drivers/crypto/qce/dma.c  | 22 +++++++++++++---------
 drivers/crypto/qce/dma.h  |  3 +--
 3 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index cdcddf8f9f02b..e2cda24960f63 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -232,13 +232,13 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = qce_dma_request(qce->dev, &qce->dma);
+	ret = devm_qce_dma_request(qce->dev, &qce->dma);
 	if (ret)
 		return ret;
 
 	ret = qce_check_version(qce);
 	if (ret)
-		goto err_dma;
+		return ret;
 
 	spin_lock_init(&qce->lock);
 	tasklet_init(&qce->done_tasklet, qce_tasklet_req_done,
@@ -248,16 +248,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	qce->async_req_enqueue = qce_async_request_enqueue;
 	qce->async_req_done = qce_async_request_done;
 
-	ret = qce_register_algs(qce);
-	if (ret)
-		goto err_dma;
-
-	return 0;
-
-err_dma:
-	qce_dma_release(&qce->dma);
-
-	return ret;
+	return qce_register_algs(qce);
 }
 
 static void qce_crypto_remove(struct platform_device *pdev)
@@ -266,7 +257,6 @@ static void qce_crypto_remove(struct platform_device *pdev)
 
 	tasklet_kill(&qce->done_tasklet);
 	qce_unregister_algs(qce);
-	qce_dma_release(&qce->dma);
 }
 
 static const struct of_device_id qce_crypto_of_match[] = {
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 46db5bf366b44..1dec7aea852dd 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -3,12 +3,22 @@
  * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/device.h>
 #include <linux/dmaengine.h>
 #include <crypto/scatterwalk.h>
 
 #include "dma.h"
 
-int qce_dma_request(struct device *dev, struct qce_dma_data *dma)
+static void qce_dma_release(void *data)
+{
+	struct qce_dma_data *dma = data;
+
+	dma_release_channel(dma->txchan);
+	dma_release_channel(dma->rxchan);
+	kfree(dma->result_buf);
+}
+
+int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
 {
 	int ret;
 
@@ -31,7 +41,8 @@ int qce_dma_request(struct device *dev, struct qce_dma_data *dma)
 
 	dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
 
-	return 0;
+	return devm_add_action_or_reset(dev, qce_dma_release, dma);
+
 error_nomem:
 	dma_release_channel(dma->rxchan);
 error_rx:
@@ -39,13 +50,6 @@ int qce_dma_request(struct device *dev, struct qce_dma_data *dma)
 	return ret;
 }
 
-void qce_dma_release(struct qce_dma_data *dma)
-{
-	dma_release_channel(dma->txchan);
-	dma_release_channel(dma->rxchan);
-	kfree(dma->result_buf);
-}
-
 struct scatterlist *
 qce_sgtable_add(struct sg_table *sgt, struct scatterlist *new_sgl,
 		unsigned int max_len)
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index 7864021693608..31629185000e1 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -34,8 +34,7 @@ struct qce_dma_data {
 	void *ignore_buf;
 };
 
-int qce_dma_request(struct device *dev, struct qce_dma_data *dma);
-void qce_dma_release(struct qce_dma_data *dma);
+int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma);
 int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *sg_in,
 		     int in_ents, struct scatterlist *sg_out, int out_ents,
 		     dma_async_tx_callback cb, void *cb_param);

-- 
2.45.2

Re: [PATCH 5/9] crypto: qce - convert qce_dma_request() to use devres

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Make qce_dma_request() into a managed interface. With this we can
> simplify the error path in probe() and drop another operations from
> remove().
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 16 +++-------------
>   drivers/crypto/qce/dma.c  | 22 +++++++++++++---------
>   drivers/crypto/qce/dma.h  |  3 +--
>   3 files changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index cdcddf8f9f02b..e2cda24960f63 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -232,13 +232,13 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = qce_dma_request(qce->dev, &qce->dma);
> +	ret = devm_qce_dma_request(qce->dev, &qce->dma);
>   	if (ret)
>   		return ret;
>   
>   	ret = qce_check_version(qce);
>   	if (ret)
> -		goto err_dma;
> +		return ret;
>   
>   	spin_lock_init(&qce->lock);
>   	tasklet_init(&qce->done_tasklet, qce_tasklet_req_done,
> @@ -248,16 +248,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	qce->async_req_enqueue = qce_async_request_enqueue;
>   	qce->async_req_done = qce_async_request_done;
>   
> -	ret = qce_register_algs(qce);
> -	if (ret)
> -		goto err_dma;
> -
> -	return 0;
> -
> -err_dma:
> -	qce_dma_release(&qce->dma);
> -
> -	return ret;
> +	return qce_register_algs(qce);
>   }
>   
>   static void qce_crypto_remove(struct platform_device *pdev)
> @@ -266,7 +257,6 @@ static void qce_crypto_remove(struct platform_device *pdev)
>   
>   	tasklet_kill(&qce->done_tasklet);
>   	qce_unregister_algs(qce);
> -	qce_dma_release(&qce->dma);
>   }
>   
>   static const struct of_device_id qce_crypto_of_match[] = {
> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 46db5bf366b44..1dec7aea852dd 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
> @@ -3,12 +3,22 @@
>    * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved.
>    */
>   
> +#include <linux/device.h>
>   #include <linux/dmaengine.h>
>   #include <crypto/scatterwalk.h>
>   
>   #include "dma.h"
>   
> -int qce_dma_request(struct device *dev, struct qce_dma_data *dma)
> +static void qce_dma_release(void *data)
> +{
> +	struct qce_dma_data *dma = data;
> +
> +	dma_release_channel(dma->txchan);
> +	dma_release_channel(dma->rxchan);
> +	kfree(dma->result_buf);
> +}
> +
> +int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
>   {
>   	int ret;
>   
> @@ -31,7 +41,8 @@ int qce_dma_request(struct device *dev, struct qce_dma_data *dma)
>   
>   	dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
>   
> -	return 0;
> +	return devm_add_action_or_reset(dev, qce_dma_release, dma);
> +
>   error_nomem:
>   	dma_release_channel(dma->rxchan);
>   error_rx:
> @@ -39,13 +50,6 @@ int qce_dma_request(struct device *dev, struct qce_dma_data *dma)
>   	return ret;
>   }
>   
> -void qce_dma_release(struct qce_dma_data *dma)
> -{
> -	dma_release_channel(dma->txchan);
> -	dma_release_channel(dma->rxchan);
> -	kfree(dma->result_buf);
> -}
> -
>   struct scatterlist *
>   qce_sgtable_add(struct sg_table *sgt, struct scatterlist *new_sgl,
>   		unsigned int max_len)
> diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
> index 7864021693608..31629185000e1 100644
> --- a/drivers/crypto/qce/dma.h
> +++ b/drivers/crypto/qce/dma.h
> @@ -34,8 +34,7 @@ struct qce_dma_data {
>   	void *ignore_buf;
>   };
>   
> -int qce_dma_request(struct device *dev, struct qce_dma_data *dma);
> -void qce_dma_release(struct qce_dma_data *dma);
> +int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma);
>   int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *sg_in,
>   		     int in_ents, struct scatterlist *sg_out, int out_ents,
>   		     dma_async_tx_callback cb, void *cb_param);
> 

Nice rework :-)

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 6/9] crypto: qce - make qce_register_algs() a managed interface

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Make qce_register_algs() a managed interface. This allows us to further
simplify the remove() callback.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index e2cda24960f63..5e21754c7f822 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/device.h>
 #include <linux/dma-mapping.h>
 #include <linux/interconnect.h>
 #include <linux/interrupt.h>
@@ -37,9 +38,10 @@ static const struct qce_algo_ops *qce_ops[] = {
 #endif
 };
 
-static void qce_unregister_algs(struct qce_device *qce)
+static void qce_unregister_algs(void *data)
 {
 	const struct qce_algo_ops *ops;
+	struct qce_device *qce = data;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
@@ -48,7 +50,7 @@ static void qce_unregister_algs(struct qce_device *qce)
 	}
 }
 
-static int qce_register_algs(struct qce_device *qce)
+static int devm_qce_register_algs(struct qce_device *qce)
 {
 	const struct qce_algo_ops *ops;
 	int i, j, ret = -ENODEV;
@@ -63,7 +65,7 @@ static int qce_register_algs(struct qce_device *qce)
 		}
 	}
 
-	return 0;
+	return devm_add_action_or_reset(qce->dev, qce_unregister_algs, qce);
 }
 
 static int qce_handle_request(struct crypto_async_request *async_req)
@@ -248,7 +250,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	qce->async_req_enqueue = qce_async_request_enqueue;
 	qce->async_req_done = qce_async_request_done;
 
-	return qce_register_algs(qce);
+	return devm_qce_register_algs(qce);
 }
 
 static void qce_crypto_remove(struct platform_device *pdev)
@@ -256,7 +258,6 @@ static void qce_crypto_remove(struct platform_device *pdev)
 	struct qce_device *qce = platform_get_drvdata(pdev);
 
 	tasklet_kill(&qce->done_tasklet);
-	qce_unregister_algs(qce);
 }
 
 static const struct of_device_id qce_crypto_of_match[] = {

-- 
2.45.2

Re: [PATCH 6/9] crypto: qce - make qce_register_algs() a managed interface

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Make qce_register_algs() a managed interface. This allows us to further
> simplify the remove() callback.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index e2cda24960f63..5e21754c7f822 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -4,6 +4,7 @@
>    */
>   
>   #include <linux/clk.h>
> +#include <linux/device.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/interconnect.h>
>   #include <linux/interrupt.h>
> @@ -37,9 +38,10 @@ static const struct qce_algo_ops *qce_ops[] = {
>   #endif
>   };
>   
> -static void qce_unregister_algs(struct qce_device *qce)
> +static void qce_unregister_algs(void *data)
>   {
>   	const struct qce_algo_ops *ops;
> +	struct qce_device *qce = data;
>   	int i;
>   
>   	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
> @@ -48,7 +50,7 @@ static void qce_unregister_algs(struct qce_device *qce)
>   	}
>   }
>   
> -static int qce_register_algs(struct qce_device *qce)
> +static int devm_qce_register_algs(struct qce_device *qce)
>   {
>   	const struct qce_algo_ops *ops;
>   	int i, j, ret = -ENODEV;
> @@ -63,7 +65,7 @@ static int qce_register_algs(struct qce_device *qce)
>   		}
>   	}
>   
> -	return 0;
> +	return devm_add_action_or_reset(qce->dev, qce_unregister_algs, qce);
>   }
>   
>   static int qce_handle_request(struct crypto_async_request *async_req)
> @@ -248,7 +250,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	qce->async_req_enqueue = qce_async_request_enqueue;
>   	qce->async_req_done = qce_async_request_done;
>   
> -	return qce_register_algs(qce);
> +	return devm_qce_register_algs(qce);
>   }
>   
>   static void qce_crypto_remove(struct platform_device *pdev)
> @@ -256,7 +258,6 @@ static void qce_crypto_remove(struct platform_device *pdev)
>   	struct qce_device *qce = platform_get_drvdata(pdev);
>   
>   	tasklet_kill(&qce->done_tasklet);
> -	qce_unregister_algs(qce);
>   }
>   
>   static const struct of_device_id qce_crypto_of_match[] = {
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 7/9] crypto: qce - use __free() for a buffer that's always freed

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The buffer allocated in qce_ahash_hmac_setkey is always freed before
returning to use __free() to automate it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/sha.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
index fc72af8aa9a72..916908c04b635 100644
--- a/drivers/crypto/qce/sha.c
+++ b/drivers/crypto/qce/sha.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
@@ -336,7 +337,6 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
 	struct scatterlist sg;
 	unsigned int blocksize;
 	struct crypto_ahash *ahash_tfm;
-	u8 *buf;
 	int ret;
 	const char *alg_name;
 
@@ -370,7 +370,8 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
 				   crypto_req_done, &wait);
 	crypto_ahash_clear_flags(ahash_tfm, ~0);
 
-	buf = kzalloc(keylen + QCE_MAX_ALIGN_SIZE, GFP_KERNEL);
+	u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
+					GFP_KERNEL);
 	if (!buf) {
 		ret = -ENOMEM;
 		goto err_free_req;
@@ -382,7 +383,6 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
 
 	ret = crypto_wait_req(crypto_ahash_digest(req), &wait);
 
-	kfree(buf);
 err_free_req:
 	ahash_request_free(req);
 err_free_ahash:

-- 
2.45.2

Re: [PATCH 7/9] crypto: qce - use __free() for a buffer that's always freed

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The buffer allocated in qce_ahash_hmac_setkey is always freed before
> returning to use __free() to automate it.

I think you wanted to use "so" instead of "to", otherwise it means nothing ^^

> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/sha.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
> index fc72af8aa9a72..916908c04b635 100644
> --- a/drivers/crypto/qce/sha.c
> +++ b/drivers/crypto/qce/sha.c
> @@ -3,6 +3,7 @@
>    * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
>    */
>   
> +#include <linux/cleanup.h>
>   #include <linux/device.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/interrupt.h>
> @@ -336,7 +337,6 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
>   	struct scatterlist sg;
>   	unsigned int blocksize;
>   	struct crypto_ahash *ahash_tfm;
> -	u8 *buf;
>   	int ret;
>   	const char *alg_name;
>   
> @@ -370,7 +370,8 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
>   				   crypto_req_done, &wait);
>   	crypto_ahash_clear_flags(ahash_tfm, ~0);
>   
> -	buf = kzalloc(keylen + QCE_MAX_ALIGN_SIZE, GFP_KERNEL);
> +	u8 *buf __free(kfree) = kzalloc(keylen + QCE_MAX_ALIGN_SIZE,
> +					GFP_KERNEL);
>   	if (!buf) {
>   		ret = -ENOMEM;
>   		goto err_free_req;
> @@ -382,7 +383,6 @@ static int qce_ahash_hmac_setkey(struct crypto_ahash *tfm, const u8 *key,
>   
>   	ret = crypto_wait_req(crypto_ahash_digest(req), &wait);
>   
> -	kfree(buf);
>   err_free_req:
>   	ahash_request_free(req);
>   err_free_ahash:
> 

With the typo fixes:

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 8/9] crypto: qce - convert tasklet to workqueue

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's nothing about the qce driver that requires running from a
tasklet. Switch to using the system workqueue.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 20 ++++++--------------
 drivers/crypto/qce/core.h |  6 ++++--
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index 5e21754c7f822..6de9f1e23e282 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -122,15 +122,16 @@ static int qce_handle_queue(struct qce_device *qce,
 	err = qce_handle_request(async_req);
 	if (err) {
 		qce->result = err;
-		tasklet_schedule(&qce->done_tasklet);
+		schedule_work(&qce->done_work);
 	}
 
 	return ret;
 }
 
-static void qce_tasklet_req_done(unsigned long data)
+static void qce_req_done_work(struct work_struct *work)
 {
-	struct qce_device *qce = (struct qce_device *)data;
+	struct qce_device *qce = container_of(work, struct qce_device,
+					      done_work);
 	struct crypto_async_request *req;
 	unsigned long flags;
 
@@ -154,7 +155,7 @@ static int qce_async_request_enqueue(struct qce_device *qce,
 static void qce_async_request_done(struct qce_device *qce, int ret)
 {
 	qce->result = ret;
-	tasklet_schedule(&qce->done_tasklet);
+	schedule_work(&qce->done_work);
 }
 
 static int qce_check_version(struct qce_device *qce)
@@ -243,8 +244,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
 		return ret;
 
 	spin_lock_init(&qce->lock);
-	tasklet_init(&qce->done_tasklet, qce_tasklet_req_done,
-		     (unsigned long)qce);
+	INIT_WORK(&qce->done_work, qce_req_done_work);
 	crypto_init_queue(&qce->queue, QCE_QUEUE_LENGTH);
 
 	qce->async_req_enqueue = qce_async_request_enqueue;
@@ -253,13 +253,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	return devm_qce_register_algs(qce);
 }
 
-static void qce_crypto_remove(struct platform_device *pdev)
-{
-	struct qce_device *qce = platform_get_drvdata(pdev);
-
-	tasklet_kill(&qce->done_tasklet);
-}
-
 static const struct of_device_id qce_crypto_of_match[] = {
 	{ .compatible = "qcom,crypto-v5.1", },
 	{ .compatible = "qcom,crypto-v5.4", },
@@ -270,7 +263,6 @@ MODULE_DEVICE_TABLE(of, qce_crypto_of_match);
 
 static struct platform_driver qce_crypto_driver = {
 	.probe = qce_crypto_probe,
-	.remove = qce_crypto_remove,
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.of_match_table = qce_crypto_of_match,
diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
index 228fcd69ec511..39e75a75a4293 100644
--- a/drivers/crypto/qce/core.h
+++ b/drivers/crypto/qce/core.h
@@ -6,13 +6,15 @@
 #ifndef _CORE_H_
 #define _CORE_H_
 
+#include <linux/workqueue.h>
+
 #include "dma.h"
 
 /**
  * struct qce_device - crypto engine device structure
  * @queue: crypto request queue
  * @lock: the lock protects queue and req
- * @done_tasklet: done tasklet object
+ * @done_work: workqueue context
  * @req: current active request
  * @result: result of current transform
  * @base: virtual IO base
@@ -29,7 +31,7 @@
 struct qce_device {
 	struct crypto_queue queue;
 	spinlock_t lock;
-	struct tasklet_struct done_tasklet;
+	struct work_struct done_work;
 	struct crypto_async_request *req;
 	int result;
 	void __iomem *base;

-- 
2.45.2

Re: [PATCH 8/9] crypto: qce - convert tasklet to workqueue

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> There's nothing about the qce driver that requires running from a
> tasklet. Switch to using the system workqueue.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 20 ++++++--------------
>   drivers/crypto/qce/core.h |  6 ++++--
>   2 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index 5e21754c7f822..6de9f1e23e282 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -122,15 +122,16 @@ static int qce_handle_queue(struct qce_device *qce,
>   	err = qce_handle_request(async_req);
>   	if (err) {
>   		qce->result = err;
> -		tasklet_schedule(&qce->done_tasklet);
> +		schedule_work(&qce->done_work);
>   	}
>   
>   	return ret;
>   }
>   
> -static void qce_tasklet_req_done(unsigned long data)
> +static void qce_req_done_work(struct work_struct *work)
>   {
> -	struct qce_device *qce = (struct qce_device *)data;
> +	struct qce_device *qce = container_of(work, struct qce_device,
> +					      done_work);
>   	struct crypto_async_request *req;
>   	unsigned long flags;
>   
> @@ -154,7 +155,7 @@ static int qce_async_request_enqueue(struct qce_device *qce,
>   static void qce_async_request_done(struct qce_device *qce, int ret)
>   {
>   	qce->result = ret;
> -	tasklet_schedule(&qce->done_tasklet);
> +	schedule_work(&qce->done_work);
>   }
>   
>   static int qce_check_version(struct qce_device *qce)
> @@ -243,8 +244,7 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   		return ret;
>   
>   	spin_lock_init(&qce->lock);
> -	tasklet_init(&qce->done_tasklet, qce_tasklet_req_done,
> -		     (unsigned long)qce);
> +	INIT_WORK(&qce->done_work, qce_req_done_work);
>   	crypto_init_queue(&qce->queue, QCE_QUEUE_LENGTH);
>   
>   	qce->async_req_enqueue = qce_async_request_enqueue;
> @@ -253,13 +253,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	return devm_qce_register_algs(qce);
>   }
>   
> -static void qce_crypto_remove(struct platform_device *pdev)
> -{
> -	struct qce_device *qce = platform_get_drvdata(pdev);
> -
> -	tasklet_kill(&qce->done_tasklet);
> -}
> -
>   static const struct of_device_id qce_crypto_of_match[] = {
>   	{ .compatible = "qcom,crypto-v5.1", },
>   	{ .compatible = "qcom,crypto-v5.4", },
> @@ -270,7 +263,6 @@ MODULE_DEVICE_TABLE(of, qce_crypto_of_match);
>   
>   static struct platform_driver qce_crypto_driver = {
>   	.probe = qce_crypto_probe,
> -	.remove = qce_crypto_remove,
>   	.driver = {
>   		.name = KBUILD_MODNAME,
>   		.of_match_table = qce_crypto_of_match,
> diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
> index 228fcd69ec511..39e75a75a4293 100644
> --- a/drivers/crypto/qce/core.h
> +++ b/drivers/crypto/qce/core.h
> @@ -6,13 +6,15 @@
>   #ifndef _CORE_H_
>   #define _CORE_H_
>   
> +#include <linux/workqueue.h>
> +
>   #include "dma.h"
>   
>   /**
>    * struct qce_device - crypto engine device structure
>    * @queue: crypto request queue
>    * @lock: the lock protects queue and req
> - * @done_tasklet: done tasklet object
> + * @done_work: workqueue context
>    * @req: current active request
>    * @result: result of current transform
>    * @base: virtual IO base
> @@ -29,7 +31,7 @@
>   struct qce_device {
>   	struct crypto_queue queue;
>   	spinlock_t lock;
> -	struct tasklet_struct done_tasklet;
> +	struct work_struct done_work;
>   	struct crypto_async_request *req;
>   	int result;
>   	void __iomem *base;
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 9/9] crypto: qce - switch to using a mutex

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Having switched to workqueue from tasklet, we are no longer limited to
atomic APIs and can now convert the spinlock to a mutex. This, along
with the conversion from tasklet to workqueue grants us ~15% improvement
in cryptsetup benchmarks for AES encryption.

While at it: use guards to simplify locking code.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 46 +++++++++++++++++++++-------------------------
 drivers/crypto/qce/core.h |  3 ++-
 2 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index 6de9f1e23e282..e95e84486d9ae 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
@@ -11,7 +12,6 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
-#include <linux/spinlock.h>
 #include <linux/types.h>
 #include <crypto/algapi.h>
 #include <crypto/internal/hash.h>
@@ -89,34 +89,28 @@ static int qce_handle_queue(struct qce_device *qce,
 			    struct crypto_async_request *req)
 {
 	struct crypto_async_request *async_req, *backlog;
-	unsigned long flags;
 	int ret = 0, err;
 
-	spin_lock_irqsave(&qce->lock, flags);
+	scoped_guard(mutex, &qce->lock) {
+		if (req)
+			ret = crypto_enqueue_request(&qce->queue, req);
 
-	if (req)
-		ret = crypto_enqueue_request(&qce->queue, req);
+		/* busy, do not dequeue request */
+		if (qce->req)
+			return ret;
 
-	/* busy, do not dequeue request */
-	if (qce->req) {
-		spin_unlock_irqrestore(&qce->lock, flags);
-		return ret;
+		backlog = crypto_get_backlog(&qce->queue);
+		async_req = crypto_dequeue_request(&qce->queue);
+		if (async_req)
+			qce->req = async_req;
 	}
 
-	backlog = crypto_get_backlog(&qce->queue);
-	async_req = crypto_dequeue_request(&qce->queue);
-	if (async_req)
-		qce->req = async_req;
-
-	spin_unlock_irqrestore(&qce->lock, flags);
-
 	if (!async_req)
 		return ret;
 
 	if (backlog) {
-		spin_lock_bh(&qce->lock);
-		crypto_request_complete(backlog, -EINPROGRESS);
-		spin_unlock_bh(&qce->lock);
+		scoped_guard(mutex, &qce->lock)
+			crypto_request_complete(backlog, -EINPROGRESS);
 	}
 
 	err = qce_handle_request(async_req);
@@ -133,12 +127,11 @@ static void qce_req_done_work(struct work_struct *work)
 	struct qce_device *qce = container_of(work, struct qce_device,
 					      done_work);
 	struct crypto_async_request *req;
-	unsigned long flags;
 
-	spin_lock_irqsave(&qce->lock, flags);
-	req = qce->req;
-	qce->req = NULL;
-	spin_unlock_irqrestore(&qce->lock, flags);
+	scoped_guard(mutex, &qce->lock) {
+		req = qce->req;
+		qce->req = NULL;
+	}
 
 	if (req)
 		crypto_request_complete(req, qce->result);
@@ -243,7 +236,10 @@ static int qce_crypto_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	spin_lock_init(&qce->lock);
+	ret = devm_mutex_init(qce->dev, &qce->lock);
+	if (ret)
+		return ret;
+
 	INIT_WORK(&qce->done_work, qce_req_done_work);
 	crypto_init_queue(&qce->queue, QCE_QUEUE_LENGTH);
 
diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
index 39e75a75a4293..eb6fa7a8b64a8 100644
--- a/drivers/crypto/qce/core.h
+++ b/drivers/crypto/qce/core.h
@@ -6,6 +6,7 @@
 #ifndef _CORE_H_
 #define _CORE_H_
 
+#include <linux/mutex.h>
 #include <linux/workqueue.h>
 
 #include "dma.h"
@@ -30,7 +31,7 @@
  */
 struct qce_device {
 	struct crypto_queue queue;
-	spinlock_t lock;
+	struct mutex lock;
 	struct work_struct done_work;
 	struct crypto_async_request *req;
 	int result;

-- 
2.45.2

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[neil.armstrong]

On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Having switched to workqueue from tasklet, we are no longer limited to
> atomic APIs and can now convert the spinlock to a mutex. This, along
> with the conversion from tasklet to workqueue grants us ~15% improvement
> in cryptsetup benchmarks for AES encryption.

Can you share on which platforms you did the tests and the results you got ?

> 
> While at it: use guards to simplify locking code.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 46 +++++++++++++++++++++-------------------------
>   drivers/crypto/qce/core.h |  3 ++-
>   2 files changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index 6de9f1e23e282..e95e84486d9ae 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -3,6 +3,7 @@
>    * Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
>    */
>   
> +#include <linux/cleanup.h>
>   #include <linux/clk.h>
>   #include <linux/device.h>
>   #include <linux/dma-mapping.h>
> @@ -11,7 +12,6 @@
>   #include <linux/module.h>
>   #include <linux/mod_devicetable.h>
>   #include <linux/platform_device.h>
> -#include <linux/spinlock.h>
>   #include <linux/types.h>
>   #include <crypto/algapi.h>
>   #include <crypto/internal/hash.h>
> @@ -89,34 +89,28 @@ static int qce_handle_queue(struct qce_device *qce,
>   			    struct crypto_async_request *req)
>   {
>   	struct crypto_async_request *async_req, *backlog;
> -	unsigned long flags;
>   	int ret = 0, err;
>   
> -	spin_lock_irqsave(&qce->lock, flags);
> +	scoped_guard(mutex, &qce->lock) {
> +		if (req)
> +			ret = crypto_enqueue_request(&qce->queue, req);
>   
> -	if (req)
> -		ret = crypto_enqueue_request(&qce->queue, req);
> +		/* busy, do not dequeue request */
> +		if (qce->req)
> +			return ret;
>   
> -	/* busy, do not dequeue request */
> -	if (qce->req) {
> -		spin_unlock_irqrestore(&qce->lock, flags);
> -		return ret;
> +		backlog = crypto_get_backlog(&qce->queue);
> +		async_req = crypto_dequeue_request(&qce->queue);
> +		if (async_req)
> +			qce->req = async_req;
>   	}
>   
> -	backlog = crypto_get_backlog(&qce->queue);
> -	async_req = crypto_dequeue_request(&qce->queue);
> -	if (async_req)
> -		qce->req = async_req;
> -
> -	spin_unlock_irqrestore(&qce->lock, flags);
> -
>   	if (!async_req)
>   		return ret;
>   
>   	if (backlog) {
> -		spin_lock_bh(&qce->lock);
> -		crypto_request_complete(backlog, -EINPROGRESS);
> -		spin_unlock_bh(&qce->lock);
> +		scoped_guard(mutex, &qce->lock)
> +			crypto_request_complete(backlog, -EINPROGRESS);
>   	}
>   
>   	err = qce_handle_request(async_req);
> @@ -133,12 +127,11 @@ static void qce_req_done_work(struct work_struct *work)
>   	struct qce_device *qce = container_of(work, struct qce_device,
>   					      done_work);
>   	struct crypto_async_request *req;
> -	unsigned long flags;
>   
> -	spin_lock_irqsave(&qce->lock, flags);
> -	req = qce->req;
> -	qce->req = NULL;
> -	spin_unlock_irqrestore(&qce->lock, flags);
> +	scoped_guard(mutex, &qce->lock) {
> +		req = qce->req;
> +		qce->req = NULL;
> +	}
>   
>   	if (req)
>   		crypto_request_complete(req, qce->result);
> @@ -243,7 +236,10 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	spin_lock_init(&qce->lock);
> +	ret = devm_mutex_init(qce->dev, &qce->lock);
> +	if (ret)
> +		return ret;
> +
>   	INIT_WORK(&qce->done_work, qce_req_done_work);
>   	crypto_init_queue(&qce->queue, QCE_QUEUE_LENGTH);
>   
> diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
> index 39e75a75a4293..eb6fa7a8b64a8 100644
> --- a/drivers/crypto/qce/core.h
> +++ b/drivers/crypto/qce/core.h
> @@ -6,6 +6,7 @@
>   #ifndef _CORE_H_
>   #define _CORE_H_
>   
> +#include <linux/mutex.h>
>   #include <linux/workqueue.h>
>   
>   #include "dma.h"
> @@ -30,7 +31,7 @@
>    */
>   struct qce_device {
>   	struct crypto_queue queue;
> -	spinlock_t lock;
> +	struct mutex lock;
>   	struct work_struct done_work;
>   	struct crypto_async_request *req;
>   	int result;
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[Bartosz Golaszewski]

On Tue, 3 Dec 2024 14:53:21 +0100, neil.armstrong@linaro.org said:
> On 03/12/2024 10:19, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> Having switched to workqueue from tasklet, we are no longer limited to
>> atomic APIs and can now convert the spinlock to a mutex. This, along
>> with the conversion from tasklet to workqueue grants us ~15% improvement
>> in cryptsetup benchmarks for AES encryption.
>
> Can you share on which platforms you did the tests and the results you got ?
>

Sure, I tested on sm8650 with the following results (they vary from
one run to other but are more or less in this range):

With this series:

#     Algorithm |       Key |      Encryption |      Decryption
        aes-cbc        128b        94.1 MiB/s       138.6 MiB/s
    serpent-cbc        128b               N/A               N/A
    twofish-cbc        128b               N/A               N/A
        aes-cbc        256b        94.8 MiB/s       128.5 MiB/s
    serpent-cbc        256b               N/A               N/A
    twofish-cbc        256b               N/A               N/A
        aes-xts        256b       132.9 MiB/s       131.8 MiB/s
    serpent-xts        256b               N/A               N/A
    twofish-xts        256b               N/A               N/A
        aes-xts        512b       122.6 MiB/s       122.4 MiB/s
    serpent-xts        512b               N/A               N/A
    twofish-xts        512b               N/A               N/A

Without it:

#     Algorithm |       Key |      Encryption |      Decryption
        aes-cbc        128b        96.4 MiB/s       141.0 MiB/s
    serpent-cbc        128b               N/A               N/A
    twofish-cbc        128b               N/A               N/A
        aes-cbc        256b        67.0 MiB/s        97.8 MiB/s
    serpent-cbc        256b               N/A               N/A
    twofish-cbc        256b               N/A               N/A
        aes-xts        256b       131.7 MiB/s       132.0 MiB/s
    serpent-xts        256b               N/A               N/A
    twofish-xts        256b               N/A               N/A
        aes-xts        512b        93.9 MiB/s        96.8 MiB/s
    serpent-xts        512b               N/A               N/A
    twofish-xts        512b               N/A               N/A

AES-CBC and AES-XTS with shorter keys remain pretty much the same. I'm not
sure why that is. I also tested on sa8775p but there are no visible
improvements there. :(

Bart

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[neil.armstrong]

On 03/12/2024 16:10, Bartosz Golaszewski wrote:
> On Tue, 3 Dec 2024 14:53:21 +0100, neil.armstrong@linaro.org said:
>> On 03/12/2024 10:19, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> Having switched to workqueue from tasklet, we are no longer limited to
>>> atomic APIs and can now convert the spinlock to a mutex. This, along
>>> with the conversion from tasklet to workqueue grants us ~15% improvement
>>> in cryptsetup benchmarks for AES encryption.
>>
>> Can you share on which platforms you did the tests and the results you got ?
>>
> 
> Sure, I tested on sm8650 with the following results (they vary from
> one run to other but are more or less in this range):
> 
> With this series:
> 
> #     Algorithm |       Key |      Encryption |      Decryption
>          aes-cbc        128b        94.1 MiB/s       138.6 MiB/s
>      serpent-cbc        128b               N/A               N/A
>      twofish-cbc        128b               N/A               N/A
>          aes-cbc        256b        94.8 MiB/s       128.5 MiB/s
>      serpent-cbc        256b               N/A               N/A
>      twofish-cbc        256b               N/A               N/A
>          aes-xts        256b       132.9 MiB/s       131.8 MiB/s
>      serpent-xts        256b               N/A               N/A
>      twofish-xts        256b               N/A               N/A
>          aes-xts        512b       122.6 MiB/s       122.4 MiB/s
>      serpent-xts        512b               N/A               N/A
>      twofish-xts        512b               N/A               N/A
> 
> Without it:
> 
> #     Algorithm |       Key |      Encryption |      Decryption
>          aes-cbc        128b        96.4 MiB/s       141.0 MiB/s
>      serpent-cbc        128b               N/A               N/A
>      twofish-cbc        128b               N/A               N/A
>          aes-cbc        256b        67.0 MiB/s        97.8 MiB/s
>      serpent-cbc        256b               N/A               N/A
>      twofish-cbc        256b               N/A               N/A
>          aes-xts        256b       131.7 MiB/s       132.0 MiB/s
>      serpent-xts        256b               N/A               N/A
>      twofish-xts        256b               N/A               N/A
>          aes-xts        512b        93.9 MiB/s        96.8 MiB/s
>      serpent-xts        512b               N/A               N/A
>      twofish-xts        512b               N/A               N/A
> 
> AES-CBC and AES-XTS with shorter keys remain pretty much the same. I'm not
> sure why that is. I also tested on sa8775p but there are no visible
> improvements there. :(

Thanks for the results !

Neil

> 
> Bart

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[Eric Biggers]

On Tue, Dec 03, 2024 at 10:10:13AM -0500, Bartosz Golaszewski wrote:
> On Tue, 3 Dec 2024 14:53:21 +0100, neil.armstrong@linaro.org said:
> > On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>
> >> Having switched to workqueue from tasklet, we are no longer limited to
> >> atomic APIs and can now convert the spinlock to a mutex. This, along
> >> with the conversion from tasklet to workqueue grants us ~15% improvement
> >> in cryptsetup benchmarks for AES encryption.
> >
> > Can you share on which platforms you did the tests and the results you got ?
> >
> 
> Sure, I tested on sm8650 with the following results (they vary from
> one run to other but are more or less in this range):

FYI, when I test this driver on sm8650 with linux-next there are lots of test
failures, including ones where the driver straight out returns wrong hashes:

[    6.330656] alg: skcipher: xts-aes-qce setkey failed on test vector 0; expected_error=0, actual_error=-126, flags=0x1
[    6.347196] alg: self-tests for xts(aes) using xts-aes-qce failed (rc=-126)
[    6.347200] alg: self-tests for xts(aes) using xts-aes-qce failed (rc=-126)
[    6.784951] alg: skcipher: ctr-aes-qce encryption test failed (wrong output IV) on test vector 4, cfg="in-place (one sglist)"
[    6.810709] alg: self-tests for ctr(aes) using ctr-aes-qce failed (rc=-22)
[    6.941639] alg: skcipher: cbc-3des-qce setkey failed on test vector "random: len=16 klen=24"; expected_error=0, actual_error=-126, flags=0x1
[    6.947029] alg: self-tests for ctr(aes) using ctr-aes-qce failed (rc=-22)
[    6.954394] alg: self-tests for cbc(des3_ede) using cbc-3des-qce failed (rc=-126)
[    6.975348] alg: self-tests for cbc(des3_ede) using cbc-3des-qce failed (rc=-126)
[    7.454433] alg: skcipher: ecb-3des-qce setkey failed on test vector "random: len=32 klen=24"; expected_error=0, actual_error=-126, flags=0x1
[    7.454482] alg: self-tests for ecb(des3_ede) using ecb-3des-qce failed (rc=-126)
[    7.454493] alg: self-tests for ecb(des3_ede) using ecb-3des-qce failed (rc=-126)
[    8.593828] alg: ahash: hmac-sha256-qce test failed (wrong result) on test vector "random: psize=0 ksize=80", cfg="random: may_sleep use_finup src_divs=[<reimport>100.0%@+1800] key_offset=7"
[    8.627337] alg: self-tests for hmac(sha256) using hmac-sha256-qce failed (rc=-22)
[    8.639889] alg: self-tests for hmac(sha256) using hmac-sha256-qce failed (rc=-22)
[    8.933595] alg: ahash: hmac-sha1-qce test failed (wrong result) on test vector "random: psize=0 ksize=56", cfg="random: inplace_one_sglist use_finup nosimd_setkey src_divs=[100.0%@+3969] key_offset=71"
[    8.952885] alg: self-tests for hmac(sha1) using hmac-sha1-qce failed (rc=-22)
[    8.965096] alg: self-tests for hmac(sha1) using hmac-sha1-qce failed (rc=-22)

Also a KASAN error:

[    9.420862] CPU: 3 UID: 0 PID: 393 Comm: cryptomgr_test Tainted: G S      W          6.13.0-rc7-next-20250117-00007-g58182eb6d73d #12
[    9.420881] Tainted: [S]=CPU_OUT_OF_SPEC, [W]=WARN
[    9.420886] Hardware name: Qualcomm Technologies, Inc. SM8650 HDK (DT)
[    9.420891] Call trace:
[    9.420895]  show_stack+0x18/0x24 (C)
[    9.420918]  dump_stack_lvl+0x60/0x80
[    9.420937]  print_report+0x17c/0x4d0
[    9.420960]  kasan_report+0xb0/0xf8
[    9.420983]  kasan_check_range+0x100/0x1a8
[    9.421001]  __asan_memcpy+0x3c/0xa0
[    9.421020]  swiotlb_bounce+0x1b4/0x340
[    9.421034]  swiotlb_tbl_map_single+0x2ac/0x5a0
[    9.421050]  iommu_dma_map_page+0x2b8/0x4cc
[    9.421063]  iommu_dma_map_sg+0x2e8/0xb3c
[    9.421072]  __dma_map_sg_attrs+0x11c/0x1b0
[    9.421086]  dma_map_sg_attrs+0x10/0x20
[    9.421097]  qce_aead_async_req_handle+0x784/0x1aa0
[    9.421125]  qce_handle_queue+0x20c/0x3e8
[    9.421139]  qce_async_request_enqueue+0x10/0x1c
[    9.421153]  qce_aead_crypt+0x1f0/0x81c
[    9.421168]  qce_aead_encrypt+0x14/0x20
[    9.421182]  crypto_aead_encrypt+0xa0/0xe0
[    9.421194]  test_aead_vec_cfg+0x84c/0x1be8
[    9.421207]  test_aead_vs_generic_impl+0x530/0x850
[    9.421219]  alg_test_aead+0x7c8/0xe40
[    9.421230]  alg_test+0x1f4/0xb0c
[    9.421241]  cryptomgr_test+0x50/0x80
[    9.421251]  kthread+0x378/0x678
[    9.421272]  ret_from_fork+0x10/0x20

[    9.550765] Allocated by task 393:
[    9.554279]  kasan_save_stack+0x3c/0x64
[    9.558252]  kasan_save_track+0x20/0x40
[    9.562221]  kasan_save_alloc_info+0x40/0x54
[    9.566641]  __kasan_kmalloc+0xb8/0xbc
[    9.570515]  __kmalloc_noprof+0x188/0x410
[    9.574669]  qce_aead_async_req_handle+0xbd4/0x1aa0
[    9.579701]  qce_handle_queue+0x20c/0x3e8
[    9.583841]  qce_async_request_enqueue+0x10/0x1c
[    9.588607]  qce_aead_crypt+0x1f0/0x81c
[    9.592577]  qce_aead_encrypt+0x14/0x20
[    9.596547]  crypto_aead_encrypt+0xa0/0xe0
[    9.600776]  test_aead_vec_cfg+0x84c/0x1be8
[    9.605097]  test_aead_vs_generic_impl+0x530/0x850
[    9.610039]  alg_test_aead+0x7c8/0xe40
[    9.613911]  alg_test+0x1f4/0xb0c
[    9.617345]  cryptomgr_test+0x50/0x80
[    9.621124]  kthread+0x378/0x678
[    9.624473]  ret_from_fork+0x10/0x20

[    9.629738] The buggy address belongs to the object at ffff5c7440d1cc00
                which belongs to the cache kmalloc-32 of size 32
[    9.642426] The buggy address is located 0 bytes inside of
                allocated 22-byte region [ffff5c7440d1cc00, ffff5c7440d1cc16)

[    9.656672] The buggy address belongs to the physical page:
[    9.662409] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x880d1c
[    9.670646] flags: 0xbfffe0000000000(node=0|zone=2|lastcpupid=0x1ffff)
[    9.677371] page_type: f5(slab)
[    9.680624] raw: 0bfffe0000000000 ffff5c7440002780 dead000000000100 dead000000000122
[    9.688595] raw: 0000000000000000 0000000080400040 00000000f5000000 0000000000000000
[    9.696560] page dumped because: kasan: bad access detected

[    9.703854] Memory state around the buggy address:
[    9.708796]  ffff5c7440d1cb00: fa fb fb fb fc fc fc fc fa fb fb fb fc fc fc fc
[    9.716227]  ffff5c7440d1cb80: fa fb fb fb fc fc fc fc fa fb fb fb fc fc fc fc
[    9.723660] >ffff5c7440d1cc00: 00 00 06 fc fc fc fc fc fa fb fb fb fc fc fc fc
[    9.731092]                          ^
[    9.734959]  ffff5c7440d1cc80: fa fb fb fb fc fc fc fc 00 00 06 fc fc fc fc fc
[    9.742392]  ffff5c7440d1cd00: 00 00 06 fc fc fc fc fc fa fb fb fb fc fc fc fc
[    9.749824] ==================================================================


I personally still struggle to understand how this driver could plausibly be
useful when the software crypto has no issues, is much faster, and is much
better tested.  What is motivating having this driver in the kernel?

- Eric

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[Bartosz Golaszewski]

On Sat, Jan 18, 2025 at 9:06 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Dec 03, 2024 at 10:10:13AM -0500, Bartosz Golaszewski wrote:
> > On Tue, 3 Dec 2024 14:53:21 +0100, neil.armstrong@linaro.org said:
> > > On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> > >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >>
> > >> Having switched to workqueue from tasklet, we are no longer limited to
> > >> atomic APIs and can now convert the spinlock to a mutex. This, along
> > >> with the conversion from tasklet to workqueue grants us ~15% improvement
> > >> in cryptsetup benchmarks for AES encryption.
> > >
> > > Can you share on which platforms you did the tests and the results you got ?
> > >
> >
> > Sure, I tested on sm8650 with the following results (they vary from
> > one run to other but are more or less in this range):
>
> FYI, when I test this driver on sm8650 with linux-next there are lots of test
> failures, including ones where the driver straight out returns wrong hashes:
>
> [    6.330656] alg: skcipher: xts-aes-qce setkey failed on test vector 0; expected_error=0, actual_error=-126, flags=0x1
> [    6.347196] alg: self-tests for xts(aes) using xts-aes-qce failed (rc=-126)
> [    6.347200] alg: self-tests for xts(aes) using xts-aes-qce failed (rc=-126)
> [    6.784951] alg: skcipher: ctr-aes-qce encryption test failed (wrong output IV) on test vector 4, cfg="in-place (one sglist)"
> [    6.810709] alg: self-tests for ctr(aes) using ctr-aes-qce failed (rc=-22)
> [    6.941639] alg: skcipher: cbc-3des-qce setkey failed on test vector "random: len=16 klen=24"; expected_error=0, actual_error=-126, flags=0x1
> [    6.947029] alg: self-tests for ctr(aes) using ctr-aes-qce failed (rc=-22)
> [    6.954394] alg: self-tests for cbc(des3_ede) using cbc-3des-qce failed (rc=-126)
> [    6.975348] alg: self-tests for cbc(des3_ede) using cbc-3des-qce failed (rc=-126)
> [    7.454433] alg: skcipher: ecb-3des-qce setkey failed on test vector "random: len=32 klen=24"; expected_error=0, actual_error=-126, flags=0x1
> [    7.454482] alg: self-tests for ecb(des3_ede) using ecb-3des-qce failed (rc=-126)
> [    7.454493] alg: self-tests for ecb(des3_ede) using ecb-3des-qce failed (rc=-126)
> [    8.593828] alg: ahash: hmac-sha256-qce test failed (wrong result) on test vector "random: psize=0 ksize=80", cfg="random: may_sleep use_finup src_divs=[<reimport>100.0%@+1800] key_offset=7"
> [    8.627337] alg: self-tests for hmac(sha256) using hmac-sha256-qce failed (rc=-22)
> [    8.639889] alg: self-tests for hmac(sha256) using hmac-sha256-qce failed (rc=-22)
> [    8.933595] alg: ahash: hmac-sha1-qce test failed (wrong result) on test vector "random: psize=0 ksize=56", cfg="random: inplace_one_sglist use_finup nosimd_setkey src_divs=[100.0%@+3969] key_offset=71"
> [    8.952885] alg: self-tests for hmac(sha1) using hmac-sha1-qce failed (rc=-22)
> [    8.965096] alg: self-tests for hmac(sha1) using hmac-sha1-qce failed (rc=-22)
>

I was testing with kcapi-speed and cryptsetup benchmark. I've never
seen any errors.

Is this after my changes only or did it exist before? You're testing
with the tcrypt module? How are you inserting it exactly? What params?

> Also a KASAN error:
>
> [    9.420862] CPU: 3 UID: 0 PID: 393 Comm: cryptomgr_test Tainted: G S      W          6.13.0-rc7-next-20250117-00007-g58182eb6d73d #12
> [    9.420881] Tainted: [S]=CPU_OUT_OF_SPEC, [W]=WARN
> [    9.420886] Hardware name: Qualcomm Technologies, Inc. SM8650 HDK (DT)
> [    9.420891] Call trace:
> [    9.420895]  show_stack+0x18/0x24 (C)
> [    9.420918]  dump_stack_lvl+0x60/0x80
> [    9.420937]  print_report+0x17c/0x4d0
> [    9.420960]  kasan_report+0xb0/0xf8
> [    9.420983]  kasan_check_range+0x100/0x1a8
> [    9.421001]  __asan_memcpy+0x3c/0xa0
> [    9.421020]  swiotlb_bounce+0x1b4/0x340
> [    9.421034]  swiotlb_tbl_map_single+0x2ac/0x5a0
> [    9.421050]  iommu_dma_map_page+0x2b8/0x4cc
> [    9.421063]  iommu_dma_map_sg+0x2e8/0xb3c
> [    9.421072]  __dma_map_sg_attrs+0x11c/0x1b0
> [    9.421086]  dma_map_sg_attrs+0x10/0x20
> [    9.421097]  qce_aead_async_req_handle+0x784/0x1aa0
> [    9.421125]  qce_handle_queue+0x20c/0x3e8
> [    9.421139]  qce_async_request_enqueue+0x10/0x1c
> [    9.421153]  qce_aead_crypt+0x1f0/0x81c
> [    9.421168]  qce_aead_encrypt+0x14/0x20
> [    9.421182]  crypto_aead_encrypt+0xa0/0xe0
> [    9.421194]  test_aead_vec_cfg+0x84c/0x1be8
> [    9.421207]  test_aead_vs_generic_impl+0x530/0x850
> [    9.421219]  alg_test_aead+0x7c8/0xe40
> [    9.421230]  alg_test+0x1f4/0xb0c
> [    9.421241]  cryptomgr_test+0x50/0x80
> [    9.421251]  kthread+0x378/0x678
> [    9.421272]  ret_from_fork+0x10/0x20
>
> [    9.550765] Allocated by task 393:
> [    9.554279]  kasan_save_stack+0x3c/0x64
> [    9.558252]  kasan_save_track+0x20/0x40
> [    9.562221]  kasan_save_alloc_info+0x40/0x54
> [    9.566641]  __kasan_kmalloc+0xb8/0xbc
> [    9.570515]  __kmalloc_noprof+0x188/0x410
> [    9.574669]  qce_aead_async_req_handle+0xbd4/0x1aa0
> [    9.579701]  qce_handle_queue+0x20c/0x3e8
> [    9.583841]  qce_async_request_enqueue+0x10/0x1c
> [    9.588607]  qce_aead_crypt+0x1f0/0x81c
> [    9.592577]  qce_aead_encrypt+0x14/0x20
> [    9.596547]  crypto_aead_encrypt+0xa0/0xe0
> [    9.600776]  test_aead_vec_cfg+0x84c/0x1be8
> [    9.605097]  test_aead_vs_generic_impl+0x530/0x850
> [    9.610039]  alg_test_aead+0x7c8/0xe40
> [    9.613911]  alg_test+0x1f4/0xb0c
> [    9.617345]  cryptomgr_test+0x50/0x80
> [    9.621124]  kthread+0x378/0x678
> [    9.624473]  ret_from_fork+0x10/0x20
>
> [    9.629738] The buggy address belongs to the object at ffff5c7440d1cc00
>                 which belongs to the cache kmalloc-32 of size 32
> [    9.642426] The buggy address is located 0 bytes inside of
>                 allocated 22-byte region [ffff5c7440d1cc00, ffff5c7440d1cc16)
>
> [    9.656672] The buggy address belongs to the physical page:
> [    9.662409] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x880d1c
> [    9.670646] flags: 0xbfffe0000000000(node=0|zone=2|lastcpupid=0x1ffff)
> [    9.677371] page_type: f5(slab)
> [    9.680624] raw: 0bfffe0000000000 ffff5c7440002780 dead000000000100 dead000000000122
> [    9.688595] raw: 0000000000000000 0000000080400040 00000000f5000000 0000000000000000
> [    9.696560] page dumped because: kasan: bad access detected
>
> [    9.703854] Memory state around the buggy address:
> [    9.708796]  ffff5c7440d1cb00: fa fb fb fb fc fc fc fc fa fb fb fb fc fc fc fc
> [    9.716227]  ffff5c7440d1cb80: fa fb fb fb fc fc fc fc fa fb fb fb fc fc fc fc
> [    9.723660] >ffff5c7440d1cc00: 00 00 06 fc fc fc fc fc fa fb fb fb fc fc fc fc
> [    9.731092]                          ^
> [    9.734959]  ffff5c7440d1cc80: fa fb fb fb fc fc fc fc 00 00 06 fc fc fc fc fc
> [    9.742392]  ffff5c7440d1cd00: 00 00 06 fc fc fc fc fc fa fb fb fb fc fc fc fc
> [    9.749824] ==================================================================
>

Hmm, I am running with KASAN on, never seen this How would I run
tcrypt to trigger it? By default the module doesn't load with no
params and gives me:

modprobe: ERROR: could not insert 'tcrypt': Resource temporarily unavailable

>
> I personally still struggle to understand how this driver could plausibly be
> useful when the software crypto has no issues, is much faster, and is much
> better tested.  What is motivating having this driver in the kernel?

We want to use it in conjunction with the upcoming scminvoke (for
loading TAs and invoking objects - used to program the keys into the
QCE) to support the DRM use-case for decrypting streaming data inside
secure buffers upstream.

Bart

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[Eric Biggers]

On Sat, Jan 18, 2025 at 10:28:26AM +0100, Bartosz Golaszewski wrote:
> I was testing with kcapi-speed and cryptsetup benchmark. I've never
> seen any errors.
> 
> Is this after my changes only or did it exist before? You're testing
> with the tcrypt module? How are you inserting it exactly? What params?

Those are all benchmarks, not tests.  The tests run at registration time if you
just enable the kconfig options for them:

    # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
    CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y

The test failures and KASAN error occur on mainline too, so yes they occur
before your patchset too.

> >
> > I personally still struggle to understand how this driver could plausibly be
> > useful when the software crypto has no issues, is much faster, and is much
> > better tested.  What is motivating having this driver in the kernel?
> 
> We want to use it in conjunction with the upcoming scminvoke (for
> loading TAs and invoking objects - used to program the keys into the
> QCE) to support the DRM use-case for decrypting streaming data inside
> secure buffers upstream.

Notably lacking is any claim that any of the current features of the driver are
actually useful.

- Eric

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[Bartosz Golaszewski]

On Sat, Jan 18, 2025 at 6:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sat, Jan 18, 2025 at 10:28:26AM +0100, Bartosz Golaszewski wrote:
> > I was testing with kcapi-speed and cryptsetup benchmark. I've never
> > seen any errors.
> >
> > Is this after my changes only or did it exist before? You're testing
> > with the tcrypt module? How are you inserting it exactly? What params?
>
> Those are all benchmarks, not tests.  The tests run at registration time if you
> just enable the kconfig options for them:
>
>     # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
>     CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
>
> The test failures and KASAN error occur on mainline too, so yes they occur
> before your patchset too.
>
> > >
> > > I personally still struggle to understand how this driver could plausibly be
> > > useful when the software crypto has no issues, is much faster, and is much
> > > better tested.  What is motivating having this driver in the kernel?
> >
> > We want to use it in conjunction with the upcoming scminvoke (for
> > loading TAs and invoking objects - used to program the keys into the
> > QCE) to support the DRM use-case for decrypting streaming data inside
> > secure buffers upstream.
>
> Notably lacking is any claim that any of the current features of the driver are
> actually useful.
>

Noted. I'm still quite early into working on the upstream-bound code
supporting the streaming use-case but I will consider a proposal to
remove existing features that are better provided by ARM CE.

Thanks,
Bartosz

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[Bartosz Golaszewski]

On Mon, Jan 20, 2025 at 2:46 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Sat, Jan 18, 2025 at 6:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Sat, Jan 18, 2025 at 10:28:26AM +0100, Bartosz Golaszewski wrote:
> > > I was testing with kcapi-speed and cryptsetup benchmark. I've never
> > > seen any errors.
> > >
> > > Is this after my changes only or did it exist before? You're testing
> > > with the tcrypt module? How are you inserting it exactly? What params?
> >
> > Those are all benchmarks, not tests.  The tests run at registration time if you
> > just enable the kconfig options for them:
> >
> >     # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
> >     CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
> >
> > The test failures and KASAN error occur on mainline too, so yes they occur
> > before your patchset too.
> >
> > > >
> > > > I personally still struggle to understand how this driver could plausibly be
> > > > useful when the software crypto has no issues, is much faster, and is much
> > > > better tested.  What is motivating having this driver in the kernel?
> > >
> > > We want to use it in conjunction with the upcoming scminvoke (for
> > > loading TAs and invoking objects - used to program the keys into the
> > > QCE) to support the DRM use-case for decrypting streaming data inside
> > > secure buffers upstream.
> >
> > Notably lacking is any claim that any of the current features of the driver are
> > actually useful.
> >
>
> Noted. I'm still quite early into working on the upstream-bound code
> supporting the streaming use-case but I will consider a proposal to
> remove existing features that are better provided by ARM CE.
>
> Thanks,
> Bartosz

Just an FYI, I was informed by Qualcomm that upcoming platforms will
contain an upgrade to this IP and it will be up to 3x faster than ARM
CE. In this case we'll keep this driver around and I will focus on
fixing existing issues.

Bart

Re: [PATCH 9/9] crypto: qce - switch to using a mutex

[Eric Biggers]

On Thu, Feb 20, 2025 at 10:14:20AM +0100, Bartosz Golaszewski wrote:
> On Mon, Jan 20, 2025 at 2:46 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Sat, Jan 18, 2025 at 6:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Sat, Jan 18, 2025 at 10:28:26AM +0100, Bartosz Golaszewski wrote:
> > > > I was testing with kcapi-speed and cryptsetup benchmark. I've never
> > > > seen any errors.
> > > >
> > > > Is this after my changes only or did it exist before? You're testing
> > > > with the tcrypt module? How are you inserting it exactly? What params?
> > >
> > > Those are all benchmarks, not tests.  The tests run at registration time if you
> > > just enable the kconfig options for them:
> > >
> > >     # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
> > >     CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y
> > >
> > > The test failures and KASAN error occur on mainline too, so yes they occur
> > > before your patchset too.
> > >
> > > > >
> > > > > I personally still struggle to understand how this driver could plausibly be
> > > > > useful when the software crypto has no issues, is much faster, and is much
> > > > > better tested.  What is motivating having this driver in the kernel?
> > > >
> > > > We want to use it in conjunction with the upcoming scminvoke (for
> > > > loading TAs and invoking objects - used to program the keys into the
> > > > QCE) to support the DRM use-case for decrypting streaming data inside
> > > > secure buffers upstream.
> > >
> > > Notably lacking is any claim that any of the current features of the driver are
> > > actually useful.
> > >
> >
> > Noted. I'm still quite early into working on the upstream-bound code
> > supporting the streaming use-case but I will consider a proposal to
> > remove existing features that are better provided by ARM CE.
> >
> > Thanks,
> > Bartosz
> 
> Just an FYI, I was informed by Qualcomm that upcoming platforms will
> contain an upgrade to this IP and it will be up to 3x faster than ARM
> CE.

I suspect that is measured under some ideal condition that won't be reached in
the real world, but we'll see.

- Eric

Re: [PATCH 0/9] crypto: qce - refactor the driver

[Eric Biggers]

On Tue, Dec 03, 2024 at 10:19:28AM +0100, Bartosz Golaszewski wrote:
> This driver will soon be getting more features so show it some 
> refactoring love in the meantime. Switching to using a workqueue and 
> sleeping locks improves cryptsetup benchmark results for AES encryption.

What is motivating this work?  I thought this driver is useless because ARMv8 CE
is an order of magnitude faster.

- Eric

Re: [PATCH 0/9] crypto: qce - refactor the driver

[Eric Biggers]

On Tue, Dec 03, 2024 at 09:35:03AM -0800, Eric Biggers wrote:
> On Tue, Dec 03, 2024 at 10:19:28AM +0100, Bartosz Golaszewski wrote:
> > This driver will soon be getting more features so show it some 
> > refactoring love in the meantime. Switching to using a workqueue and 
> > sleeping locks improves cryptsetup benchmark results for AES encryption.
> 
> What is motivating this work?  I thought this driver is useless because ARMv8 CE
> is an order of magnitude faster.
> 

I see what might be happening.  This driver registers its algorithms with too
high of a priority, which makes it get used when it shouldn't be.  I've sent out
a patch to fix that.

- Eric

Re: [PATCH 0/9] crypto: qce - refactor the driver

[Bartosz Golaszewski]

On Tue, Dec 3, 2024 at 6:35 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Dec 03, 2024 at 10:19:28AM +0100, Bartosz Golaszewski wrote:
> > This driver will soon be getting more features so show it some
> > refactoring love in the meantime. Switching to using a workqueue and
> > sleeping locks improves cryptsetup benchmark results for AES encryption.
>
> What is motivating this work?  I thought this driver is useless because ARMv8 CE
> is an order of magnitude faster.
>

We'll be extending support for this IP with some additional features
soon like using device unique keys etc. This is just some refreshment
of this module.

Bartosz

Re: [PATCH 0/9] crypto: qce - refactor the driver

[Herbert Xu]

On Tue, Dec 03, 2024 at 10:19:28AM +0100, Bartosz Golaszewski wrote:
> This driver will soon be getting more features so show it some 
> refactoring love in the meantime. Switching to using a workqueue and 
> sleeping locks improves cryptsetup benchmark results for AES encryption.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> Bartosz Golaszewski (9):
>       crypto: qce - fix goto jump in error path
>       crypto: qce - unregister previously registered algos in error path
>       crypto: qce - remove unneeded call to icc_set_bw() in error path
>       crypto: qce - shrink code with devres clk helpers
>       crypto: qce - convert qce_dma_request() to use devres
>       crypto: qce - make qce_register_algs() a managed interface
>       crypto: qce - use __free() for a buffer that's always freed
>       crypto: qce - convert tasklet to workqueue
>       crypto: qce - switch to using a mutex
> 
>  drivers/crypto/qce/core.c | 131 ++++++++++++++++------------------------------
>  drivers/crypto/qce/core.h |   9 ++--
>  drivers/crypto/qce/dma.c  |  22 ++++----
>  drivers/crypto/qce/dma.h  |   3 +-
>  drivers/crypto/qce/sha.c  |   6 +--
>  5 files changed, 68 insertions(+), 103 deletions(-)
> ---
> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
> change-id: 20241128-crypto-qce-refactor-ab58869eec34

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