linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] crypto: qat - fix concurrency related issues
@ 2023-02-27 20:55 Shashank Gupta
  2023-02-27 20:55 ` [PATCH 1/5] crypto: qat - delay sysfs initialization Shashank Gupta
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Shashank Gupta @ 2023-02-27 20:55 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, qat-linux, Shashank Gupta

This set fixes issues related to using unprotected QAT device state    
machine functions that might cause concurrency issues at the time of state  
transition.

The first patch fixes the QAT 4XXX device's unexpected behaviour that
might occur if the user changes the device state or configuration via
sysfs while the driver performing device bring-up. The sequence is changed
in the probe function now the sysfs attribute is created after the device
initialization.

The second patch fixes the concurrency issue in the sysfs `state` 
attribute if multiple processes change the state of the qat device in 
parallel. The change introduces the protected wrapper function 
adf_dev_up() and adf_dev_down() that protects the transition of the device
state. These are used in adf_sysfs.c instead of low-level state machine 
functions.

The third patch replaces the use of unsafe low-level device state machine 
function with its protected wrapper functions.

The forth patch refactor device restart logic by moving it into 
adf_dev_restart() which uses safe adf_dev_up() and adf_dev_down().

The fifth patch define state machine functions static as they are unsafe
to use for state transition now performed by safe adf_dev_up() and 
adf_dev_down().

Shashank Gupta (5):
  crypto: qat - delay sysfs initialization
  crypto: qat - fix concurrency issue when device state changes
  crypto: qat - replace state machine calls
  crypto: qat - refactor device restart logic
  crypto: qat - make state machine functions static

 drivers/crypto/qat/qat_4xxx/adf_drv.c             | 21 ++---
 drivers/crypto/qat/qat_c3xxx/adf_drv.c            | 17 +---
 drivers/crypto/qat/qat_c3xxxvf/adf_drv.c          | 13 +--
 drivers/crypto/qat/qat_c62x/adf_drv.c             | 17 +---
 drivers/crypto/qat/qat_c62xvf/adf_drv.c           | 13 +--
 drivers/crypto/qat/qat_common/adf_accel_devices.h |  1 +
 drivers/crypto/qat/qat_common/adf_aer.c           |  4 +-
 drivers/crypto/qat/qat_common/adf_common_drv.h    |  8 +-
 drivers/crypto/qat/qat_common/adf_ctl_drv.c       | 27 +++----
 drivers/crypto/qat/qat_common/adf_dev_mgr.c       |  2 +
 drivers/crypto/qat/qat_common/adf_init.c          | 96 ++++++++++++++++++++---
 drivers/crypto/qat/qat_common/adf_sriov.c         | 10 +--
 drivers/crypto/qat/qat_common/adf_sysfs.c         | 23 +-----
 drivers/crypto/qat/qat_common/adf_vf_isr.c        |  3 +-
 drivers/crypto/qat/qat_dh895xcc/adf_drv.c         | 17 +---
 drivers/crypto/qat/qat_dh895xccvf/adf_drv.c       | 13 +--
 16 files changed, 132 insertions(+), 153 deletions(-)

-- 
2.16.4


^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH 1/5] crypto: qat - delay sysfs initialization
  2023-02-27 20:55 [PATCH 0/5] crypto: qat - fix concurrency related issues Shashank Gupta
@ 2023-02-27 20:55 ` Shashank Gupta
  2023-02-27 20:55 ` [PATCH 2/5] crypto: qat - fix concurrency issue when device state changes Shashank Gupta
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Shashank Gupta @ 2023-02-27 20:55 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, qat-linux, Shashank Gupta

The function adf_sysfs_init() is used by qat_4xxx to create sysfs
attributes. This is called by the probe function before starting a
device. With this sequence, there might be a chance that the sysfs
entries for configuration might be changed by a user while the driver
is performing a device bring-up causing unexpected behaviors.

Delay the creation of sysfs entries after adf_dev_start().

Signed-off-by: Shashank Gupta <shashank.gupta@intel.com>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/qat/qat_4xxx/adf_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/qat/qat_4xxx/adf_drv.c b/drivers/crypto/qat/qat_4xxx/adf_drv.c
index b3a4c7b23864..f7fdb435a70e 100644
--- a/drivers/crypto/qat/qat_4xxx/adf_drv.c
+++ b/drivers/crypto/qat/qat_4xxx/adf_drv.c
@@ -411,10 +411,6 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_err_disable_aer;
 	}
 
-	ret = adf_sysfs_init(accel_dev);
-	if (ret)
-		goto out_err_disable_aer;
-
 	ret = hw_data->dev_config(accel_dev);
 	if (ret)
 		goto out_err_disable_aer;
@@ -427,6 +423,10 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto out_err_dev_stop;
 
+	ret = adf_sysfs_init(accel_dev);
+	if (ret)
+		goto out_err_dev_stop;
+
 	return ret;
 
 out_err_dev_stop:
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 2/5] crypto: qat - fix concurrency issue when device state changes
  2023-02-27 20:55 [PATCH 0/5] crypto: qat - fix concurrency related issues Shashank Gupta
  2023-02-27 20:55 ` [PATCH 1/5] crypto: qat - delay sysfs initialization Shashank Gupta
@ 2023-02-27 20:55 ` Shashank Gupta
  2023-02-27 20:55 ` [PATCH 3/5] crypto: qat - replace state machine calls Shashank Gupta
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Shashank Gupta @ 2023-02-27 20:55 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, qat-linux, Shashank Gupta

The sysfs `state` attribute is not protected against race conditions.
If multiple processes perform a device state transition on the same
device in parallel, unexpected behaviors might occur.

For transitioning the device state, adf_sysfs.c calls the functions
adf_dev_init(), adf_dev_start(), adf_dev_stop() and adf_dev_shutdown()
which are unprotected and interdependent on each other. To perform a
state transition, these functions needs to be called in a specific
order:
  * device up:   adf_dev_init() -> adf_dev_start()
  * device down: adf_dev_stop() -> adf_dev_shutdown()

This change introduces the functions adf_dev_up() and adf_dev_down()
which wrap the state machine functions and protect them with a
per-device lock. These are then used in adf_sysfs.c instead of the
individual state transition functions.

Fixes: 5ee52118ac14 ("crypto: qat - expose device state through sysfs for 4xxx")
Signed-off-by: Shashank Gupta <shashank.gupta@intel.com>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/qat/qat_common/adf_accel_devices.h |  1 +
 drivers/crypto/qat/qat_common/adf_common_drv.h    |  3 ++
 drivers/crypto/qat/qat_common/adf_dev_mgr.c       |  2 +
 drivers/crypto/qat/qat_common/adf_init.c          | 64 +++++++++++++++++++++++
 drivers/crypto/qat/qat_common/adf_sysfs.c         | 23 ++------
 5 files changed, 73 insertions(+), 20 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_accel_devices.h b/drivers/crypto/qat/qat_common/adf_accel_devices.h
index 284f5aad3ee0..7be933d6f0ff 100644
--- a/drivers/crypto/qat/qat_common/adf_accel_devices.h
+++ b/drivers/crypto/qat/qat_common/adf_accel_devices.h
@@ -310,6 +310,7 @@ struct adf_accel_dev {
 			u8 pf_compat_ver;
 		} vf;
 	};
+	struct mutex state_lock; /* protect state of the device */
 	bool is_vf;
 	u32 accel_id;
 };
diff --git a/drivers/crypto/qat/qat_common/adf_common_drv.h b/drivers/crypto/qat/qat_common/adf_common_drv.h
index 7189265573c0..4bf1fceb7052 100644
--- a/drivers/crypto/qat/qat_common/adf_common_drv.h
+++ b/drivers/crypto/qat/qat_common/adf_common_drv.h
@@ -58,6 +58,9 @@ void adf_dev_stop(struct adf_accel_dev *accel_dev);
 void adf_dev_shutdown(struct adf_accel_dev *accel_dev);
 int adf_dev_shutdown_cache_cfg(struct adf_accel_dev *accel_dev);
 
+int adf_dev_up(struct adf_accel_dev *accel_dev, bool init_config);
+int adf_dev_down(struct adf_accel_dev *accel_dev, bool cache_config);
+
 void adf_devmgr_update_class_index(struct adf_hw_device_data *hw_data);
 void adf_clean_vf_map(bool);
 
diff --git a/drivers/crypto/qat/qat_common/adf_dev_mgr.c b/drivers/crypto/qat/qat_common/adf_dev_mgr.c
index 4c752eed10fe..86ee36feefad 100644
--- a/drivers/crypto/qat/qat_common/adf_dev_mgr.c
+++ b/drivers/crypto/qat/qat_common/adf_dev_mgr.c
@@ -223,6 +223,7 @@ int adf_devmgr_add_dev(struct adf_accel_dev *accel_dev,
 		map->attached = true;
 		list_add_tail(&map->list, &vfs_table);
 	}
+	mutex_init(&accel_dev->state_lock);
 unlock:
 	mutex_unlock(&table_lock);
 	return ret;
@@ -269,6 +270,7 @@ void adf_devmgr_rm_dev(struct adf_accel_dev *accel_dev,
 		}
 	}
 unlock:
+	mutex_destroy(&accel_dev->state_lock);
 	list_del(&accel_dev->list);
 	mutex_unlock(&table_lock);
 }
diff --git a/drivers/crypto/qat/qat_common/adf_init.c b/drivers/crypto/qat/qat_common/adf_init.c
index cef7bb8ec007..988cffd0b833 100644
--- a/drivers/crypto/qat/qat_common/adf_init.c
+++ b/drivers/crypto/qat/qat_common/adf_init.c
@@ -400,3 +400,67 @@ int adf_dev_shutdown_cache_cfg(struct adf_accel_dev *accel_dev)
 
 	return 0;
 }
+
+int adf_dev_down(struct adf_accel_dev *accel_dev, bool reconfig)
+{
+	int ret = 0;
+
+	if (!accel_dev)
+		return -EINVAL;
+
+	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;
+	}
+
+	adf_dev_stop(accel_dev);
+	adf_dev_shutdown(accel_dev);
+
+out:
+	mutex_unlock(&accel_dev->state_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(adf_dev_down);
+
+int adf_dev_up(struct adf_accel_dev *accel_dev, bool config)
+{
+	int ret = 0;
+
+	if (!accel_dev)
+		return -EINVAL;
+
+	mutex_lock(&accel_dev->state_lock);
+
+	if (adf_dev_started(accel_dev)) {
+		dev_info(&GET_DEV(accel_dev), "Device qat_dev%d already up\n",
+			 accel_dev->accel_id);
+		ret = -EALREADY;
+		goto out;
+	}
+
+	if (config && GET_HW_DATA(accel_dev)->dev_config) {
+		ret = GET_HW_DATA(accel_dev)->dev_config(accel_dev);
+		if (unlikely(ret))
+			goto out;
+	}
+
+	ret = adf_dev_init(accel_dev);
+	if (unlikely(ret))
+		goto out;
+
+	ret = adf_dev_start(accel_dev);
+
+out:
+	mutex_unlock(&accel_dev->state_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(adf_dev_up);
diff --git a/drivers/crypto/qat/qat_common/adf_sysfs.c b/drivers/crypto/qat/qat_common/adf_sysfs.c
index e8b078e719c2..3eb6611ab1b1 100644
--- a/drivers/crypto/qat/qat_common/adf_sysfs.c
+++ b/drivers/crypto/qat/qat_common/adf_sysfs.c
@@ -50,38 +50,21 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr,
 
 	switch (ret) {
 	case DEV_DOWN:
-		if (!adf_dev_started(accel_dev)) {
-			dev_info(dev, "Device qat_dev%d already down\n",
-				 accel_id);
-			return -EINVAL;
-		}
-
 		dev_info(dev, "Stopping device qat_dev%d\n", accel_id);
 
-		ret = adf_dev_shutdown_cache_cfg(accel_dev);
+		ret = adf_dev_down(accel_dev, true);
 		if (ret < 0)
 			return -EINVAL;
 
 		break;
 	case DEV_UP:
-		if (adf_dev_started(accel_dev)) {
-			dev_info(dev, "Device qat_dev%d already up\n",
-				 accel_id);
-			return -EINVAL;
-		}
-
 		dev_info(dev, "Starting device qat_dev%d\n", accel_id);
 
-		ret = GET_HW_DATA(accel_dev)->dev_config(accel_dev);
-		if (!ret)
-			ret = adf_dev_init(accel_dev);
-		if (!ret)
-			ret = adf_dev_start(accel_dev);
-
+		ret = adf_dev_up(accel_dev, true);
 		if (ret < 0) {
 			dev_err(dev, "Failed to start device qat_dev%d\n",
 				accel_id);
-			adf_dev_shutdown_cache_cfg(accel_dev);
+			adf_dev_down(accel_dev, true);
 			return ret;
 		}
 		break;
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 3/5] crypto: qat - replace state machine calls
  2023-02-27 20:55 [PATCH 0/5] crypto: qat - fix concurrency related issues Shashank Gupta
  2023-02-27 20:55 ` [PATCH 1/5] crypto: qat - delay sysfs initialization Shashank Gupta
  2023-02-27 20:55 ` [PATCH 2/5] crypto: qat - fix concurrency issue when device state changes Shashank Gupta
@ 2023-02-27 20:55 ` Shashank Gupta
  2023-02-27 20:55 ` [PATCH 4/5] crypto: qat - refactor device restart logic Shashank Gupta
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Shashank Gupta @ 2023-02-27 20:55 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, qat-linux, Shashank Gupta

The device state machine functions are unsafe and interdependent on each
other. To perform a state transition, these shall be called in a
specific order:
  * device up:   adf_dev_init() -> adf_dev_start()
  * device down: adf_dev_stop() -> adf_dev_shutdown()

Replace all the state machine functions used in the QAT driver with the
safe wrappers adf_dev_up() and adf_dev_down().

Signed-off-by: Shashank Gupta <shashank.gupta@intel.com>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/qat/qat_4xxx/adf_drv.c       | 17 +++--------------
 drivers/crypto/qat/qat_c3xxx/adf_drv.c      | 17 +++--------------
 drivers/crypto/qat/qat_c3xxxvf/adf_drv.c    | 13 +++----------
 drivers/crypto/qat/qat_c62x/adf_drv.c       | 17 +++--------------
 drivers/crypto/qat/qat_c62xvf/adf_drv.c     | 13 +++----------
 drivers/crypto/qat/qat_common/adf_ctl_drv.c | 27 +++++++++------------------
 drivers/crypto/qat/qat_common/adf_sriov.c   | 10 ++--------
 drivers/crypto/qat/qat_common/adf_vf_isr.c  |  3 +--
 drivers/crypto/qat/qat_dh895xcc/adf_drv.c   | 17 +++--------------
 drivers/crypto/qat/qat_dh895xccvf/adf_drv.c | 13 +++----------
 10 files changed, 33 insertions(+), 114 deletions(-)

diff --git a/drivers/crypto/qat/qat_4xxx/adf_drv.c b/drivers/crypto/qat/qat_4xxx/adf_drv.c
index f7fdb435a70e..6f862b56c51c 100644
--- a/drivers/crypto/qat/qat_4xxx/adf_drv.c
+++ b/drivers/crypto/qat/qat_4xxx/adf_drv.c
@@ -411,15 +411,7 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_err_disable_aer;
 	}
 
-	ret = hw_data->dev_config(accel_dev);
-	if (ret)
-		goto out_err_disable_aer;
-
-	ret = adf_dev_init(accel_dev);
-	if (ret)
-		goto out_err_dev_shutdown;
-
-	ret = adf_dev_start(accel_dev);
+	ret = adf_dev_up(accel_dev, true);
 	if (ret)
 		goto out_err_dev_stop;
 
@@ -430,9 +422,7 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return ret;
 
 out_err_dev_stop:
-	adf_dev_stop(accel_dev);
-out_err_dev_shutdown:
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 out_err_disable_aer:
 	adf_disable_aer(accel_dev);
 out_err:
@@ -448,8 +438,7 @@ static void adf_remove(struct pci_dev *pdev)
 		pr_err("QAT: Driver removal failed\n");
 		return;
 	}
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 	adf_disable_aer(accel_dev);
 	adf_cleanup_accel(accel_dev);
 }
diff --git a/drivers/crypto/qat/qat_c3xxx/adf_drv.c b/drivers/crypto/qat/qat_c3xxx/adf_drv.c
index 1f4fbf4562b2..4c00c4933805 100644
--- a/drivers/crypto/qat/qat_c3xxx/adf_drv.c
+++ b/drivers/crypto/qat/qat_c3xxx/adf_drv.c
@@ -201,24 +201,14 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_err_disable_aer;
 	}
 
-	ret = hw_data->dev_config(accel_dev);
-	if (ret)
-		goto out_err_disable_aer;
-
-	ret = adf_dev_init(accel_dev);
-	if (ret)
-		goto out_err_dev_shutdown;
-
-	ret = adf_dev_start(accel_dev);
+	ret = adf_dev_up(accel_dev, true);
 	if (ret)
 		goto out_err_dev_stop;
 
 	return ret;
 
 out_err_dev_stop:
-	adf_dev_stop(accel_dev);
-out_err_dev_shutdown:
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 out_err_disable_aer:
 	adf_disable_aer(accel_dev);
 out_err_free_reg:
@@ -239,8 +229,7 @@ static void adf_remove(struct pci_dev *pdev)
 		pr_err("QAT: Driver removal failed\n");
 		return;
 	}
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 	adf_disable_aer(accel_dev);
 	adf_cleanup_accel(accel_dev);
 	adf_cleanup_pci_dev(accel_dev);
diff --git a/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c b/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c
index cf4ef83e186f..e8cc10f64134 100644
--- a/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c
+++ b/drivers/crypto/qat/qat_c3xxxvf/adf_drv.c
@@ -173,20 +173,14 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Completion for VF2PF request/response message exchange */
 	init_completion(&accel_dev->vf.msg_received);
 
-	ret = adf_dev_init(accel_dev);
-	if (ret)
-		goto out_err_dev_shutdown;
-
-	ret = adf_dev_start(accel_dev);
+	ret = adf_dev_up(accel_dev, false);
 	if (ret)
 		goto out_err_dev_stop;
 
 	return ret;
 
 out_err_dev_stop:
-	adf_dev_stop(accel_dev);
-out_err_dev_shutdown:
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 out_err_free_reg:
 	pci_release_regions(accel_pci_dev->pci_dev);
 out_err_disable:
@@ -206,8 +200,7 @@ static void adf_remove(struct pci_dev *pdev)
 		return;
 	}
 	adf_flush_vf_wq(accel_dev);
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 	adf_cleanup_accel(accel_dev);
 	adf_cleanup_pci_dev(accel_dev);
 	kfree(accel_dev);
diff --git a/drivers/crypto/qat/qat_c62x/adf_drv.c b/drivers/crypto/qat/qat_c62x/adf_drv.c
index 4ccaf298250c..fcb2f5b8e053 100644
--- a/drivers/crypto/qat/qat_c62x/adf_drv.c
+++ b/drivers/crypto/qat/qat_c62x/adf_drv.c
@@ -201,24 +201,14 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_err_disable_aer;
 	}
 
-	ret = hw_data->dev_config(accel_dev);
-	if (ret)
-		goto out_err_disable_aer;
-
-	ret = adf_dev_init(accel_dev);
-	if (ret)
-		goto out_err_dev_shutdown;
-
-	ret = adf_dev_start(accel_dev);
+	ret = adf_dev_up(accel_dev, true);
 	if (ret)
 		goto out_err_dev_stop;
 
 	return ret;
 
 out_err_dev_stop:
-	adf_dev_stop(accel_dev);
-out_err_dev_shutdown:
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 out_err_disable_aer:
 	adf_disable_aer(accel_dev);
 out_err_free_reg:
@@ -239,8 +229,7 @@ static void adf_remove(struct pci_dev *pdev)
 		pr_err("QAT: Driver removal failed\n");
 		return;
 	}
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 	adf_disable_aer(accel_dev);
 	adf_cleanup_accel(accel_dev);
 	adf_cleanup_pci_dev(accel_dev);
diff --git a/drivers/crypto/qat/qat_c62xvf/adf_drv.c b/drivers/crypto/qat/qat_c62xvf/adf_drv.c
index 0e642c94b929..37566309df94 100644
--- a/drivers/crypto/qat/qat_c62xvf/adf_drv.c
+++ b/drivers/crypto/qat/qat_c62xvf/adf_drv.c
@@ -173,20 +173,14 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Completion for VF2PF request/response message exchange */
 	init_completion(&accel_dev->vf.msg_received);
 
-	ret = adf_dev_init(accel_dev);
-	if (ret)
-		goto out_err_dev_shutdown;
-
-	ret = adf_dev_start(accel_dev);
+	ret = adf_dev_up(accel_dev, false);
 	if (ret)
 		goto out_err_dev_stop;
 
 	return ret;
 
 out_err_dev_stop:
-	adf_dev_stop(accel_dev);
-out_err_dev_shutdown:
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 out_err_free_reg:
 	pci_release_regions(accel_pci_dev->pci_dev);
 out_err_disable:
@@ -206,8 +200,7 @@ static void adf_remove(struct pci_dev *pdev)
 		return;
 	}
 	adf_flush_vf_wq(accel_dev);
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 	adf_cleanup_accel(accel_dev);
 	adf_cleanup_pci_dev(accel_dev);
 	kfree(accel_dev);
diff --git a/drivers/crypto/qat/qat_common/adf_ctl_drv.c b/drivers/crypto/qat/qat_common/adf_ctl_drv.c
index 9190532b27eb..b79ce4d0cc44 100644
--- a/drivers/crypto/qat/qat_common/adf_ctl_drv.c
+++ b/drivers/crypto/qat/qat_common/adf_ctl_drv.c
@@ -243,8 +243,7 @@ static void adf_ctl_stop_devices(u32 id)
 			if (!accel_dev->is_vf)
 				continue;
 
-			adf_dev_stop(accel_dev);
-			adf_dev_shutdown(accel_dev);
+			adf_dev_down(accel_dev, false);
 		}
 	}
 
@@ -253,8 +252,7 @@ static void adf_ctl_stop_devices(u32 id)
 			if (!adf_dev_started(accel_dev))
 				continue;
 
-			adf_dev_stop(accel_dev);
-			adf_dev_shutdown(accel_dev);
+			adf_dev_down(accel_dev, false);
 		}
 	}
 }
@@ -308,23 +306,16 @@ static int adf_ctl_ioctl_dev_start(struct file *fp, unsigned int cmd,
 	if (!accel_dev)
 		goto out;
 
-	if (!adf_dev_started(accel_dev)) {
-		dev_info(&GET_DEV(accel_dev),
-			 "Starting acceleration device qat_dev%d.\n",
-			 ctl_data->device_id);
-		ret = adf_dev_init(accel_dev);
-		if (!ret)
-			ret = adf_dev_start(accel_dev);
-	} else {
-		dev_info(&GET_DEV(accel_dev),
-			 "Acceleration device qat_dev%d already started.\n",
-			 ctl_data->device_id);
-	}
+	dev_info(&GET_DEV(accel_dev),
+		 "Starting acceleration device qat_dev%d.\n",
+		 ctl_data->device_id);
+
+	ret = adf_dev_up(accel_dev, false);
+
 	if (ret) {
 		dev_err(&GET_DEV(accel_dev), "Failed to start qat_dev%d\n",
 			ctl_data->device_id);
-		adf_dev_stop(accel_dev);
-		adf_dev_shutdown(accel_dev);
+		adf_dev_down(accel_dev, false);
 	}
 out:
 	kfree(ctl_data);
diff --git a/drivers/crypto/qat/qat_common/adf_sriov.c b/drivers/crypto/qat/qat_common/adf_sriov.c
index d85a90cc387b..f44025bb6f99 100644
--- a/drivers/crypto/qat/qat_common/adf_sriov.c
+++ b/drivers/crypto/qat/qat_common/adf_sriov.c
@@ -159,7 +159,7 @@ int adf_sriov_configure(struct pci_dev *pdev, int numvfs)
 			return -EBUSY;
 		}
 
-		ret = adf_dev_shutdown_cache_cfg(accel_dev);
+		ret = adf_dev_down(accel_dev, true);
 		if (ret)
 			return ret;
 	}
@@ -184,13 +184,7 @@ int adf_sriov_configure(struct pci_dev *pdev, int numvfs)
 	if (!accel_dev->pf.vf_info)
 		return -ENOMEM;
 
-	if (adf_dev_init(accel_dev)) {
-		dev_err(&GET_DEV(accel_dev), "Failed to init qat_dev%d\n",
-			accel_dev->accel_id);
-		return -EFAULT;
-	}
-
-	if (adf_dev_start(accel_dev)) {
+	if (adf_dev_up(accel_dev, false)) {
 		dev_err(&GET_DEV(accel_dev), "Failed to start qat_dev%d\n",
 			accel_dev->accel_id);
 		return -EFAULT;
diff --git a/drivers/crypto/qat/qat_common/adf_vf_isr.c b/drivers/crypto/qat/qat_common/adf_vf_isr.c
index 8c95fcd8e64b..b05c3957a160 100644
--- a/drivers/crypto/qat/qat_common/adf_vf_isr.c
+++ b/drivers/crypto/qat/qat_common/adf_vf_isr.c
@@ -71,8 +71,7 @@ static void adf_dev_stop_async(struct work_struct *work)
 	struct adf_accel_dev *accel_dev = stop_data->accel_dev;
 
 	adf_dev_restarting_notify(accel_dev);
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 
 	/* Re-enable PF2VF interrupts */
 	adf_enable_pf2vf_interrupts(accel_dev);
diff --git a/drivers/crypto/qat/qat_dh895xcc/adf_drv.c b/drivers/crypto/qat/qat_dh895xcc/adf_drv.c
index ebeb17b67fcd..4d27e4e43642 100644
--- a/drivers/crypto/qat/qat_dh895xcc/adf_drv.c
+++ b/drivers/crypto/qat/qat_dh895xcc/adf_drv.c
@@ -201,24 +201,14 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_err_disable_aer;
 	}
 
-	ret = hw_data->dev_config(accel_dev);
-	if (ret)
-		goto out_err_disable_aer;
-
-	ret = adf_dev_init(accel_dev);
-	if (ret)
-		goto out_err_dev_shutdown;
-
-	ret = adf_dev_start(accel_dev);
+	ret = adf_dev_up(accel_dev, true);
 	if (ret)
 		goto out_err_dev_stop;
 
 	return ret;
 
 out_err_dev_stop:
-	adf_dev_stop(accel_dev);
-out_err_dev_shutdown:
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 out_err_disable_aer:
 	adf_disable_aer(accel_dev);
 out_err_free_reg:
@@ -239,8 +229,7 @@ static void adf_remove(struct pci_dev *pdev)
 		pr_err("QAT: Driver removal failed\n");
 		return;
 	}
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 	adf_disable_aer(accel_dev);
 	adf_cleanup_accel(accel_dev);
 	adf_cleanup_pci_dev(accel_dev);
diff --git a/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c b/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c
index c1485e702b3e..96854a1cd87e 100644
--- a/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c
+++ b/drivers/crypto/qat/qat_dh895xccvf/adf_drv.c
@@ -173,20 +173,14 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Completion for VF2PF request/response message exchange */
 	init_completion(&accel_dev->vf.msg_received);
 
-	ret = adf_dev_init(accel_dev);
-	if (ret)
-		goto out_err_dev_shutdown;
-
-	ret = adf_dev_start(accel_dev);
+	ret = adf_dev_up(accel_dev, false);
 	if (ret)
 		goto out_err_dev_stop;
 
 	return ret;
 
 out_err_dev_stop:
-	adf_dev_stop(accel_dev);
-out_err_dev_shutdown:
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 out_err_free_reg:
 	pci_release_regions(accel_pci_dev->pci_dev);
 out_err_disable:
@@ -206,8 +200,7 @@ static void adf_remove(struct pci_dev *pdev)
 		return;
 	}
 	adf_flush_vf_wq(accel_dev);
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
+	adf_dev_down(accel_dev, false);
 	adf_cleanup_accel(accel_dev);
 	adf_cleanup_pci_dev(accel_dev);
 	kfree(accel_dev);
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 4/5] crypto: qat - refactor device restart logic
  2023-02-27 20:55 [PATCH 0/5] crypto: qat - fix concurrency related issues Shashank Gupta
                   ` (2 preceding siblings ...)
  2023-02-27 20:55 ` [PATCH 3/5] crypto: qat - replace state machine calls Shashank Gupta
@ 2023-02-27 20:55 ` Shashank Gupta
  2023-02-27 20:55 ` [PATCH 5/5] crypto: qat - make state machine functions static Shashank Gupta
  2023-03-10 11:31 ` [PATCH 0/5] crypto: qat - fix concurrency related issues Herbert Xu
  5 siblings, 0 replies; 7+ messages in thread
From: Shashank Gupta @ 2023-02-27 20:55 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, qat-linux, Shashank Gupta

Refactor the restart logic by moving it into the function
adf_dev_restart() which uses the safe function adf_dev_up() and
adf_dev_down().

This commit does not implement any functional change.

Signed-off-by: Shashank Gupta <shashank.gupta@intel.com>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/qat/qat_common/adf_aer.c        |  4 +---
 drivers/crypto/qat/qat_common/adf_common_drv.h |  1 +
 drivers/crypto/qat/qat_common/adf_init.c       | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_aer.c b/drivers/crypto/qat/qat_common/adf_aer.c
index fe9bb2f3536a..9fa76c527051 100644
--- a/drivers/crypto/qat/qat_common/adf_aer.c
+++ b/drivers/crypto/qat/qat_common/adf_aer.c
@@ -90,9 +90,7 @@ static void adf_device_reset_worker(struct work_struct *work)
 	struct adf_accel_dev *accel_dev = reset_data->accel_dev;
 
 	adf_dev_restarting_notify(accel_dev);
-	adf_dev_stop(accel_dev);
-	adf_dev_shutdown(accel_dev);
-	if (adf_dev_init(accel_dev) || adf_dev_start(accel_dev)) {
+	if (adf_dev_restart(accel_dev)) {
 		/* The device hanged and we can't restart it so stop here */
 		dev_err(&GET_DEV(accel_dev), "Restart device failed\n");
 		kfree(reset_data);
diff --git a/drivers/crypto/qat/qat_common/adf_common_drv.h b/drivers/crypto/qat/qat_common/adf_common_drv.h
index 4bf1fceb7052..3666109b6320 100644
--- a/drivers/crypto/qat/qat_common/adf_common_drv.h
+++ b/drivers/crypto/qat/qat_common/adf_common_drv.h
@@ -60,6 +60,7 @@ int adf_dev_shutdown_cache_cfg(struct adf_accel_dev *accel_dev);
 
 int adf_dev_up(struct adf_accel_dev *accel_dev, bool init_config);
 int adf_dev_down(struct adf_accel_dev *accel_dev, bool cache_config);
+int adf_dev_restart(struct adf_accel_dev *accel_dev);
 
 void adf_devmgr_update_class_index(struct adf_hw_device_data *hw_data);
 void adf_clean_vf_map(bool);
diff --git a/drivers/crypto/qat/qat_common/adf_init.c b/drivers/crypto/qat/qat_common/adf_init.c
index 988cffd0b833..11ade5d8e4a0 100644
--- a/drivers/crypto/qat/qat_common/adf_init.c
+++ b/drivers/crypto/qat/qat_common/adf_init.c
@@ -464,3 +464,21 @@ int adf_dev_up(struct adf_accel_dev *accel_dev, bool config)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(adf_dev_up);
+
+int adf_dev_restart(struct adf_accel_dev *accel_dev)
+{
+	int ret = 0;
+
+	if (!accel_dev)
+		return -EFAULT;
+
+	adf_dev_down(accel_dev, false);
+
+	ret = adf_dev_up(accel_dev, false);
+	/* if device is already up return success*/
+	if (ret == -EALREADY)
+		return 0;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(adf_dev_restart);
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 5/5] crypto: qat - make state machine functions static
  2023-02-27 20:55 [PATCH 0/5] crypto: qat - fix concurrency related issues Shashank Gupta
                   ` (3 preceding siblings ...)
  2023-02-27 20:55 ` [PATCH 4/5] crypto: qat - refactor device restart logic Shashank Gupta
@ 2023-02-27 20:55 ` Shashank Gupta
  2023-03-10 11:31 ` [PATCH 0/5] crypto: qat - fix concurrency related issues Herbert Xu
  5 siblings, 0 replies; 7+ messages in thread
From: Shashank Gupta @ 2023-02-27 20:55 UTC (permalink / raw)
  To: herbert; +Cc: linux-crypto, qat-linux, Shashank Gupta

The state machine functions adf_dev_init(), adf_dev_start(),
adf_dev_stop() adf_dev_shutdown() and adf_dev_shutdown_cache_cfg()
are only used internally within adf_init.c.
Do not export these functions and make them static as state transitions
are now performed using the safe function adf_dev_up() and
adf_dev_down().

This commit does not implement any functional change.

Signed-off-by: Shashank Gupta <shashank.gupta@intel.com>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/qat/qat_common/adf_common_drv.h |  6 ------
 drivers/crypto/qat/qat_common/adf_init.c       | 14 +++++---------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_common_drv.h b/drivers/crypto/qat/qat_common/adf_common_drv.h
index 3666109b6320..b2f14aaf6950 100644
--- a/drivers/crypto/qat/qat_common/adf_common_drv.h
+++ b/drivers/crypto/qat/qat_common/adf_common_drv.h
@@ -52,12 +52,6 @@ struct service_hndl {
 int adf_service_register(struct service_hndl *service);
 int adf_service_unregister(struct service_hndl *service);
 
-int adf_dev_init(struct adf_accel_dev *accel_dev);
-int adf_dev_start(struct adf_accel_dev *accel_dev);
-void adf_dev_stop(struct adf_accel_dev *accel_dev);
-void adf_dev_shutdown(struct adf_accel_dev *accel_dev);
-int adf_dev_shutdown_cache_cfg(struct adf_accel_dev *accel_dev);
-
 int adf_dev_up(struct adf_accel_dev *accel_dev, bool init_config);
 int adf_dev_down(struct adf_accel_dev *accel_dev, bool cache_config);
 int adf_dev_restart(struct adf_accel_dev *accel_dev);
diff --git a/drivers/crypto/qat/qat_common/adf_init.c b/drivers/crypto/qat/qat_common/adf_init.c
index 11ade5d8e4a0..0985f64ab11a 100644
--- a/drivers/crypto/qat/qat_common/adf_init.c
+++ b/drivers/crypto/qat/qat_common/adf_init.c
@@ -56,7 +56,7 @@ int adf_service_unregister(struct service_hndl *service)
  *
  * Return: 0 on success, error code otherwise.
  */
-int adf_dev_init(struct adf_accel_dev *accel_dev)
+static int adf_dev_init(struct adf_accel_dev *accel_dev)
 {
 	struct service_hndl *service;
 	struct list_head *list_itr;
@@ -146,7 +146,6 @@ int adf_dev_init(struct adf_accel_dev *accel_dev)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(adf_dev_init);
 
 /**
  * adf_dev_start() - Start acceleration service for the given accel device
@@ -158,7 +157,7 @@ EXPORT_SYMBOL_GPL(adf_dev_init);
  *
  * Return: 0 on success, error code otherwise.
  */
-int adf_dev_start(struct adf_accel_dev *accel_dev)
+static int adf_dev_start(struct adf_accel_dev *accel_dev)
 {
 	struct adf_hw_device_data *hw_data = accel_dev->hw_device;
 	struct service_hndl *service;
@@ -219,7 +218,6 @@ int adf_dev_start(struct adf_accel_dev *accel_dev)
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(adf_dev_start);
 
 /**
  * adf_dev_stop() - Stop acceleration service for the given accel device
@@ -231,7 +229,7 @@ EXPORT_SYMBOL_GPL(adf_dev_start);
  *
  * Return: void
  */
-void adf_dev_stop(struct adf_accel_dev *accel_dev)
+static void adf_dev_stop(struct adf_accel_dev *accel_dev)
 {
 	struct service_hndl *service;
 	struct list_head *list_itr;
@@ -276,7 +274,6 @@ void adf_dev_stop(struct adf_accel_dev *accel_dev)
 			clear_bit(ADF_STATUS_AE_STARTED, &accel_dev->status);
 	}
 }
-EXPORT_SYMBOL_GPL(adf_dev_stop);
 
 /**
  * adf_dev_shutdown() - shutdown acceleration services and data strucutures
@@ -285,7 +282,7 @@ EXPORT_SYMBOL_GPL(adf_dev_stop);
  * Cleanup the ring data structures and the admin comms and arbitration
  * services.
  */
-void adf_dev_shutdown(struct adf_accel_dev *accel_dev)
+static void adf_dev_shutdown(struct adf_accel_dev *accel_dev)
 {
 	struct adf_hw_device_data *hw_data = accel_dev->hw_device;
 	struct service_hndl *service;
@@ -343,7 +340,6 @@ void adf_dev_shutdown(struct adf_accel_dev *accel_dev)
 	adf_cleanup_etr_data(accel_dev);
 	adf_dev_restore(accel_dev);
 }
-EXPORT_SYMBOL_GPL(adf_dev_shutdown);
 
 int adf_dev_restarting_notify(struct adf_accel_dev *accel_dev)
 {
@@ -375,7 +371,7 @@ int adf_dev_restarted_notify(struct adf_accel_dev *accel_dev)
 	return 0;
 }
 
-int adf_dev_shutdown_cache_cfg(struct adf_accel_dev *accel_dev)
+static int adf_dev_shutdown_cache_cfg(struct adf_accel_dev *accel_dev)
 {
 	char services[ADF_CFG_MAX_VAL_LEN_IN_BYTES] = {0};
 	int ret;
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH 0/5] crypto: qat - fix concurrency related issues
  2023-02-27 20:55 [PATCH 0/5] crypto: qat - fix concurrency related issues Shashank Gupta
                   ` (4 preceding siblings ...)
  2023-02-27 20:55 ` [PATCH 5/5] crypto: qat - make state machine functions static Shashank Gupta
@ 2023-03-10 11:31 ` Herbert Xu
  5 siblings, 0 replies; 7+ messages in thread
