crypto: qat - fix concurrency related issues

PATCH In review Shashank Gupta 2 participants 7 messages Feb 27, 2023 – Mar 10, 2023 Crypto API

Share & tools

Shashank Gupta

author

2023-02-27 15:44 UTC #0 lore raw

cover crypto: qat - fix concurrency related issues

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(-)

Signature · 1 line

2.16.4

Shashank Gupta

author

2023-02-27 15:44 UTC #1 lore raw

PATCH 1/5 crypto: qat - delay sysfs initialization

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 <[email protected]>

Reviewed-by Giovanni Cabiddu <[email protected]>

---
 drivers/crypto/qat/qat_4xxx/adf_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/crypto/qat/qat_4xxx/adf_drv.c +4 -4

index b3a4c7b23864..f7fdb435a70e 100644
@@ -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:

Signature · 1 line

2.16.4

Shashank Gupta

author

2023-02-27 15:44 UTC #2 lore raw

PATCH 2/5 crypto: qat - fix concurrency issue when device state changes

In reply to 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 <[email protected]>

Reviewed-by Giovanni Cabiddu <[email protected]>

---
 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(-)

drivers/crypto/qat/qat_common/adf_accel_devices.h +1 -0

index 284f5aad3ee0..7be933d6f0ff 100644
@@ -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;
 };

drivers/crypto/qat/qat_common/adf_common_drv.h +3 -0

index 7189265573c0..4bf1fceb7052 100644
@@ -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);

drivers/crypto/qat/qat_common/adf_dev_mgr.c +2 -0

index 4c752eed10fe..86ee36feefad 100644
@@ -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);
 }

drivers/crypto/qat/qat_common/adf_init.c +64 -0

67 lines not on this page · raw message

drivers/crypto/qat/qat_common/adf_sysfs.c +3 ~~-20~~

index e8b078e719c2..3eb6611ab1b1 100644
@@ -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;

Signature · 1 line

2.16.4

Shashank Gupta

author

2023-02-27 15:45 UTC #3 lore raw

PATCH 3/5 crypto: qat - replace state machine calls

In reply to 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 <[email protected]>

Reviewed-by Giovanni Cabiddu <[email protected]>

---
 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(-)

drivers/crypto/qat/qat_4xxx/adf_drv.c +3 ~~-14~~

index f7fdb435a70e..6f862b56c51c 100644
@@ -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);
 }

drivers/crypto/qat/qat_c3xxx/adf_drv.c +3 ~~-14~~

index 1f4fbf4562b2..4c00c4933805 100644
@@ -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);

drivers/crypto/qat/qat_c3xxxvf/adf_drv.c +3 ~~-10~~

index cf4ef83e186f..e8cc10f64134 100644
@@ -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);

drivers/crypto/qat/qat_c62x/adf_drv.c +3 ~~-14~~

index 4ccaf298250c..fcb2f5b8e053 100644
@@ -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);

drivers/crypto/qat/qat_c62xvf/adf_drv.c +3 ~~-10~~

index 0e642c94b929..37566309df94 100644
@@ -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);

drivers/crypto/qat/qat_common/adf_ctl_drv.c +9 ~~-18~~

index 9190532b27eb..b79ce4d0cc44 100644
@@ -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);

drivers/crypto/qat/qat_common/adf_sriov.c +2 -8

index d85a90cc387b..f44025bb6f99 100644
@@ -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;

drivers/crypto/qat/qat_common/adf_vf_isr.c +1 -2

index 8c95fcd8e64b..b05c3957a160 100644
@@ -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);

drivers/crypto/qat/qat_dh895xcc/adf_drv.c +3 ~~-14~~

index ebeb17b67fcd..4d27e4e43642 100644
@@ -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);

drivers/crypto/qat/qat_dh895xccvf/adf_drv.c +3 ~~-10~~

index c1485e702b3e..96854a1cd87e 100644
@@ -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);

Signature · 1 line

2.16.4

Shashank Gupta

author

2023-02-27 15:45 UTC #4 lore raw

PATCH 4/5 crypto: qat - refactor device restart logic

In reply to 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 <[email protected]>

Reviewed-by Giovanni Cabiddu <[email protected]>

---
 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(-)

drivers/crypto/qat/qat_common/adf_aer.c +1 -3

index fe9bb2f3536a..9fa76c527051 100644
@@ -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);

drivers/crypto/qat/qat_common/adf_common_drv.h +1 -0

index 4bf1fceb7052..3666109b6320 100644
@@ -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);

drivers/crypto/qat/qat_common/adf_init.c +18 -0

index 988cffd0b833..11ade5d8e4a0 100644
@@ -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);

Signature · 1 line

2.16.4

Shashank Gupta

author

2023-02-27 15:45 UTC #5 lore raw

PATCH 5/5 crypto: qat - make state machine functions static

In reply to 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 <[email protected]>

Reviewed-by Giovanni Cabiddu <[email protected]>

---
 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(-)

drivers/crypto/qat/qat_common/adf_common_drv.h +0 -6

index 3666109b6320..b2f14aaf6950 100644
@@ -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);

drivers/crypto/qat/qat_common/adf_init.c +5 -9

index 11ade5d8e4a0..0985f64ab11a 100644
@@ -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;

Signature · 1 line

2.16.4

Herbert Xu

maintainer

2023-03-10 11:31 UTC #6 lore raw

In reply to Shashank Gupta

Quoted 43 lines · 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 +++----

Quoted 11 lines

 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(-)

Signature · 1 line

2.16.4

All applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt