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,
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,
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
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)
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
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
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
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
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
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
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.
linux-stable-mirror@lists.linaro.org
-
Bartosz Golaszewski -
Eric Biggers -
Herbert Xu -
neil.armstrong＠linaro.org