From: Herbert Xu @ 2023-03-10 11:31 UTC (permalink / raw)
  To: Shashank Gupta; +Cc: linux-crypto, qat-linux

On Mon, Feb 27, 2023 at 03:55:40PM -0500, Shashank Gupta wrote:
> This set fixes issues related to using unprotected QAT device state    
> machine functions that might cause concurrency issues at the time of state  
> transition.
> 
> The first patch fixes the QAT 4XXX device's unexpected behaviour that
> might occur if the user changes the device state or configuration via
> sysfs while the driver performing device bring-up. The sequence is changed
> in the probe function now the sysfs attribute is created after the device
> initialization.
> 
> The second patch fixes the concurrency issue in the sysfs `state` 
> attribute if multiple processes change the state of the qat device in 
> parallel. The change introduces the protected wrapper function 
> adf_dev_up() and adf_dev_down() that protects the transition of the device
> state. These are used in adf_sysfs.c instead of low-level state machine 
> functions.
> 
> The third patch replaces the use of unsafe low-level device state machine 
> function with its protected wrapper functions.
> 
> The forth patch refactor device restart logic by moving it into 
> adf_dev_restart() which uses safe adf_dev_up() and adf_dev_down().
> 
> The fifth patch define state machine functions static as they are unsafe
> to use for state transition now performed by safe adf_dev_up() and 
> adf_dev_down().
> 
> Shashank Gupta (5):
>   crypto: qat - delay sysfs initialization
>   crypto: qat - fix concurrency issue when device state changes
>   crypto: qat - replace state machine calls
>   crypto: qat - refactor device restart logic
>   crypto: qat - make state machine functions static
> 
>  drivers/crypto/qat/qat_4xxx/adf_drv.c             | 21 ++---
>  drivers/crypto/qat/qat_c3xxx/adf_drv.c            | 17 +---
>  drivers/crypto/qat/qat_c3xxxvf/adf_drv.c          | 13 +--
>  drivers/crypto/qat/qat_c62x/adf_drv.c             | 17 +---
>  drivers/crypto/qat/qat_c62xvf/adf_drv.c           | 13 +--
>  drivers/crypto/qat/qat_common/adf_accel_devices.h |  1 +
>  drivers/crypto/qat/qat_common/adf_aer.c           |  4 +-
>  drivers/crypto/qat/qat_common/adf_common_drv.h    |  8 +-
>  drivers/crypto/qat/qat_common/adf_ctl_drv.c       | 27 +++----
>  drivers/crypto/qat/qat_common/adf_dev_mgr.c       |  2 +
>  drivers/crypto/qat/qat_common/adf_init.c          | 96 ++++++++++++++++++++---
>  drivers/crypto/qat/qat_common/adf_sriov.c         | 10 +--
>  drivers/crypto/qat/qat_common/adf_sysfs.c         | 23 +-----
>  drivers/crypto/qat/qat_common/adf_vf_isr.c        |  3 +-
>  drivers/crypto/qat/qat_dh895xcc/adf_drv.c         | 17 +---
>  drivers/crypto/qat/qat_dh895xccvf/adf_drv.c       | 13 +--
>  16 files changed, 132 insertions(+), 153 deletions(-)
> 
> -- 
> 2.16.4

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-10 11:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-27 20:55 [PATCH 0/5] crypto: qat - fix concurrency related issues Shashank Gupta
2023-02-27 20:55 ` [PATCH 1/5] crypto: qat - delay sysfs initialization Shashank Gupta
2023-02-27 20:55 ` [PATCH 2/5] crypto: qat - fix concurrency issue when device state changes Shashank Gupta
2023-02-27 20:55 ` [PATCH 3/5] crypto: qat - replace state machine calls Shashank Gupta
2023-02-27 20:55 ` [PATCH 4/5] crypto: qat - refactor device restart logic Shashank Gupta
2023-02-27 20:55 ` [PATCH 5/5] crypto: qat - make state machine functions static Shashank Gupta
2023-03-10 11:31 ` [PATCH 0/5] crypto: qat - fix concurrency related issues Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).