This set combines a set of fixes in the QAT driver related to the core
state machines and the change in state that can be triggered through
sysfs.
Here is a summary of the changes:
* Patch #1 resolves a bug that prevents resources to be freed up.
* Patch #2 is a simple cleanup.
* Patch #3 fix the behaviour of the command `up` triggered through a
write to /sys/bus/pci/devices/<BDF>/qat/state.
* Patches #4 and #5 fix a corner case in the un-registration of
algorithms in the state machines.
Giovanni Cabiddu (5):
crypto: qat - fix state machines cleanup paths
crypto: qat - do not shadow error code
crypto: qat - ignore subsequent state up commands
crypto: qat - fix unregistration of crypto algorithms
crypto: qat - fix unregistration of compression algorithms
.../intel/qat/qat_common/adf_common_drv.h | 2 ++
drivers/crypto/intel/qat/qat_common/adf_init.c | 17 ++++++++---------
drivers/crypto/intel/qat/qat_common/adf_sysfs.c | 15 ++++++++++++---
3 files changed, 22 insertions(+), 12 deletions(-)
base-commit: be369945f2f612c40f771fe265db1ca658cdc0d1
--
2.41.0
If the device is already in the up state, a subsequent write of `up` to
the sysfs attribute /sys/bus/pci/devices/<BDF>/qat/state brings the
device down.
Fix this behaviour by ignoring subsequent `up` commands if the device is
already in the up state.
Fixes: 1bdc85550a2b ("crypto: qat - fix concurrency issue when device state changes")
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Adam Guerin <[email protected]>
---
drivers/crypto/intel/qat/qat_common/adf_sysfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/intel/qat/qat_common/adf_sysfs.c b/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
index 5e14c374ebd3..8672cfa2800f 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
@@ -68,7 +68,9 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
dev_info(dev, "Starting device qat_dev%d\n", accel_id);
ret = adf_dev_up(accel_dev, true);
- if (ret < 0) {
+ if (ret == -EALREADY) {
+ break;
+ } else if (ret) {
dev_err(dev, "Failed to start device qat_dev%d\n",
accel_id);
adf_dev_down(accel_dev, true);
--
2.41.0
The function adf_dev_init(), through the subsystem qat_crypto, populates
the list of list of crypto instances accel_dev->crypto_list.
If the list of instances is not empty, the function adf_dev_start() will
then call qat_algs_registers() and qat_asym_algs_register() to register
the crypto algorithms into the crypto framework.
If any of the functions in adf_dev_start() fail, the caller of such
function, in the error path calls adf_dev_down() which in turn call
adf_dev_stop() and adf_dev_shutdown(), see for example the function
state_store in adf_sriov.c.
However, if the registration of crypto algorithms is not done,
adf_dev_stop() will try to unregister the algorithms regardless.
This might cause the counter active_devs in qat_algs.c and
qat_asym_algs.c to get to a negative value.
Add a new state, ADF_STATUS_CRYPTO_ALGS_REGISTERED, which tracks if the
crypto algorithms are registered into the crypto framework. Then use
this to unregister the algorithms if such flag is set. This ensures that
the crypto algorithms are only unregistered if previously registered.
Fixes: d8cba25d2c68 ("crypto: qat - Intel(R) QAT driver framework")
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Adam Guerin <[email protected]>
---
drivers/crypto/intel/qat/qat_common/adf_common_drv.h | 1 +
drivers/crypto/intel/qat/qat_common/adf_init.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/intel/qat/qat_common/adf_common_drv.h b/drivers/crypto/intel/qat/qat_common/adf_common_drv.h
index d4c602b4fec7..95efe0f26811 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_common_drv.h
+++ b/drivers/crypto/intel/qat/qat_common/adf_common_drv.h
@@ -25,6 +25,7 @@
#define ADF_STATUS_AE_STARTED 6
#define ADF_STATUS_PF_RUNNING 7
#define ADF_STATUS_IRQ_ALLOCATED 8
+#define ADF_STATUS_CRYPTO_ALGS_REGISTERED 9
enum adf_dev_reset_mode {
ADF_DEV_RESET_ASYNC = 0,
diff --git a/drivers/crypto/intel/qat/qat_common/adf_init.c b/drivers/crypto/intel/qat/qat_common/adf_init.c
index 542318a5b771..37cf5ce99092 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_init.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_init.c
@@ -227,6 +227,7 @@ static int adf_dev_start(struct adf_accel_dev *accel_dev)
clear_bit(ADF_STATUS_STARTED, &accel_dev->status);
return -EFAULT;
}
+ set_bit(ADF_STATUS_CRYPTO_ALGS_REGISTERED, &accel_dev->status);
if (!list_empty(&accel_dev->compression_list) && qat_comp_algs_register()) {
dev_err(&GET_DEV(accel_dev),
@@ -267,10 +268,12 @@ static void adf_dev_stop(struct adf_accel_dev *accel_dev)
clear_bit(ADF_STATUS_STARTING, &accel_dev->status);
clear_bit(ADF_STATUS_STARTED, &accel_dev->status);
- if (!list_empty(&accel_dev->crypto_list)) {
+ if (!list_empty(&accel_dev->crypto_list) &&
+ test_bit(ADF_STATUS_CRYPTO_ALGS_REGISTERED, &accel_dev->status)) {
qat_algs_unregister();
qat_asym_algs_unregister();
}
+ clear_bit(ADF_STATUS_CRYPTO_ALGS_REGISTERED, &accel_dev->status);
if (!list_empty(&accel_dev->compression_list))
qat_comp_algs_unregister();
--
2.41.0
Do not shadow the return code from adf_dev_down() in the error path of
the DEV_DOWN command.
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Adam Guerin <[email protected]>
---
drivers/crypto/intel/qat/qat_common/adf_sysfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/intel/qat/qat_common/adf_sysfs.c b/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
index a8f33558d7cb..5e14c374ebd3 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
@@ -60,8 +60,8 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
}
ret = adf_dev_down(accel_dev, true);
- if (ret < 0)
- return -EINVAL;
+ if (ret)
+ return ret;
break;
case DEV_UP:
--
2.41.0
Commit 1bdc85550a2b ("crypto: qat - fix concurrency issue when device
state changes") introduced the function adf_dev_down() which wraps the
functions adf_dev_stop() and adf_dev_shutdown().
In a subsequent change, the sequence adf_dev_stop() followed by
adf_dev_shutdown() was then replaced across the driver with just a call
to the function adf_dev_down().
The functions adf_dev_stop() and adf_dev_shutdown() are called in error
paths to stop the accelerator and free up resources and can be called
even if the counterparts adf_dev_init() and adf_dev_start() did not
complete successfully.
However, the implementation of adf_dev_down() prevents the stop/shutdown
sequence if the device is found already down.
For example, if adf_dev_init() fails, the device status is not set as
started and therefore a call to adf_dev_down() won't be calling
adf_dev_shutdown() to undo what adf_dev_init() did.
Do not check if a device is started in adf_dev_down() but do the
equivalent check in adf_sysfs.c when handling a DEV_DOWN command from
the user.
Fixes: 2b60f79c7b81 ("crypto: qat - replace state machine calls")
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Adam Guerin <[email protected]>
---
drivers/crypto/intel/qat/qat_common/adf_init.c | 7 -------
drivers/crypto/intel/qat/qat_common/adf_sysfs.c | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/crypto/intel/qat/qat_common/adf_init.c b/drivers/crypto/intel/qat/qat_common/adf_init.c
index 79a81e25de97..542318a5b771 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_init.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_init.c
@@ -428,13 +428,6 @@ int adf_dev_down(struct adf_accel_dev *accel_dev, bool reconfig)
mutex_lock(&accel_dev->state_lock);
- if (!adf_dev_started(accel_dev)) {
- dev_info(&GET_DEV(accel_dev), "Device qat_dev%d already down\n",
- accel_dev->accel_id);
- ret = -EINVAL;
- goto out;
- }
-
if (reconfig) {
ret = adf_dev_shutdown_cache_cfg(accel_dev);
goto out;
diff --git a/drivers/crypto/intel/qat/qat_common/adf_sysfs.c b/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
index a74d2f930367..a8f33558d7cb 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_sysfs.c
@@ -52,6 +52,13 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
case DEV_DOWN:
dev_info(dev, "Stopping device qat_dev%d\n", accel_id);
+ if (!adf_dev_started(accel_dev)) {
+ dev_info(&GET_DEV(accel_dev), "Device qat_dev%d already down\n",
+ accel_id);
+
+ break;
+ }
+
ret = adf_dev_down(accel_dev, true);
if (ret < 0)
return -EINVAL;
--
2.41.0
On Thu, Sep 14, 2023 at 10:55:44AM +0100, Giovanni Cabiddu wrote:
> This set combines a set of fixes in the QAT driver related to the core
> state machines and the change in state that can be triggered through
> sysfs.
>
> Here is a summary of the changes:
> * Patch #1 resolves a bug that prevents resources to be freed up.
> * Patch #2 is a simple cleanup.
> * Patch #3 fix the behaviour of the command `up` triggered through a
> write to /sys/bus/pci/devices/<BDF>/qat/state.
> * Patches #4 and #5 fix a corner case in the un-registration of
> algorithms in the state machines.
>
> Giovanni Cabiddu (5):
> crypto: qat - fix state machines cleanup paths
> crypto: qat - do not shadow error code
> crypto: qat - ignore subsequent state up commands
> crypto: qat - fix unregistration of crypto algorithms
> crypto: qat - fix unregistration of compression algorithms
>
> .../intel/qat/qat_common/adf_common_drv.h | 2 ++
> drivers/crypto/intel/qat/qat_common/adf_init.c | 17 ++++++++---------
> drivers/crypto/intel/qat/qat_common/adf_sysfs.c | 15 ++++++++++++---
> 3 files changed, 22 insertions(+), 12 deletions(-)
>
>
> base-commit: be369945f2f612c40f771fe265db1ca658cdc0d1
> --
> 2.41.0
All applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt