[PATCH v3 00/18] UFS patches for kernel v5.15

[PATCH v3 00/18] UFS patches for kernel v5.15

[Bart Van Assche]

Hi Martin,

Please consider the patches in this series for kernel v5.15.

Thank you,

Bart.

Changes compared to v2:
- Included a stack corruption fix.
- Dropped patch "Remove a local variable" and added patches "Revert "Utilize
  Transfer Request List Completion Notification Register"" and "Optimize
  serialization of setup_xfer_req() calls".
- Added patch "Optimize serialization of setup_xfer_req() calls".

Changes compared to v1:
- Left out the SCSI core patches for the SCSI error handler in order not to
  delay the UFS patches by the conversation around the SCSI error handler
  patches.
- Restored the WARN_ON_ONCE(tag < 0) statements in the patch that removes
  ufshcd_valid_tag().
- Split "Fix a race in the completion path" in two patches.
- Added a fault injection patch.

Bart Van Assche (18):
  scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
  scsi: ufs: Reduce power management code duplication
  scsi: ufs: Only include power management code if necessary
  scsi: ufs: Rename the second ufshcd_probe_hba() argument
  scsi: ufs: Use DECLARE_COMPLETION_ONSTACK() where appropriate
  scsi: ufs: Remove ufshcd_valid_tag()
  scsi: ufs: Verify UIC locking requirements at runtime
  scsi: ufs: Improve static type checking for the host controller state
  scsi: ufs: Remove several wmb() calls
  scsi: ufs: Inline ufshcd_outstanding_req_clear()
  scsi: ufs: Revert "Utilize Transfer Request List Completion
    Notification Register"
  scsi: ufs: Optimize serialization of setup_xfer_req() calls
  scsi: ufs: Optimize SCSI command processing
  scsi: ufs: Fix the SCSI abort handler
  scsi: ufs: Request sense data asynchronously
  scsi: ufs: Synchronize SCSI and UFS error handling
  scsi: ufs: Retry aborted SCSI commands instead of completing these
    successfully
  scsi: ufs: Add fault injection support

 drivers/scsi/ufs/Kconfig               |   7 +
 drivers/scsi/ufs/Makefile              |   1 +
 drivers/scsi/ufs/cdns-pltfrm.c         |   7 +-
 drivers/scsi/ufs/tc-dwc-g210-pci.c     |  32 +-
 drivers/scsi/ufs/tc-dwc-g210-pltfrm.c  |   7 +-
 drivers/scsi/ufs/ufs-exynos.c          |   7 +-
 drivers/scsi/ufs/ufs-fault-injection.c |  70 ++++
 drivers/scsi/ufs/ufs-fault-injection.h |  24 ++
 drivers/scsi/ufs/ufs-hisi.c            |   7 +-
 drivers/scsi/ufs/ufs-mediatek.c        |   7 +-
 drivers/scsi/ufs/ufs-qcom.c            |   7 +-
 drivers/scsi/ufs/ufshcd-pci.c          |  48 +--
 drivers/scsi/ufs/ufshcd-pltfrm.c       |  47 ---
 drivers/scsi/ufs/ufshcd-pltfrm.h       |  18 -
 drivers/scsi/ufs/ufshcd.c              | 491 +++++++++++--------------
 drivers/scsi/ufs/ufshcd.h              |  63 ++--
 drivers/scsi/ufs/ufshci.h              |   1 -
 17 files changed, 373 insertions(+), 471 deletions(-)
 create mode 100644 drivers/scsi/ufs/ufs-fault-injection.c
 create mode 100644 drivers/scsi/ufs/ufs-fault-injection.h

[PATCH v3 01/18] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()

[Bart Van Assche]

If param_offset > buff_len then the memcpy() statement in
ufshcd_read_desc_param() corrupts memory since it copies
256 + buff_len - param_offset bytes into a buffer with size buff_len.
Since param_offset < 256 this results in writing past the bound of the
output buffer.

Fixes: cbe193f6f093 ("scsi: ufs: Fix potential NULL pointer access during memcpy")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 064a44e628d6..6c251afe65f9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3418,9 +3418,11 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 
 	if (is_kmalloc) {
 		/* Make sure we don't copy more data than available */
-		if (param_offset + param_size > buff_len)
-			param_size = buff_len - param_offset;
-		memcpy(param_read_buf, &desc_buf[param_offset], param_size);
+		if (param_offset >= buff_len)
+			ret = -EINVAL;
+		else
+			memcpy(param_read_buf, &desc_buf[param_offset],
+			       min_t(u32, param_size, buff_len - param_offset));
 	}
 out:
 	if (is_kmalloc)

RE: [PATCH v3 01/18] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()

[Avri Altman]

> 
> If param_offset > buff_len then the memcpy() statement in
> ufshcd_read_desc_param() corrupts memory since it copies
> 256 + buff_len - param_offset bytes into a buffer with size buff_len.
> Since param_offset < 256 this results in writing past the bound of the output
> buffer.
> 
> Fixes: cbe193f6f093 ("scsi: ufs: Fix potential NULL pointer access during
> memcpy")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Your fix is fine IMO.
However, the root cause of this weird bug is that rpmb has its own unit descriptor,
But ufshcd_map_desc_id_to_length doesn't accept index as argument, and returned 0x2d instead of 0x23, as it should.

Thanks,
Avri
> ---
>  drivers/scsi/ufs/ufshcd.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 064a44e628d6..6c251afe65f9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3418,9 +3418,11 @@ int ufshcd_read_desc_param(struct ufs_hba
> *hba,
> 
>         if (is_kmalloc) {
>                 /* Make sure we don't copy more data than available */
> -               if (param_offset + param_size > buff_len)
> -                       param_size = buff_len - param_offset;
> -               memcpy(param_read_buf, &desc_buf[param_offset], param_size);
> +               if (param_offset >= buff_len)
> +                       ret = -EINVAL;
> +               else
> +                       memcpy(param_read_buf, &desc_buf[param_offset],
> +                              min_t(u32, param_size, buff_len -
> + param_offset));
>         }
>  out:
>         if (is_kmalloc)

[PATCH v3 02/18] scsi: ufs: Reduce power management code duplication

[Bart Van Assche]

Move the dev_get_drvdata() calls into the ufshcd_{system,runtime}_*()
functions. Remove ufshcd_runtime_idle() since it is empty. This patch
does not change any functionality.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/cdns-pltfrm.c        |  7 ++--
 drivers/scsi/ufs/tc-dwc-g210-pci.c    | 32 ++----------------
 drivers/scsi/ufs/tc-dwc-g210-pltfrm.c |  7 ++--
 drivers/scsi/ufs/ufs-exynos.c         |  7 ++--
 drivers/scsi/ufs/ufs-hisi.c           |  7 ++--
 drivers/scsi/ufs/ufs-mediatek.c       |  7 ++--
 drivers/scsi/ufs/ufs-qcom.c           |  7 ++--
 drivers/scsi/ufs/ufshcd-pci.c         | 48 ++-------------------------
 drivers/scsi/ufs/ufshcd-pltfrm.c      | 47 --------------------------
 drivers/scsi/ufs/ufshcd-pltfrm.h      | 18 ----------
 drivers/scsi/ufs/ufshcd.c             | 41 ++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h             |  9 +++--
 12 files changed, 41 insertions(+), 196 deletions(-)

diff --git a/drivers/scsi/ufs/cdns-pltfrm.c b/drivers/scsi/ufs/cdns-pltfrm.c
index 908ff39c4856..7da8be2f35c4 100644
--- a/drivers/scsi/ufs/cdns-pltfrm.c
+++ b/drivers/scsi/ufs/cdns-pltfrm.c
@@ -318,11 +318,8 @@ static int cdns_ufs_pltfrm_remove(struct platform_device *pdev)
 }
 
 static const struct dev_pm_ops cdns_ufs_dev_pm_ops = {
-	.suspend         = ufshcd_pltfrm_suspend,
-	.resume          = ufshcd_pltfrm_resume,
-	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
-	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
-	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 	.prepare	 = ufshcd_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
diff --git a/drivers/scsi/ufs/tc-dwc-g210-pci.c b/drivers/scsi/ufs/tc-dwc-g210-pci.c
index ec4589afbc13..679289e1a78e 100644
--- a/drivers/scsi/ufs/tc-dwc-g210-pci.c
+++ b/drivers/scsi/ufs/tc-dwc-g210-pci.c
@@ -23,31 +23,6 @@ static int tc_type = TC_G210_INV;
 module_param(tc_type, int, 0);
 MODULE_PARM_DESC(tc_type, "Test Chip Type (20 = 20-bit, 40 = 40-bit)");
 
-static int tc_dwc_g210_pci_suspend(struct device *dev)
-{
-	return ufshcd_system_suspend(dev_get_drvdata(dev));
-}
-
-static int tc_dwc_g210_pci_resume(struct device *dev)
-{
-	return ufshcd_system_resume(dev_get_drvdata(dev));
-}
-
-static int tc_dwc_g210_pci_runtime_suspend(struct device *dev)
-{
-	return ufshcd_runtime_suspend(dev_get_drvdata(dev));
-}
-
-static int tc_dwc_g210_pci_runtime_resume(struct device *dev)
-{
-	return ufshcd_runtime_resume(dev_get_drvdata(dev));
-}
-
-static int tc_dwc_g210_pci_runtime_idle(struct device *dev)
-{
-	return ufshcd_runtime_idle(dev_get_drvdata(dev));
-}
-
 /*
  * struct ufs_hba_dwc_vops - UFS DWC specific variant operations
  */
@@ -143,11 +118,8 @@ tc_dwc_g210_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 }
 
 static const struct dev_pm_ops tc_dwc_g210_pci_pm_ops = {
-	.suspend	= tc_dwc_g210_pci_suspend,
-	.resume		= tc_dwc_g210_pci_resume,
-	.runtime_suspend = tc_dwc_g210_pci_runtime_suspend,
-	.runtime_resume  = tc_dwc_g210_pci_runtime_resume,
-	.runtime_idle    = tc_dwc_g210_pci_runtime_idle,
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 	.prepare	 = ufshcd_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
diff --git a/drivers/scsi/ufs/tc-dwc-g210-pltfrm.c b/drivers/scsi/ufs/tc-dwc-g210-pltfrm.c
index a1268e4f44d6..783ec43efa78 100644
--- a/drivers/scsi/ufs/tc-dwc-g210-pltfrm.c
+++ b/drivers/scsi/ufs/tc-dwc-g210-pltfrm.c
@@ -84,11 +84,8 @@ static int tc_dwc_g210_pltfm_remove(struct platform_device *pdev)
 }
 
 static const struct dev_pm_ops tc_dwc_g210_pltfm_pm_ops = {
-	.suspend	= ufshcd_pltfrm_suspend,
-	.resume		= ufshcd_pltfrm_resume,
-	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
-	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
-	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 };
 
 static struct platform_driver tc_dwc_g210_pltfm_driver = {
diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c
index cf46d6f86e0e..5aa096e5b6cc 100644
--- a/drivers/scsi/ufs/ufs-exynos.c
+++ b/drivers/scsi/ufs/ufs-exynos.c
@@ -1287,11 +1287,8 @@ static const struct of_device_id exynos_ufs_of_match[] = {
 };
 
 static const struct dev_pm_ops exynos_ufs_pm_ops = {
-	.suspend	= ufshcd_pltfrm_suspend,
-	.resume		= ufshcd_pltfrm_resume,
-	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
-	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
-	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 	.prepare	 = ufshcd_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c
index 5b147a48161b..6b706de8354b 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -572,11 +572,8 @@ static int ufs_hisi_remove(struct platform_device *pdev)
 }
 
 static const struct dev_pm_ops ufs_hisi_pm_ops = {
-	.suspend	= ufshcd_pltfrm_suspend,
-	.resume		= ufshcd_pltfrm_resume,
-	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
-	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
-	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 	.prepare	 = ufshcd_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index d2c251628a05..80b3545dd17d 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -1140,11 +1140,8 @@ static int ufs_mtk_remove(struct platform_device *pdev)
 }
 
 static const struct dev_pm_ops ufs_mtk_pm_ops = {
-	.suspend         = ufshcd_pltfrm_suspend,
-	.resume          = ufshcd_pltfrm_resume,
-	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
-	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
-	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 	.prepare	 = ufshcd_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 9b1d18d7c9bb..9d9770f1db4f 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1546,11 +1546,8 @@ MODULE_DEVICE_TABLE(acpi, ufs_qcom_acpi_match);
 #endif
 
 static const struct dev_pm_ops ufs_qcom_pm_ops = {
-	.suspend	= ufshcd_pltfrm_suspend,
-	.resume		= ufshcd_pltfrm_resume,
-	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
-	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
-	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 	.prepare	 = ufshcd_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index e6c334bfb4c2..b3bcc5c882da 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -385,48 +385,6 @@ static struct ufs_hba_variant_ops ufs_intel_lkf_hba_vops = {
 	.device_reset		= ufs_intel_device_reset,
 };
 
-#ifdef CONFIG_PM_SLEEP
-/**
- * ufshcd_pci_suspend - suspend power management function
- * @dev: pointer to PCI device handle
- *
- * Returns 0 if successful
- * Returns non-zero otherwise
- */
-static int ufshcd_pci_suspend(struct device *dev)
-{
-	return ufshcd_system_suspend(dev_get_drvdata(dev));
-}
-
-/**
- * ufshcd_pci_resume - resume power management function
- * @dev: pointer to PCI device handle
- *
- * Returns 0 if successful
- * Returns non-zero otherwise
- */
-static int ufshcd_pci_resume(struct device *dev)
-{
-	return ufshcd_system_resume(dev_get_drvdata(dev));
-}
-
-#endif /* !CONFIG_PM_SLEEP */
-
-#ifdef CONFIG_PM
-static int ufshcd_pci_runtime_suspend(struct device *dev)
-{
-	return ufshcd_runtime_suspend(dev_get_drvdata(dev));
-}
-static int ufshcd_pci_runtime_resume(struct device *dev)
-{
-	return ufshcd_runtime_resume(dev_get_drvdata(dev));
-}
-static int ufshcd_pci_runtime_idle(struct device *dev)
-{
-	return ufshcd_runtime_idle(dev_get_drvdata(dev));
-}
-#endif /* !CONFIG_PM */
-
 /**
  * ufshcd_pci_shutdown - main function to put the controller in reset state
  * @pdev: pointer to PCI device handle
@@ -510,10 +468,8 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 }
 
 static const struct dev_pm_ops ufshcd_pci_pm_ops = {
-	SET_RUNTIME_PM_OPS(ufshcd_pci_runtime_suspend,
-			   ufshcd_pci_runtime_resume,
-			   ufshcd_pci_runtime_idle)
-	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_pci_suspend, ufshcd_pci_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
+	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 #ifdef CONFIG_PM_SLEEP
 	.prepare	= ufshcd_suspend_prepare,
 	.complete	= ufshcd_resume_complete,
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 298e22ef907e..8859c13f4e09 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -170,53 +170,6 @@ static int ufshcd_parse_regulator_info(struct ufs_hba *hba)
 	return err;
 }
 
-#ifdef CONFIG_PM
-/**
- * ufshcd_pltfrm_suspend - suspend power management function
- * @dev: pointer to device handle
- *
- * Returns 0 if successful
- * Returns non-zero otherwise
- */
-int ufshcd_pltfrm_suspend(struct device *dev)
-{
-	return ufshcd_system_suspend(dev_get_drvdata(dev));
-}
-EXPORT_SYMBOL_GPL(ufshcd_pltfrm_suspend);
-
-/**
- * ufshcd_pltfrm_resume - resume power management function
- * @dev: pointer to device handle
- *
- * Returns 0 if successful
- * Returns non-zero otherwise
- */
-int ufshcd_pltfrm_resume(struct device *dev)
-{
-	return ufshcd_system_resume(dev_get_drvdata(dev));
-}
-EXPORT_SYMBOL_GPL(ufshcd_pltfrm_resume);
-
-int ufshcd_pltfrm_runtime_suspend(struct device *dev)
-{
-	return ufshcd_runtime_suspend(dev_get_drvdata(dev));
-}
-EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_suspend);
-
-int ufshcd_pltfrm_runtime_resume(struct device *dev)
-{
-	return ufshcd_runtime_resume(dev_get_drvdata(dev));
-}
-EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_resume);
-
-int ufshcd_pltfrm_runtime_idle(struct device *dev)
-{
-	return ufshcd_runtime_idle(dev_get_drvdata(dev));
-}
-EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_idle);
-
-#endif /* CONFIG_PM */
-
 void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
 {
 	ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.h b/drivers/scsi/ufs/ufshcd-pltfrm.h
index 772a8e848098..c33e28ac6ef6 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.h
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.h
@@ -33,22 +33,4 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 		       const struct ufs_hba_variant_ops *vops);
 void ufshcd_pltfrm_shutdown(struct platform_device *pdev);
 
-#ifdef CONFIG_PM
-
-int ufshcd_pltfrm_suspend(struct device *dev);
-int ufshcd_pltfrm_resume(struct device *dev);
-int ufshcd_pltfrm_runtime_suspend(struct device *dev);
-int ufshcd_pltfrm_runtime_resume(struct device *dev);
-int ufshcd_pltfrm_runtime_idle(struct device *dev);
-
-#else /* !CONFIG_PM */
-
-#define ufshcd_pltfrm_suspend	NULL
-#define ufshcd_pltfrm_resume	NULL
-#define ufshcd_pltfrm_runtime_suspend	NULL
-#define ufshcd_pltfrm_runtime_resume	NULL
-#define ufshcd_pltfrm_runtime_idle	NULL
-
-#endif /* CONFIG_PM */
-
 #endif /* UFSHCD_PLTFRM_H_ */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6c251afe65f9..58b1742ec9b9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9208,15 +9208,17 @@ static int ufshcd_resume(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_system_suspend - system suspend routine
- * @hba: per adapter instance
+ * ufshcd_system_suspend - system suspend callback
+ * @dev: Device associated with the UFS controller.
  *
- * Check the description of ufshcd_suspend() function for more details.
+ * Executed before putting the system into a sleep state in which the contents
+ * of main memory are preserved.
  *
  * Returns 0 for success and non-zero for failure
  */
-int ufshcd_system_suspend(struct ufs_hba *hba)
+int ufshcd_system_suspend(struct device *dev)
 {
+	struct ufs_hba *hba = dev_get_drvdata(dev);
 	int ret = 0;
 	ktime_t start = ktime_get();
 
@@ -9233,16 +9235,19 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 EXPORT_SYMBOL(ufshcd_system_suspend);
 
 /**
- * ufshcd_system_resume - system resume routine
- * @hba: per adapter instance
+ * ufshcd_system_resume - system resume callback
+ * @dev: Device associated with the UFS controller.
+ *
+ * Executed after waking the system up from a sleep state in which the contents
+ * of main memory were preserved.
  *
  * Returns 0 for success and non-zero for failure
  */
-
-int ufshcd_system_resume(struct ufs_hba *hba)
+int ufshcd_system_resume(struct device *dev)
 {
-	int ret = 0;
+	struct ufs_hba *hba = dev_get_drvdata(dev);
 	ktime_t start = ktime_get();
+	int ret = 0;
 
 	if (pm_runtime_suspended(hba->dev))
 		goto out;
@@ -9259,15 +9264,16 @@ int ufshcd_system_resume(struct ufs_hba *hba)
 EXPORT_SYMBOL(ufshcd_system_resume);
 
 /**
- * ufshcd_runtime_suspend - runtime suspend routine
- * @hba: per adapter instance
+ * ufshcd_runtime_suspend - runtime suspend callback
+ * @dev: Device associated with the UFS controller.
  *
  * Check the description of ufshcd_suspend() function for more details.
  *
  * Returns 0 for success and non-zero for failure
  */
-int ufshcd_runtime_suspend(struct ufs_hba *hba)
+int ufshcd_runtime_suspend(struct device *dev)
 {
+	struct ufs_hba *hba = dev_get_drvdata(dev);
 	int ret;
 	ktime_t start = ktime_get();
 
@@ -9282,7 +9288,7 @@ EXPORT_SYMBOL(ufshcd_runtime_suspend);
 
 /**
  * ufshcd_runtime_resume - runtime resume routine
- * @hba: per adapter instance
+ * @dev: Device associated with the UFS controller.
  *
  * This function basically brings controller
  * to active state. Following operations are done in this function:
@@ -9290,8 +9296,9 @@ EXPORT_SYMBOL(ufshcd_runtime_suspend);
  * 1. Turn on all the controller related clocks
  * 2. Turn ON VCC rail
  */
-int ufshcd_runtime_resume(struct ufs_hba *hba)
+int ufshcd_runtime_resume(struct device *dev)
 {
+	struct ufs_hba *hba = dev_get_drvdata(dev);
 	int ret;
 	ktime_t start = ktime_get();
 
@@ -9304,12 +9311,6 @@ int ufshcd_runtime_resume(struct ufs_hba *hba)
 }
 EXPORT_SYMBOL(ufshcd_runtime_resume);
 
-int ufshcd_runtime_idle(struct ufs_hba *hba)
-{
-	return 0;
-}
-EXPORT_SYMBOL(ufshcd_runtime_idle);
-
 /**
  * ufshcd_shutdown - shutdown routine
  * @hba: per adapter instance
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 971cfabc4a1e..cc971aebb9da 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1001,11 +1001,10 @@ static inline u8 ufshcd_wb_get_query_index(struct ufs_hba *hba)
 	return 0;
 }
 
-extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
-extern int ufshcd_runtime_resume(struct ufs_hba *hba);
-extern int ufshcd_runtime_idle(struct ufs_hba *hba);
-extern int ufshcd_system_suspend(struct ufs_hba *hba);
-extern int ufshcd_system_resume(struct ufs_hba *hba);
+extern int ufshcd_runtime_suspend(struct device *dev);
+extern int ufshcd_runtime_resume(struct device *dev);
+extern int ufshcd_system_suspend(struct device *dev);
+extern int ufshcd_system_resume(struct device *dev);
 extern int ufshcd_shutdown(struct ufs_hba *hba);
 extern int ufshcd_dme_configure_adapt(struct ufs_hba *hba,
 				      int agreed_gear,

Re: [PATCH v3 02/18] scsi: ufs: Reduce power management code duplication

[Stanley Chu]

Hi Bart,

> Move the dev_get_drvdata() calls into the ufshcd_{system,runtime}_*()
> functions. Remove ufshcd_runtime_idle() since it is empty. This patch
> does not change any functionality.
>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

[PATCH v3 03/18] scsi: ufs: Only include power management code if necessary

[Bart Van Assche]

This patch slightly reduces the UFS driver size if built with power
management support disabled.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 8 ++++++++
 drivers/scsi/ufs/ufshcd.h | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 58b1742ec9b9..0503ebe197f6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8740,6 +8740,7 @@ static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
 		usleep_range(5000, 5100);
 }
 
+#ifdef CONFIG_PM
 static int ufshcd_vreg_set_hpm(struct ufs_hba *hba)
 {
 	int ret = 0;
@@ -8767,6 +8768,7 @@ static int ufshcd_vreg_set_hpm(struct ufs_hba *hba)
 out:
 	return ret;
 }
+#endif /* CONFIG_PM */
 
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba)
 {
@@ -9169,6 +9171,7 @@ static int ufshcd_suspend(struct ufs_hba *hba)
 	return ret;
 }
 
+#ifdef CONFIG_PM
 /**
  * ufshcd_resume - helper function for resume operations
  * @hba: per adapter instance
@@ -9206,7 +9209,9 @@ static int ufshcd_resume(struct ufs_hba *hba)
 		ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret);
 	return ret;
 }
+#endif /* CONFIG_PM */
 
+#ifdef CONFIG_PM_SLEEP
 /**
  * ufshcd_system_suspend - system suspend callback
  * @dev: Device associated with the UFS controller.
@@ -9262,7 +9267,9 @@ int ufshcd_system_resume(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL(ufshcd_system_resume);
+#endif /* CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_PM
 /**
  * ufshcd_runtime_suspend - runtime suspend callback
  * @dev: Device associated with the UFS controller.
@@ -9310,6 +9317,7 @@ int ufshcd_runtime_resume(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL(ufshcd_runtime_resume);
+#endif /* CONFIG_PM */
 
 /**
  * ufshcd_shutdown - shutdown routine
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index cc971aebb9da..0a3afd9499c9 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1001,10 +1001,14 @@ static inline u8 ufshcd_wb_get_query_index(struct ufs_hba *hba)
 	return 0;
 }
 
+#ifdef CONFIG_PM
 extern int ufshcd_runtime_suspend(struct device *dev);
 extern int ufshcd_runtime_resume(struct device *dev);
+#endif
+#ifdef CONFIG_PM_SLEEP
 extern int ufshcd_system_suspend(struct device *dev);
 extern int ufshcd_system_resume(struct device *dev);
+#endif
 extern int ufshcd_shutdown(struct ufs_hba *hba);
 extern int ufshcd_dme_configure_adapt(struct ufs_hba *hba,
 				      int agreed_gear,

[PATCH v3 04/18] scsi: ufs: Rename the second ufshcd_probe_hba() argument

[Bart Van Assche]

Rename the second argument of ufshcd_probe_hba() such that the name of
that argument reflects its purpose instead of how the function is called.
See also commit 1b9e21412f72 ("scsi: ufs: Split ufshcd_probe_hba() based
on its called flow").

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0503ebe197f6..36b60afcce34 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7968,13 +7968,13 @@ static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_probe_hba - probe hba to detect device and initialize
+ * ufshcd_probe_hba - probe hba to detect device and initialize it
  * @hba: per-adapter instance
- * @async: asynchronous execution or not
+ * @init_dev_params: whether or not to call ufshcd_device_params_init().
  *
  * Execute link-startup and verify device initialization
  */
-static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
+static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 {
 	int ret;
 	unsigned long flags;
@@ -8006,7 +8006,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
 	 * Initialize UFS device parameters used by driver, these
 	 * parameters are associated with UFS descriptors.
 	 */
-	if (async) {
+	if (init_dev_params) {
 		ret = ufshcd_device_params_init(hba);
 		if (ret)
 			goto out;

Re: [PATCH v3 04/18] scsi: ufs: Rename the second ufshcd_probe_hba() argument

[Stanley Chu]

Hi Bart,

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Rename the second argument of ufshcd_probe_hba() such that the name
> of
> that argument reflects its purpose instead of how the function is
> called.
> See also commit 1b9e21412f72 ("scsi: ufs: Split ufshcd_probe_hba()
> based
> on its called flow").
> 

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

[PATCH v3 05/18] scsi: ufs: Use DECLARE_COMPLETION_ONSTACK() where appropriate

[Bart Van Assche]

From Documentation/scheduler/completion.rst: "When a completion is declared
as a local variable within a function, then the initialization should
always use DECLARE_COMPLETION_ONSTACK() explicitly, not just to make
lockdep happy, but also to make it clear that limited scope had been
considered and is intentional."

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 36b60afcce34..17afd1b0bd2c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2948,11 +2948,11 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 		enum dev_cmd_type cmd_type, int timeout)
 {
 	struct request_queue *q = hba->cmd_queue;
+	DECLARE_COMPLETION_ONSTACK(wait);
 	struct request *req;
 	struct ufshcd_lrb *lrbp;
 	int err;
 	int tag;
-	struct completion wait;
 
 	down_read(&hba->clk_scaling_lock);
 
@@ -2977,7 +2977,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 		goto out;
 	}
 
-	init_completion(&wait);
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
@@ -3984,14 +3983,13 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
  */
 static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 {
-	struct completion uic_async_done;
+	DECLARE_COMPLETION_ONSTACK(uic_async_done);
 	unsigned long flags;
 	u8 status;
 	int ret;
 	bool reenable_intr = false;
 
 	mutex_lock(&hba->uic_cmd_mutex);
-	init_completion(&uic_async_done);
 	ufshcd_add_delay_before_dme_cmd(hba);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
@@ -6664,11 +6662,11 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 					enum query_opcode desc_op)
 {
 	struct request_queue *q = hba->cmd_queue;
+	DECLARE_COMPLETION_ONSTACK(wait);
 	struct request *req;
 	struct ufshcd_lrb *lrbp;
 	int err = 0;
 	int tag;
-	struct completion wait;
 	u8 upiu_flags;
 
 	down_read(&hba->clk_scaling_lock);
@@ -6686,7 +6684,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 		goto out;
 	}
 
-	init_completion(&wait);
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	lrbp->cmd = NULL;

[PATCH v3 06/18] scsi: ufs: Remove ufshcd_valid_tag()

[Bart Van Assche]

scsi_add_host() allocates shost->can_queue tags. ufshcd_init() sets
shost->can_queue to hba->nutrs. In other words, we know that tag values
will less than hba->nutrs. Hence remove the checks that verify that
blk_get_request() returns a tag less than hba->nutrs. This check
was introduced by commit 14497328b6a6 ("scsi: ufs: verify command tag
validity").

Keep the tag >= 0 check because it helps to detect use-after-free issues.

Reviewed-by: Bean Huo <beanhuo@micron.com>
CC: Avri Altman <avri.altman@wdc.com>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 42 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 17afd1b0bd2c..ec12cd4eae03 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -252,11 +252,6 @@ static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable);
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
 static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
 
-static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
-{
-	return tag >= 0 && tag < hba->nutrs;
-}
-
 static inline void ufshcd_enable_irq(struct ufs_hba *hba)
 {
 	if (!hba->is_irq_enabled) {
@@ -2700,20 +2695,12 @@ static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
  */
 static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
+	struct ufs_hba *hba = shost_priv(host);
+	int tag = cmd->request->tag;
 	struct ufshcd_lrb *lrbp;
-	struct ufs_hba *hba;
-	int tag;
 	int err = 0;
 
-	hba = shost_priv(host);
-
-	tag = cmd->request->tag;
-	if (!ufshcd_valid_tag(hba, tag)) {
-		dev_err(hba->dev,
-			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
-			__func__, tag, cmd, cmd->request);
-		BUG();
-	}
+	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
 	if (!down_read_trylock(&hba->clk_scaling_lock))
 		return SCSI_MLQUEUE_HOST_BUSY;
@@ -2967,7 +2954,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 		goto out_unlock;
 	}
 	tag = req->tag;
-	WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
+	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 	/* Set the timeout such that the SCSI error handler is not activated. */
 	req->timeout = msecs_to_jiffies(2 * timeout);
 	blk_mq_start_request(req);
@@ -6677,7 +6664,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 		goto out_unlock;
 	}
 	tag = req->tag;
-	WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
+	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
 	if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
 		err = -EBUSY;
@@ -6979,24 +6966,15 @@ static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
  */
 static int ufshcd_abort(struct scsi_cmnd *cmd)
 {
-	struct Scsi_Host *host;
-	struct ufs_hba *hba;
+	struct Scsi_Host *host = cmd->device->host;
+	struct ufs_hba *hba = shost_priv(host);
+	unsigned int tag = cmd->request->tag;
+	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	unsigned long flags;
-	unsigned int tag;
 	int err = 0;
-	struct ufshcd_lrb *lrbp;
 	u32 reg;
 
-	host = cmd->device->host;
-	hba = shost_priv(host);
-	tag = cmd->request->tag;
-	lrbp = &hba->lrb[tag];
-	if (!ufshcd_valid_tag(hba, tag)) {
-		dev_err(hba->dev,
-			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
-			__func__, tag, cmd, cmd->request);
-		BUG();
-	}
+	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
 	ufshcd_hold(hba, false);
 	reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);

[PATCH v3 07/18] scsi: ufs: Verify UIC locking requirements at runtime

[Bart Van Assche]

Instead of documenting the locking requirements of the UIC code as comments,
use lockdep_assert_held() such that lockdep verifies the lockdep
requirements at runtime if lockdep is enabled.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 16 +++++++++-------
 drivers/scsi/ufs/ufshcd.h |  2 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ec12cd4eae03..233a1cf53dce 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2241,15 +2241,15 @@ static inline u8 ufshcd_get_upmcrs(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch an UIC command to the Unipro layer
  * @hba: per adapter instance
  * @uic_cmd: UIC command
- *
- * Mutex must be held.
  */
 static inline void
 ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 {
+	lockdep_assert_held(&hba->uic_cmd_mutex);
+
 	WARN_ON(hba->active_uic_cmd);
 
 	hba->active_uic_cmd = uic_cmd;
@@ -2267,11 +2267,10 @@ ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 }
 
 /**
- * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * ufshcd_wait_for_uic_cmd - Wait for completion of an UIC command
  * @hba: per adapter instance
  * @uic_cmd: UIC command
  *
- * Must be called with mutex held.
  * Returns 0 only if success.
  */
 static int
@@ -2280,6 +2279,8 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	int ret;
 	unsigned long flags;
 
+	lockdep_assert_held(&hba->uic_cmd_mutex);
+
 	if (wait_for_completion_timeout(&uic_cmd->done,
 					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
@@ -2309,14 +2310,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
  * @uic_cmd: UIC command
  * @completion: initialize the completion only if this is set to true
  *
- * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called
- * with mutex held and host_lock locked.
  * Returns 0 only if success.
  */
 static int
 __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
 		      bool completion)
 {
+	lockdep_assert_held(&hba->uic_cmd_mutex);
+	lockdep_assert_held(hba->host->host_lock);
+
 	if (!ufshcd_ready_for_uic_cmd(hba)) {
 		dev_err(hba->dev,
 			"Controller not ready to accept UIC commands\n");
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 0a3afd9499c9..be9e6c683abe 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -683,7 +683,7 @@ struct ufs_hba_monitor {
  * @priv: pointer to variant specific private data
  * @irq: Irq number of the controller
  * @active_uic_cmd: handle of active UIC command
- * @uic_cmd_mutex: mutex for uic command
+ * @uic_cmd_mutex: mutex for UIC command
  * @tmf_tag_set: TMF tag set.
  * @tmf_queue: Used to allocate TMF tags.
  * @pwr_done: completion for power mode change

[PATCH v3 08/18] scsi: ufs: Improve static type checking for the host controller state

[Bart Van Assche]

Assign a name to the enumeration type for UFS host controller states and
remove the default clause from switch statements on this enumeration type
to make the compiler warn about unhandled enumeration labels.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 15 ---------------
 drivers/scsi/ufs/ufshcd.h | 25 +++++++++++++++++++++++--
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 233a1cf53dce..f467630be7df 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -127,15 +127,6 @@ enum {
 	UFSHCD_CAN_QUEUE	= 32,
 };
 
-/* UFSHCD states */
-enum {
-	UFSHCD_STATE_RESET,
-	UFSHCD_STATE_ERROR,
-	UFSHCD_STATE_OPERATIONAL,
-	UFSHCD_STATE_EH_SCHEDULED_FATAL,
-	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
-};
-
 /* UFSHCD error handling flags */
 enum {
 	UFSHCD_EH_IN_PROGRESS = (1 << 0),
@@ -2736,12 +2727,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		set_host_byte(cmd, DID_ERROR);
 		cmd->scsi_done(cmd);
 		goto out;
-	default:
-		dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
-				__func__, hba->ufshcd_state);
-		set_host_byte(cmd, DID_BAD_TARGET);
-		cmd->scsi_done(cmd);
-		goto out;
 	}
 
 	hba->req_abort_count = 0;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index be9e6c683abe..8dcf0df770a2 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -476,6 +476,27 @@ struct ufs_stats {
 	struct ufs_event_hist event[UFS_EVT_CNT];
 };
 
+/**
+ * enum ufshcd_state - UFS host controller state
+ * @UFSHCD_STATE_RESET: Link is not operational. Postpone SCSI command
+ *	processing.
+ * @UFSHCD_STATE_OPERATIONAL: The host controller is operational and can process
+ *	SCSI commands.
+ * @UFSHCD_STATE_EH_SCHEDULED_NON_FATAL: The error handler has been scheduled.
+ *	SCSI commands may be submitted to the controller.
+ * @UFSHCD_STATE_EH_SCHEDULED_FATAL: The error handler has been scheduled. Fail
+ *	newly submitted SCSI commands with error code DID_BAD_TARGET.
+ * @UFSHCD_STATE_ERROR: An unrecoverable error occurred, e.g. link recovery
+ *	failed. Fail all SCSI commands with error code DID_ERROR.
+ */
+enum ufshcd_state {
+	UFSHCD_STATE_RESET,
+	UFSHCD_STATE_OPERATIONAL,
+	UFSHCD_STATE_EH_SCHEDULED_NON_FATAL,
+	UFSHCD_STATE_EH_SCHEDULED_FATAL,
+	UFSHCD_STATE_ERROR,
+};
+
 enum ufshcd_quirks {
 	/* Interrupt aggregation support is broken */
 	UFSHCD_QUIRK_BROKEN_INTR_AGGR			= 1 << 0,
@@ -687,7 +708,7 @@ struct ufs_hba_monitor {
  * @tmf_tag_set: TMF tag set.
  * @tmf_queue: Used to allocate TMF tags.
  * @pwr_done: completion for power mode change
- * @ufshcd_state: UFSHCD states
+ * @ufshcd_state: UFSHCD state
  * @eh_flags: Error handling flags
  * @intr_mask: Interrupt Mask Bits
  * @ee_ctrl_mask: Exception event control mask
@@ -785,7 +806,7 @@ struct ufs_hba {
 	struct mutex uic_cmd_mutex;
 	struct completion *uic_async_done;
 
-	u32 ufshcd_state;
+	enum ufshcd_state ufshcd_state;
 	u32 eh_flags;
 	u32 intr_mask;
 	u16 ee_ctrl_mask; /* Exception event mask */

[PATCH v3 09/18] scsi: ufs: Remove several wmb() calls

[Bart Van Assche]

From arch/arm/include/asm/io.h

  #define __iowmb() wmb()
  [ ... ]
  #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })

From Documentation/memory-barriers.txt: "Note that, when using writel(), a
prior wmb() is not needed to guarantee that the cache coherent memory
writes have completed before writing to the MMIO region."

In other words, calling wmb() before writel() is not necessary. Hence
remove the wmb() calls that precede a writel() call. Remove the wmb()
calls that precede a ufshcd_send_command() call since the latter function
uses writel(). Remove the wmb() call from ufshcd_wait_for_dev_cmd()
since the following chain of events guarantees that the CPU will see
up-to-date LRB values:
* UFS controller writes to host memory.
* UFS controller posts completion interrupt after the memory writes from
  the previous step are visible to the CPU.
* complete(hba->dev_cmd.complete) is called from the UFS interrupt handler.
* The wait_for_completion(hba->dev_cmd.complete) call in
  ufshcd_wait_for_dev_cmd() returns.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f467630be7df..d1c3a984d803 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2769,8 +2769,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		ufshcd_release(hba);
 		goto out;
 	}
-	/* Make sure descriptors are ready before ringing the doorbell */
-	wmb();
 
 	ufshcd_send_command(hba, tag);
 out:
@@ -2880,8 +2878,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 	time_left = wait_for_completion_timeout(hba->dev_cmd.complete,
 			msecs_to_jiffies(max_timeout));
 
-	/* Make sure descriptors are ready before ringing the doorbell */
-	wmb();
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->dev_cmd.complete = NULL;
 	if (likely(time_left)) {
@@ -2960,8 +2956,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	hba->dev_cmd.complete = &wait;
 
 	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
-	/* Make sure descriptors are ready before ringing the doorbell */
-	wmb();
 
 	ufshcd_send_command(hba, tag);
 	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
@@ -6521,9 +6515,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	/* send command to the controller */
 	__set_bit(task_tag, &hba->outstanding_tasks);
 
-	/* Make sure descriptors are ready before ringing the task doorbell */
-	wmb();
-
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
 	/* Make sure that doorbell is committed immediately */
 	wmb();
@@ -6695,8 +6686,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	hba->dev_cmd.complete = &wait;
 
 	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
-	/* Make sure descriptors are ready before ringing the doorbell */
-	wmb();
 
 	ufshcd_send_command(hba, tag);
 	/*

RE: [PATCH v3 09/18] scsi: ufs: Remove several wmb() calls

[Avri Altman]

 
> From arch/arm/include/asm/io.h
> 
>   #define __iowmb() wmb()
>   [ ... ]
>   #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
> 
> From Documentation/memory-barriers.txt: "Note that, when using writel(),
> a
> prior wmb() is not needed to guarantee that the cache coherent memory
> writes have completed before writing to the MMIO region."
> 
> In other words, calling wmb() before writel() is not necessary. Hence
> remove the wmb() calls that precede a writel() call. Remove the wmb()
> calls that precede a ufshcd_send_command() call since the latter function
> uses writel(). Remove the wmb() call from ufshcd_wait_for_dev_cmd()
> since the following chain of events guarantees that the CPU will see
> up-to-date LRB values:
> * UFS controller writes to host memory.
> * UFS controller posts completion interrupt after the memory writes from
>   the previous step are visible to the CPU.
> * complete(hba->dev_cmd.complete) is called from the UFS interrupt
> handler.
> * The wait_for_completion(hba->dev_cmd.complete) call in
>   ufshcd_wait_for_dev_cmd() returns.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Tested-by: Avri altman <avri.altman@wdc.com>


> ---
>  drivers/scsi/ufs/ufshcd.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f467630be7df..d1c3a984d803 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2769,8 +2769,6 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>                 ufshcd_release(hba);
>                 goto out;
>         }
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
> 
>         ufshcd_send_command(hba, tag);
>  out:
> @@ -2880,8 +2878,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba
> *hba,
>         time_left = wait_for_completion_timeout(hba->dev_cmd.complete,
>                         msecs_to_jiffies(max_timeout));
> 
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         hba->dev_cmd.complete = NULL;
>         if (likely(time_left)) {
> @@ -2960,8 +2956,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>         hba->dev_cmd.complete = &wait;
> 
>         ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp-
> >ucd_req_ptr);
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
> 
>         ufshcd_send_command(hba, tag);
>         err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
> @@ -6521,9 +6515,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba
> *hba,
>         /* send command to the controller */
>         __set_bit(task_tag, &hba->outstanding_tasks);
> 
> -       /* Make sure descriptors are ready before ringing the task doorbell */
> -       wmb();
> -
>         ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
>         /* Make sure that doorbell is committed immediately */
>         wmb();
> @@ -6695,8 +6686,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>         hba->dev_cmd.complete = &wait;
> 
>         ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp-
> >ucd_req_ptr);
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
> 
>         ufshcd_send_command(hba, tag);
>         /*

[PATCH v3 10/18] scsi: ufs: Inline ufshcd_outstanding_req_clear()

[Bart Van Assche]

Inline ufshcd_outstanding_req_clear() since it only has one caller and
since its body is only one line long.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d1c3a984d803..4c6d832f5e81 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -742,16 +742,6 @@ static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
 		ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
 }
 
-/**
- * ufshcd_outstanding_req_clear - Clear a bit in outstanding request field
- * @hba: per adapter instance
- * @tag: position of the bit to be cleared
- */
-static inline void ufshcd_outstanding_req_clear(struct ufs_hba *hba, int tag)
-{
-	clear_bit(tag, &hba->outstanding_reqs);
-}
-
 /**
  * ufshcd_get_lists_status - Check UCRDY, UTRLRDY and UTMRLRDY
  * @reg: Register value of host controller status
@@ -2899,7 +2889,7 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 		 * we also need to clear the outstanding_request
 		 * field in hba
 		 */
-		ufshcd_outstanding_req_clear(hba, lrbp->task_tag);
+		clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
 	}
 
 	return err;

Re: [PATCH v3 10/18] scsi: ufs: Inline ufshcd_outstanding_req_clear()

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Inline ufshcd_outstanding_req_clear() since it only has one caller
> and
> 
> since its body is only one line long.
> 
> 
> 
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> 
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> 
> Cc: Can Guo <cang@codeaurora.org>
> 
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v3 11/18] scsi: ufs: Revert "Utilize Transfer Request List Completion Notification Register"

[Bart Van Assche]

Using the UTRLCNR register involves two MMIO accesses in the hot path while
using the doorbell register only involves a single MMIO access. Since MMIO
accesses take time, do not use the UTRLCNR register. The spinlock contention
on the SCSI host lock that is reintroduced by this patch will be addressed
by a later patch.

This reverts commit 6f7151729647e58ac7c522081255fd0c07b38105.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 52 +++++++++++----------------------------
 drivers/scsi/ufs/ufshcd.h |  5 ----
 drivers/scsi/ufs/ufshci.h |  1 -
 3 files changed, 15 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 4c6d832f5e81..cb588b705fbb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2087,6 +2087,7 @@ static inline
 void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 {
 	struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
+	unsigned long flags;
 
 	lrbp->issue_time_stamp = ktime_get();
 	lrbp->compl_time_stamp = ktime_set(0, 0);
@@ -2095,19 +2096,10 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 	ufshcd_clk_scaling_start_busy(hba);
 	if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
 		ufshcd_start_monitor(hba, lrbp);
-	if (ufshcd_has_utrlcnr(hba)) {
-		set_bit(task_tag, &hba->outstanding_reqs);
-		ufshcd_writel(hba, 1 << task_tag,
-			      REG_UTP_TRANSFER_REQ_DOOR_BELL);
-	} else {
-		unsigned long flags;
-
-		spin_lock_irqsave(hba->host->host_lock, flags);
-		set_bit(task_tag, &hba->outstanding_reqs);
-		ufshcd_writel(hba, 1 << task_tag,
-			      REG_UTP_TRANSFER_REQ_DOOR_BELL);
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
-	}
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	set_bit(task_tag, &hba->outstanding_reqs);
+	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	/* Make sure that doorbell is committed immediately */
 	wmb();
 }
@@ -5234,17 +5226,17 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 }
 
 /**
- * ufshcd_trc_handler - handle transfer requests completion
+ * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
- * @use_utrlcnr: get completed requests from UTRLCNR
  *
  * Returns
  *  IRQ_HANDLED - If interrupt is valid
  *  IRQ_NONE    - If invalid interrupt
  */
-static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
+static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 {
-	unsigned long completed_reqs = 0;
+	unsigned long completed_reqs, flags;
+	u32 tr_doorbell;
 
 	/* Resetting interrupt aggregation counters first and reading the
 	 * DOOR_BELL afterward allows us to handle all the completed requests.
@@ -5257,24 +5249,10 @@ static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
 	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
 		ufshcd_reset_intr_aggr(hba);
 
-	if (use_utrlcnr) {
-		u32 utrlcnr;
-
-		utrlcnr = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_LIST_COMPL);
-		if (utrlcnr) {
-			ufshcd_writel(hba, utrlcnr,
-				      REG_UTP_TRANSFER_REQ_LIST_COMPL);
-			completed_reqs = utrlcnr;
-		}
-	} else {
-		unsigned long flags;
-		u32 tr_doorbell;
-
-		spin_lock_irqsave(hba->host->host_lock, flags);
-		tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-		completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
-	}
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	if (completed_reqs) {
 		__ufshcd_transfer_req_compl(hba, completed_reqs);
@@ -5756,7 +5734,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 /* Complete requests that have door-bell cleared */
 static void ufshcd_complete_requests(struct ufs_hba *hba)
 {
-	ufshcd_trc_handler(hba, false);
+	ufshcd_transfer_req_compl(hba);
 	ufshcd_tmc_handler(hba);
 }
 
@@ -6397,7 +6375,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 		retval |= ufshcd_tmc_handler(hba);
 
 	if (intr_status & UTP_TRANSFER_REQ_COMPL)
-		retval |= ufshcd_trc_handler(hba, ufshcd_has_utrlcnr(hba));
+		retval |= ufshcd_transfer_req_compl(hba);
 
 	return retval;
 }
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 8dcf0df770a2..a44baec43dd5 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1176,11 +1176,6 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba)
 	return ufshcd_readl(hba, REG_UFS_VERSION);
 }
 
-static inline bool ufshcd_has_utrlcnr(struct ufs_hba *hba)
-{
-	return (hba->ufs_version >= ufshci_version(3, 0));
-}
-
 static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
 			bool up, enum ufs_notify_change_status status)
 {
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index 5affb1fce5ad..de95be5d11d4 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -39,7 +39,6 @@ enum {
 	REG_UTP_TRANSFER_REQ_DOOR_BELL		= 0x58,
 	REG_UTP_TRANSFER_REQ_LIST_CLEAR		= 0x5C,
 	REG_UTP_TRANSFER_REQ_LIST_RUN_STOP	= 0x60,
-	REG_UTP_TRANSFER_REQ_LIST_COMPL		= 0x64,
 	REG_UTP_TASK_REQ_LIST_BASE_L		= 0x70,
 	REG_UTP_TASK_REQ_LIST_BASE_H		= 0x74,
 	REG_UTP_TASK_REQ_DOOR_BELL		= 0x78,

Re: [PATCH v3 11/18] scsi: ufs: Revert "Utilize Transfer Request List Completion Notification Register"

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Using the UTRLCNR register involves two MMIO accesses in the hot path
> while
> 
> using the doorbell register only involves a single MMIO access. Since
> MMIO
> 
> accesses take time, do not use the UTRLCNR register. The spinlock
> contention
> 
> on the SCSI host lock that is reintroduced by this patch will be
> addressed
> 
> by a later patch.
> 
> 
> 
> This reverts commit 6f7151729647e58ac7c522081255fd0c07b38105.

Bart, 
This commit is the key change in "Optimize host lock on TR send/compl
paths and utilize UTRLCNR"
https://patchwork.kernel.org/project/linux-scsi/cover/1621845419-14194-1-git-send-email-cang@codeaurora.org/.

How did you compare the performance gain/loss after reverting this
commit?

Kind regards,
Bean

Re: [PATCH v3 11/18] scsi: ufs: Revert "Utilize Transfer Request List Completion Notification Register"

[Bart Van Assche]

On 7/29/21 1:03 AM, Bean Huo wrote:
> On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
>> Using the UTRLCNR register involves two MMIO accesses in the hot path
>> while
>>
>> using the doorbell register only involves a single MMIO access. Since
>> MMIO
>>
>> accesses take time, do not use the UTRLCNR register. The spinlock
>> contention
>>
>> on the SCSI host lock that is reintroduced by this patch will be
>> addressed
>>
>> by a later patch.
>>
>>
>>
>> This reverts commit 6f7151729647e58ac7c522081255fd0c07b38105.
> 
> Bart,
> This commit is the key change in "Optimize host lock on TR send/compl
> paths and utilize UTRLCNR"
> https://patchwork.kernel.org/project/linux-scsi/cover/1621845419-14194-1-git-send-email-cang@codeaurora.org/.
> 
> How did you compare the performance gain/loss after reverting this
> commit?

Hi Bean,

I measured the performance impact of patches 11 and 12 combined. In a 4 
KB read IOPS test I see that these two patches combined improve 
performance by 1%. This illustrates that the two MMIO accesses of the 
UTRLCNR register are slower than the single MMIO access of the doorbell 
register.

Thanks,

Bart.

Re: [PATCH v3 11/18] scsi: ufs: Revert "Utilize Transfer Request List Completion Notification Register"

[Bart Van Assche]

On 7/29/21 9:10 AM, Bart Van Assche wrote:
> On 7/29/21 1:03 AM, Bean Huo wrote:
>> How did you compare the performance gain/loss after reverting this
>> commit?
> 
> I measured the performance impact of patches 11 and 12 combined. In a 4 
> KB read IOPS test I see that these two patches combined improve 
> performance by 1%. This illustrates that the two MMIO accesses of the 
> UTRLCNR register are slower than the single MMIO access of the doorbell 
> register.

A correction: I wanted to refer to the combination of patches 11 and 13 
instead of 11 and 12.

Bart.

Re: [PATCH v3 11/18] scsi: ufs: Revert "Utilize Transfer Request List Completion Notification Register"

[Bean Huo]

On Thu, 2021-07-29 at 09:13 -0700, Bart Van Assche wrote:
> > I measured the performance impact of patches 11 and 12 combined. In
> > a 4 
> > KB read IOPS test I see that these two patches combined improve 
> > performance by 1%. This illustrates that the two MMIO accesses of
> > the 
> > UTRLCNR register are slower than the single MMIO access of the
> > doorbell 
> > register.
> 
> 
> A correction: I wanted to refer to the combination of patches 11 and
> 13 
> 
> instead of 11 and 12.
> 
> 
> 
> Bart.

Bart,
thanks for your explanation. 

I tested this commit before, indeed there is performance improvement.
Based on your comments, I will verify your statement.

Bean

Re: [PATCH v3 11/18] scsi: ufs: Revert "Utilize Transfer Request List Completion Notification Register"

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Using the UTRLCNR register involves two MMIO accesses in the hot path
> while
> using the doorbell register only involves a single MMIO access. Since
> MMIO
> accesses take time, do not use the UTRLCNR register. The spinlock
> contention
> on the SCSI host lock that is reintroduced by this patch will be
> addressed
> by a later patch.
> 
> This reverts commit 6f7151729647e58ac7c522081255fd0c07b38105.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 

Hi Bart, 

I did the comparison test on my platform, it is very difficult to get a
very clear and fair result between two changes. but lamely speaking, on
the small chunk size read/write, your changes wins. but on the big
chunk size, It is not very clear, the gap between the two changes can
be ignored.

Tested-by: Bean Huo <beanhuo@micron.com>

Bean

Re: [PATCH v3 11/18] scsi: ufs: Revert "Utilize Transfer Request List Completion Notification Register"

[Bart Van Assche]

On 8/2/21 8:24 AM, Bean Huo wrote:
> I did the comparison test on my platform, it is very difficult to get a
> very clear and fair result between two changes. but lamely speaking, on
> the small chunk size read/write, your changes wins. but on the big
> chunk size, It is not very clear, the gap between the two changes can
> be ignored.
> 
> Tested-by: Bean Huo <beanhuo@micron.com>

Thanks for having tested this patch :-)

Bart.

[PATCH v3 12/18] scsi: ufs: Optimize serialization of setup_xfer_req() calls

[Bart Van Assche]

Reduce the number of times the host lock is taken in the hot path.
Additionally, inline ufshcd_vops_setup_xfer_req() because that function is
too short to keep it.

Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Fixes: a45f937110fa ("scsi: ufs: Optimize host lock on transfer requests send/compl paths")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c |  3 ++-
 drivers/scsi/ufs/ufshcd.h | 12 ------------
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index cb588b705fbb..436d814f4c1e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2091,12 +2091,13 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 
 	lrbp->issue_time_stamp = ktime_get();
 	lrbp->compl_time_stamp = ktime_set(0, 0);
-	ufshcd_vops_setup_xfer_req(hba, task_tag, (lrbp->cmd ? true : false));
 	ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
 	ufshcd_clk_scaling_start_busy(hba);
 	if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
 		ufshcd_start_monitor(hba, lrbp);
 	spin_lock_irqsave(hba->host->host_lock, flags);
+	if (hba->vops && hba->vops->setup_xfer_req)
+		hba->vops->setup_xfer_req(hba, task_tag, !!lrbp->cmd);
 	set_bit(task_tag, &hba->outstanding_reqs);
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index a44baec43dd5..6df847facd1d 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1237,18 +1237,6 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
 	return -ENOTSUPP;
 }
 
-static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
-					bool is_scsi_cmd)
-{
-	if (hba->vops && hba->vops->setup_xfer_req) {
-		unsigned long flags;
-
-		spin_lock_irqsave(hba->host->host_lock, flags);
-		hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
-	}
-}
-
 static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,
 					int tag, u8 tm_function)
 {

Re: [PATCH v3 12/18] scsi: ufs: Optimize serialization of setup_xfer_req() calls

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> -       ufshcd_vops_setup_xfer_req(hba, task_tag, (lrbp->cmd ? true :
> false));
> 
>         ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
> 
>         ufshcd_clk_scaling_start_busy(hba);
> 
>         if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
> 
>                 ufshcd_start_monitor(hba, lrbp);
> 
>         spin_lock_irqsave(hba->host->host_lock, flags);
> 
> +       if (hba->vops && hba->vops->setup_xfer_req)
> 
> +               hba->vops->setup_xfer_req(hba, task_tag, !!lrbp->

Nice!

Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v3 13/18] scsi: ufs: Optimize SCSI command processing

[Bart Van Assche]

Use a spinlock to protect hba->outstanding_reqs instead of using atomic
operations to update this member variable.

This patch is a performance improvement because it reduces the number of
atomic operations in the hot path (test_and_clear_bit()) and because it
reduces the lock contention on the SCSI host lock. On my test setup this
patch improves IOPS by about 1%.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 29 ++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 436d814f4c1e..a3ad83a3bae0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2095,12 +2095,14 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 	ufshcd_clk_scaling_start_busy(hba);
 	if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
 		ufshcd_start_monitor(hba, lrbp);
-	spin_lock_irqsave(hba->host->host_lock, flags);
+
+	spin_lock_irqsave(&hba->outstanding_lock, flags);
 	if (hba->vops && hba->vops->setup_xfer_req)
 		hba->vops->setup_xfer_req(hba, task_tag, !!lrbp->cmd);
-	set_bit(task_tag, &hba->outstanding_reqs);
+	__set_bit(task_tag, &hba->outstanding_reqs);
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+
 	/* Make sure that doorbell is committed immediately */
 	wmb();
 }
@@ -2882,7 +2884,9 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 		 * we also need to clear the outstanding_request
 		 * field in hba
 		 */
-		clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
+		spin_lock_irqsave(&hba->outstanding_lock, flags);
+		__clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
+		spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 	}
 
 	return err;
@@ -5194,8 +5198,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 	bool update_scaling = false;
 
 	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
-		if (!test_and_clear_bit(index, &hba->outstanding_reqs))
-			continue;
 		lrbp = &hba->lrb[index];
 		lrbp->compl_time_stamp = ktime_get();
 		cmd = lrbp->cmd;
@@ -5250,10 +5252,14 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
 		ufshcd_reset_intr_aggr(hba);
 
-	spin_lock_irqsave(hba->host->host_lock, flags);
+	spin_lock_irqsave(&hba->outstanding_lock, flags);
 	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
+	completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
+	WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
+		  "completed: %#lx; outstanding: %#lx\n", completed_reqs,
+		  hba->outstanding_reqs);
+	hba->outstanding_reqs &= ~completed_reqs;
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 
 	if (completed_reqs) {
 		__ufshcd_transfer_req_compl(hba, completed_reqs);
@@ -9340,10 +9346,11 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 	hba = shost_priv(host);
 	hba->host = host;
 	hba->dev = dev;
-	*hba_handle = hba;
 	hba->dev_ref_clk_freq = REF_CLK_FREQ_INVAL;
-
 	INIT_LIST_HEAD(&hba->clk_list_head);
+	spin_lock_init(&hba->outstanding_lock);
+
+	*hba_handle = hba;
 
 out_error:
 	return err;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 6df847facd1d..91b0b278469d 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -695,6 +695,7 @@ struct ufs_hba_monitor {
  * @lrb: local reference block
  * @cmd_queue: Used to allocate command tags from hba->host->tag_set.
  * @outstanding_tasks: Bits representing outstanding task requests
+ * @outstanding_lock: Protects @outstanding_reqs.
  * @outstanding_reqs: Bits representing outstanding transfer requests
  * @capabilities: UFS Controller Capabilities
  * @nutrs: Transfer Request Queue depth supported by controller
@@ -781,6 +782,7 @@ struct ufs_hba {
 	struct ufshcd_lrb *lrb;
 
 	unsigned long outstanding_tasks;
+	spinlock_t outstanding_lock;
 	unsigned long outstanding_reqs;
 
 	u32 capabilities;

Re: [PATCH v3 13/18] scsi: ufs: Optimize SCSI command processing

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> 
> index 436d814f4c1e..a3ad83a3bae0 100644
> 
> --- a/drivers/scsi/ufs/ufshcd.c
> 
> +++ b/drivers/scsi/ufs/ufshcd.c
> 
> @@ -2095,12 +2095,14 @@ void ufshcd_send_command(struct ufs_hba *hba,
> unsigned int task_tag)
> 
>         ufshcd_clk_scaling_start_busy(hba);
> 
>         if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
> 
>                 ufshcd_start_monitor(hba, lrbp);
> 
> -       spin_lock_irqsave(hba->host->host_lock, flags);
> 
> +
> 
> +       spin_lock_irqsave(&hba->outstanding_lock, flags);
> 
>         if (hba->vops && hba->vops->setup_xfer_req)
> 
>                 hba->vops->setup_xfer_req(hba, task_tag, !!lrbp-
> >cmd);

Bart,

Removing hba->host->host_lock, use hba->outstanding_lock, the issue
fixed by your patch [12/18] still be fixed?

Bean

Re: [PATCH v3 13/18] scsi: ufs: Optimize SCSI command processing

[Bart Van Assche]

On 7/29/21 2:12 AM, Bean Huo wrote:
> Removing hba->host->host_lock, use hba->outstanding_lock, the issue
> fixed by your patch [12/18] still be fixed?

Yes. My understanding is that setup_xfer_req() calls must be serialized 
against each other but not against any other code that is protected by 
the host lock.

Thanks,

Bart.

Re: [PATCH v3 13/18] scsi: ufs: Optimize SCSI command processing

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Use a spinlock to protect hba->outstanding_reqs instead of using
> atomic
> 
> operations to update this member variable.
> 
> 
> 
> This patch is a performance improvement because it reduces the number
> of
> 
> atomic operations in the hot path (test_and_clear_bit()) and because
> it
> 
> reduces the lock contention on the SCSI host lock. On my test setup
> this
> 
> patch improves IOPS by about 1%.
> 
> 
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> 
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> 
> Cc: Can Guo <cang@codeaurora.org>
> 
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> 
> Cc: Avri Altman <avri.altman@wdc.com>
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v3 14/18] scsi: ufs: Fix the SCSI abort handler

[Bart Van Assche]

Make the following changes in ufshcd_abort():
- Return FAILED instead of SUCCESS if the abort handler notices that a SCSI
  command has already been completed. Returning SUCCESS in this case
  triggers a use-after-free and may trigger a kernel crash.
- Fix the code for aborting SCSI commands submitted to a WLUN.

The current approach for aborting SCSI commands that have been submitted to
a WLUN and that timed out is as follows:
- Report to the SCSI core that the command has completed successfully.
  Let the block layer free any data buffers associated with the command.
- Mark the command as outstanding in 'outstanding_reqs'.
- If the block layer tries to reuse the tag associated with the aborted
  command, busy-wait until the tag is freed.

This approach can result in:
- Memory corruption if the controller accesses the data buffer after the
  block layer has freed the associated data buffers.
- A race condition if ufshcd_queuecommand() or ufshcd_exec_dev_cmd()
  checks the bit that corresponds to an aborted command in 'outstanding_reqs'
  after it has been cleared and before it is reset.
- High energy consumption if ufshcd_queuecommand() repeatedly returns
  SCSI_MLQUEUE_HOST_BUSY.

Fix this by reporting to the SCSI error handler that aborting a SCSI
command failed if the SCSI command was submitted to a WLUN.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Fixes: 7a7e66c65d41 ("scsi: ufs: Fix a race condition between ufshcd_abort() and eh_work()")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 54 ++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a3ad83a3bae0..c35e101c5834 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2724,15 +2724,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	WARN_ON(ufshcd_is_clkgating_allowed(hba) &&
 		(hba->clk_gating.state != CLKS_ON));
 
-	if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
-		if (hba->pm_op_in_progress)
-			set_host_byte(cmd, DID_BAD_TARGET);
-		else
-			err = SCSI_MLQUEUE_HOST_BUSY;
-		ufshcd_release(hba);
-		goto out;
-	}
-
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	lrbp->cmd = cmd;
@@ -2929,11 +2920,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	req->timeout = msecs_to_jiffies(2 * timeout);
 	blk_mq_start_request(req);
 
-	if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
-		err = -EBUSY;
-		goto out;
-	}
-
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
@@ -6922,19 +6908,19 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	unsigned int tag = cmd->request->tag;
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	unsigned long flags;
-	int err = 0;
+	int err = FAILED;
 	u32 reg;
 
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
 	ufshcd_hold(hba, false);
 	reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-	/* If command is already aborted/completed, return SUCCESS */
+	/* If command is already aborted/completed, return FAILED. */
 	if (!(test_bit(tag, &hba->outstanding_reqs))) {
 		dev_err(hba->dev,
 			"%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
 			__func__, tag, hba->outstanding_reqs, reg);
-		goto out;
+		goto release;
 	}
 
 	/* Print Transfer Request of aborted task */
@@ -6963,7 +6949,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		dev_err(hba->dev,
 		"%s: cmd was completed, but without a notifying intr, tag = %d",
 		__func__, tag);
-		goto cleanup;
+		__ufshcd_transfer_req_compl(hba, 1UL << tag);
+		goto release;
 	}
 
 	/*
@@ -6976,36 +6963,33 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	 */
 	if (lrbp->lun == UFS_UPIU_UFS_DEVICE_WLUN) {
 		ufshcd_update_evt_hist(hba, UFS_EVT_ABORT, lrbp->lun);
-		__ufshcd_transfer_req_compl(hba, (1UL << tag));
-		set_bit(tag, &hba->outstanding_reqs);
+
 		spin_lock_irqsave(host->host_lock, flags);
 		hba->force_reset = true;
 		ufshcd_schedule_eh_work(hba);
 		spin_unlock_irqrestore(host->host_lock, flags);
-		goto out;
+		goto release;
 	}
 
 	/* Skip task abort in case previous aborts failed and report failure */
-	if (lrbp->req_abort_skip)
-		err = -EIO;
-	else
-		err = ufshcd_try_to_abort_task(hba, tag);
+	if (lrbp->req_abort_skip) {
+		dev_err(hba->dev, "%s: skipping abort\n", __func__);
+		ufshcd_set_req_abort_skip(hba, hba->outstanding_reqs);
+		goto release;
+	}
 
-	if (!err) {
-cleanup:
-		__ufshcd_transfer_req_compl(hba, (1UL << tag));
-out:
-		err = SUCCESS;
-	} else {
+	err = ufshcd_try_to_abort_task(hba, tag);
+	if (err) {
 		dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
 		ufshcd_set_req_abort_skip(hba, hba->outstanding_reqs);
 		err = FAILED;
+		goto release;
 	}
 
-	/*
-	 * This ufshcd_release() corresponds to the original scsi cmd that got
-	 * aborted here (as we won't get any IRQ for it).
-	 */
+	err = SUCCESS;
+
+release:
+	/* Matches the ufshcd_hold() call at the start of this function. */
 	ufshcd_release(hba);
 	return err;
 }

Re: [PATCH v3 14/18] scsi: ufs: Fix the SCSI abort handler

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Make the following changes in ufshcd_abort():
> - Return FAILED instead of SUCCESS if the abort handler notices that
> a SCSI
>   command has already been completed. Returning SUCCESS in this case
>   triggers a use-after-free and may trigger a kernel crash.
> - Fix the code for aborting SCSI commands submitted to a WLUN.
> 
> The current approach for aborting SCSI commands that have been
> submitted to
> a WLUN and that timed out is as follows:
> - Report to the SCSI core that the command has completed
> successfully.
>   Let the block layer free any data buffers associated with the
> command.
> - Mark the command as outstanding in 'outstanding_reqs'.
> - If the block layer tries to reuse the tag associated with the
> aborted
>   command, busy-wait until the tag is freed.
> 
> This approach can result in:
> - Memory corruption if the controller accesses the data buffer after
> the
>   block layer has freed the associated data buffers.
> - A race condition if ufshcd_queuecommand() or ufshcd_exec_dev_cmd()
>   checks the bit that corresponds to an aborted command in
> 'outstanding_reqs'
>   after it has been cleared and before it is reset.
> - High energy consumption if ufshcd_queuecommand() repeatedly returns
>   SCSI_MLQUEUE_HOST_BUSY.
> 
> Fix this by reporting to the SCSI error handler that aborting a SCSI
> command failed if the SCSI command was submitted to a WLUN.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Fixes: 7a7e66c65d41 ("scsi: ufs: Fix a race condition between
> ufshcd_abort() and eh_work()")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 


Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v3 15/18] scsi: ufs: Request sense data asynchronously

[Bart Van Assche]

Clearing a unit attention synchronously from inside the UFS error handler
may trigger the following deadlock:
- ufshcd_err_handler() calls ufshcd_err_handling_unprepare() and the latter
  function calls ufshcd_clear_ua_wluns().
- ufshcd_clear_ua_wluns() submits a REQUEST SENSE command and that command
  activates the SCSI error handler.
- The SCSI error handler calls ufshcd_host_reset_and_restore().
- ufshcd_host_reset_and_restore() executes the following code:
  ufshcd_schedule_eh_work(hba);
  flush_work(&hba->eh_work);

This sequence results in a deadlock (circular wait). Fix this by requesting
sense data asynchronously.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 64 ++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c35e101c5834..75730a43fcca 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7820,8 +7820,39 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 	return ret;
 }
 
+static void ufshcd_request_sense_done(struct request *rq, blk_status_t error)
+{
+	if (error != BLK_STS_OK)
+		pr_err("%s: REQUEST SENSE failed (%d)", __func__, error);
+	blk_put_request(rq);
+}
+
 static int
-ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp);
+ufshcd_request_sense_async(struct ufs_hba *hba, struct scsi_device *sdev)
+{
+	/*
+	 * From SPC-6: the REQUEST SENSE command with any allocation length
+	 * clears the sense data.
+	 */
+	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, 0, 0};
+	struct scsi_request *rq;
+	struct request *req;
+
+	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN, /*flags=*/0);
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	rq = scsi_req(req);
+	rq->cmd_len = ARRAY_SIZE(cmd);
+	memcpy(rq->cmd, cmd, rq->cmd_len);
+	rq->retries = 3;
+	req->timeout = 1 * HZ;
+	req->rq_flags |= RQF_PM | RQF_QUIET;
+
+	blk_execute_rq_nowait(/*bd_disk=*/NULL, req, /*at_head=*/true,
+			      ufshcd_request_sense_done);
+	return 0;
+}
 
 static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
 {
@@ -7849,7 +7880,7 @@ static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
 	if (ret)
 		goto out_err;
 
-	ret = ufshcd_send_request_sense(hba, sdp);
+	ret = ufshcd_request_sense_async(hba, sdp);
 	scsi_device_put(sdp);
 out_err:
 	if (ret)
@@ -8444,35 +8475,6 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
 	}
 }
 
-static int
-ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp)
-{
-	unsigned char cmd[6] = {REQUEST_SENSE,
-				0,
-				0,
-				0,
-				UFS_SENSE_SIZE,
-				0};
-	char *buffer;
-	int ret;
-
-	buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
-	if (!buffer) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	ret = scsi_execute(sdp, cmd, DMA_FROM_DEVICE, buffer,
-			UFS_SENSE_SIZE, NULL, NULL,
-			msecs_to_jiffies(1000), 3, 0, RQF_PM, NULL);
-	if (ret)
-		pr_err("%s: failed with err %d\n", __func__, ret);
-
-	kfree(buffer);
-out:
-	return ret;
-}
-
 /**
  * ufshcd_set_dev_pwr_mode - sends START STOP UNIT command to set device
  *			     power mode

Re: [PATCH v3 15/18] scsi: ufs: Request sense data asynchronously

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Clearing a unit attention synchronously from inside the UFS error
> handler
> may trigger the following deadlock:
> - ufshcd_err_handler() calls ufshcd_err_handling_unprepare() and the
> latter
>   function calls ufshcd_clear_ua_wluns().
> - ufshcd_clear_ua_wluns() submits a REQUEST SENSE command and that
> command
>   activates the SCSI error handler.
> - The SCSI error handler calls ufshcd_host_reset_and_restore().
> - ufshcd_host_reset_and_restore() executes the following code:
>   ufshcd_schedule_eh_work(hba);
>   flush_work(&hba->eh_work);
> 
> This sequence results in a deadlock (circular wait). Fix this by
> requesting
> sense data asynchronously.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Bart Van Assche]

Use the SCSI error handler instead of a custom error handling strategy.
This change reduces the number of potential races in the UFS drivers since
the UFS error handler and the SCSI error handler no longer run concurrently.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 102 ++++++++++++++++++++------------------
 drivers/scsi/ufs/ufshcd.h |   4 --
 2 files changed, 55 insertions(+), 51 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 75730a43fcca..8d87fb214281 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -17,6 +17,8 @@
 #include <linux/blk-pm.h>
 #include <linux/blkdev.h>
 #include <scsi/scsi_driver.h>
+#include <scsi/scsi_transport.h>
+#include "../scsi_transport_api.h"
 #include "ufshcd.h"
 #include "ufs_quirks.h"
 #include "unipro.h"
@@ -232,7 +234,6 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up);
 static irqreturn_t ufshcd_intr(int irq, void *__hba);
 static int ufshcd_change_power_mode(struct ufs_hba *hba,
 			     struct ufs_pa_layer_attr *pwr_mode);
-static void ufshcd_schedule_eh_work(struct ufs_hba *hba);
 static int ufshcd_setup_hba_vreg(struct ufs_hba *hba, bool on);
 static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on);
 static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
@@ -3906,6 +3907,35 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 }
 EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
 
+static inline bool ufshcd_is_saved_err_fatal(struct ufs_hba *hba)
+{
+	lockdep_assert_held(hba->host->host_lock);
+
+	return (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR) ||
+	       (hba->saved_err & (INT_FATAL_ERRORS | UFSHCD_UIC_HIBERN8_MASK));
+}
+
+static void ufshcd_schedule_eh(struct ufs_hba *hba)
+{
+	bool schedule_eh = false;
+	unsigned long flags;
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	/* handle fatal errors only when link is not in error state */
+	if (hba->ufshcd_state != UFSHCD_STATE_ERROR) {
+		if (hba->force_reset || ufshcd_is_link_broken(hba) ||
+		    ufshcd_is_saved_err_fatal(hba))
+			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
+		else
+			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
+		schedule_eh = true;
+	}
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+	if (schedule_eh)
+		scsi_schedule_eh(hba->host);
+}
+
 /**
  * ufshcd_uic_pwr_ctrl - executes UIC commands (which affects the link power
  * state) and waits for it to take effect.
@@ -3926,6 +3956,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 {
 	DECLARE_COMPLETION_ONSTACK(uic_async_done);
 	unsigned long flags;
+	bool schedule_eh = false;
 	u8 status;
 	int ret;
 	bool reenable_intr = false;
@@ -3995,10 +4026,14 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 		ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
 	if (ret) {
 		ufshcd_set_link_broken(hba);
-		ufshcd_schedule_eh_work(hba);
+		schedule_eh = true;
 	}
+
 out_unlock:
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+	if (schedule_eh)
+		ufshcd_schedule_eh(hba);
 	mutex_unlock(&hba->uic_cmd_mutex);
 
 	return ret;
@@ -5802,27 +5837,6 @@ static bool ufshcd_quirk_dl_nac_errors(struct ufs_hba *hba)
 	return err_handling;
 }
 
-/* host lock must be held before calling this func */
-static inline bool ufshcd_is_saved_err_fatal(struct ufs_hba *hba)
-{
-	return (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR) ||
-	       (hba->saved_err & (INT_FATAL_ERRORS | UFSHCD_UIC_HIBERN8_MASK));
-}
-
-/* host lock must be held before calling this func */
-static inline void ufshcd_schedule_eh_work(struct ufs_hba *hba)
-{
-	/* handle fatal errors only when link is not in error state */
-	if (hba->ufshcd_state != UFSHCD_STATE_ERROR) {
-		if (hba->force_reset || ufshcd_is_link_broken(hba) ||
-		    ufshcd_is_saved_err_fatal(hba))
-			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
-		else
-			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;
-		queue_work(hba->eh_wq, &hba->eh_work);
-	}
-}
-
 static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
 {
 	down_write(&hba->clk_scaling_lock);
@@ -5956,11 +5970,11 @@ static bool ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba)
 
 /**
  * ufshcd_err_handler - handle UFS errors that require s/w attention
- * @work: pointer to work structure
+ * @host: SCSI host pointer
  */
-static void ufshcd_err_handler(struct work_struct *work)
+static void ufshcd_err_handler(struct Scsi_Host *host)
 {
-	struct ufs_hba *hba;
+	struct ufs_hba *hba = shost_priv(host);
 	unsigned long flags;
 	bool err_xfer = false;
 	bool err_tm = false;
@@ -5968,10 +5982,9 @@ static void ufshcd_err_handler(struct work_struct *work)
 	int tag;
 	bool needs_reset = false, needs_restore = false;
 
-	hba = container_of(work, struct ufs_hba, eh_work);
-
 	down(&hba->host_sem);
 	spin_lock_irqsave(hba->host->host_lock, flags);
+	hba->host->host_eh_scheduled = 0;
 	if (ufshcd_err_handling_should_stop(hba)) {
 		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
 			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
@@ -6285,7 +6298,6 @@ static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba, u32 intr_status)
 					 "host_regs: ");
 			ufshcd_print_pwr_info(hba);
 		}
-		ufshcd_schedule_eh_work(hba);
 		retval |= IRQ_HANDLED;
 	}
 	/*
@@ -6297,6 +6309,10 @@ static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba, u32 intr_status)
 	hba->errors = 0;
 	hba->uic_error = 0;
 	spin_unlock(hba->host->host_lock);
+
+	if (queue_eh_work)
+		ufshcd_schedule_eh(hba);
+
 	return retval;
 }
 
@@ -6959,15 +6975,17 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	 * will be to send LU reset which, again, is a spec violation.
 	 * To avoid these unnecessary/illegal steps, first we clean up
 	 * the lrb taken by this cmd and re-set it in outstanding_reqs,
-	 * then queue the eh_work and bail.
+	 * then queue the error handler and bail.
 	 */
 	if (lrbp->lun == UFS_UPIU_UFS_DEVICE_WLUN) {
 		ufshcd_update_evt_hist(hba, UFS_EVT_ABORT, lrbp->lun);
 
 		spin_lock_irqsave(host->host_lock, flags);
 		hba->force_reset = true;
-		ufshcd_schedule_eh_work(hba);
 		spin_unlock_irqrestore(host->host_lock, flags);
+
+		ufshcd_schedule_eh(hba);
+
 		goto release;
 	}
 
@@ -7099,11 +7117,10 @@ static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd)
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->force_reset = true;
-	ufshcd_schedule_eh_work(hba);
 	dev_err(hba->dev, "%s: reset in progress - 1\n", __func__);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
-	flush_work(&hba->eh_work);
+	ufshcd_err_handler(hba->host);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (hba->ufshcd_state == UFSHCD_STATE_ERROR)
@@ -8463,8 +8480,6 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
 	if (hba->is_powered) {
 		ufshcd_exit_clk_scaling(hba);
 		ufshcd_exit_clk_gating(hba);
-		if (hba->eh_wq)
-			destroy_workqueue(hba->eh_wq);
 		ufs_debugfs_hba_exit(hba);
 		ufshcd_variant_hba_exit(hba);
 		ufshcd_setup_vreg(hba, false);
@@ -9303,6 +9318,10 @@ static int ufshcd_set_dma_mask(struct ufs_hba *hba)
 	return dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(32));
 }
 
+static struct scsi_transport_template ufshcd_transport_template = {
+	.eh_strategy_handler = ufshcd_err_handler,
+};
+
 /**
  * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
  * @dev: pointer to device handle
@@ -9329,6 +9348,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 		err = -ENOMEM;
 		goto out_error;
 	}
+	host->transportt = &ufshcd_transport_template;
 	hba = shost_priv(host);
 	hba->host = host;
 	hba->dev = dev;
@@ -9367,7 +9387,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	int err;
 	struct Scsi_Host *host = hba->host;
 	struct device *dev = hba->dev;
-	char eh_wq_name[sizeof("ufs_eh_wq_00")];
 
 	if (!mmio_base) {
 		dev_err(hba->dev,
@@ -9421,17 +9440,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	hba->max_pwr_info.is_valid = false;
 
-	/* Initialize work queues */
-	snprintf(eh_wq_name, sizeof(eh_wq_name), "ufs_eh_wq_%d",
-		 hba->host->host_no);
-	hba->eh_wq = create_singlethread_workqueue(eh_wq_name);
-	if (!hba->eh_wq) {
-		dev_err(hba->dev, "%s: failed to create eh workqueue\n",
-				__func__);
-		err = -ENOMEM;
-		goto out_disable;
-	}
-	INIT_WORK(&hba->eh_work, ufshcd_err_handler);
 	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
 
 	sema_init(&hba->host_sem, 1);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 91b0b278469d..d0bca2b233ef 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -716,8 +716,6 @@ struct ufs_hba_monitor {
  * @is_powered: flag to check if HBA is powered
  * @shutting_down: flag to check if shutdown has been invoked
  * @host_sem: semaphore used to serialize concurrent contexts
- * @eh_wq: Workqueue that eh_work works on
- * @eh_work: Worker to handle UFS errors that require s/w attention
  * @eeh_work: Worker to handle exception events
  * @errors: HBA errors
  * @uic_error: UFS interconnect layer error status
@@ -820,8 +818,6 @@ struct ufs_hba {
 	struct semaphore host_sem;
 
 	/* Work Queues */
-	struct workqueue_struct *eh_wq;
-	struct work_struct eh_work;
 	struct work_struct eeh_work;
 
 	/* HBA Errors */

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Use the SCSI error handler instead of a custom error handling
> strategy.
> This change reduces the number of potential races in the UFS drivers
> since
> the UFS error handler and the SCSI error handler no longer run
> concurrently.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 

Reviewed-by: Bean Huo <beanhuo@micron.com>
Tested-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Adrian Hunter]

On 22/07/21 6:34 am, Bart Van Assche wrote:
> Use the SCSI error handler instead of a custom error handling strategy.
> This change reduces the number of potential races in the UFS drivers since
> the UFS error handler and the SCSI error handler no longer run concurrently.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Hi

There is a deadlock that seems to be related to this patch, because now
requests are blocked while the error handler waits on the host_sem.


Example:

ufshcd_err_handler() races with ufshcd_wl_suspend() for host_sem.
ufshcd_wl_suspend() wins the race but now PM requests deadlock:

because:
 scsi_queue_rq() -> scsi_host_queue_ready() -> scsi_host_in_recovery() is FALSE

because:
 scsi_schedule_eh() has done:
	    scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0)


Some questions for thought:

Won't any holder of host_sem deadlock if it tries to do SCSI requests
and the error handler is waiting on host_sem?

Won't runtime resume deadlock if it is initiated by the error handler?


Regards
Adrian

RE: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Avri Altman]

Hi, 
> On 22/07/21 6:34 am, Bart Van Assche wrote:
> > Use the SCSI error handler instead of a custom error handling strategy.
> > This change reduces the number of potential races in the UFS drivers
> > since the UFS error handler and the SCSI error handler no longer run
> concurrently.
> >
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Stanley Chu <stanley.chu@mediatek.com>
> > Cc: Can Guo <cang@codeaurora.org>
> > Cc: Asutosh Das <asutoshd@codeaurora.org>
> > Cc: Avri Altman <avri.altman@wdc.com>
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Shouldn't a transport template ops be used only for scsi_transport modules?

Thanks,
Avri

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Bart Van Assche]

On 8/29/21 00:17, Avri Altman wrote:
> Shouldn't a transport template ops be used only for scsi_transport modules?

If a transport template is used by multiple SCSI LLD drivers then the 
transport implementation should be implemented as a kernel module of its 
own. Since the transport template introduced by this patch is not used 
by any other kernel module I think it's fine to define this transport 
template in the UFS driver.

Bart.

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Adrian Hunter]

On 28/08/21 12:47 pm, Adrian Hunter wrote:
> On 22/07/21 6:34 am, Bart Van Assche wrote:
>> Use the SCSI error handler instead of a custom error handling strategy.
>> This change reduces the number of potential races in the UFS drivers since
>> the UFS error handler and the SCSI error handler no longer run concurrently.
>>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Stanley Chu <stanley.chu@mediatek.com>
>> Cc: Can Guo <cang@codeaurora.org>
>> Cc: Asutosh Das <asutoshd@codeaurora.org>
>> Cc: Avri Altman <avri.altman@wdc.com>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
> 
> Hi
> 
> There is a deadlock that seems to be related to this patch, because now
> requests are blocked while the error handler waits on the host_sem.
> 
> 
> Example:
> 
> ufshcd_err_handler() races with ufshcd_wl_suspend() for host_sem.
> ufshcd_wl_suspend() wins the race but now PM requests deadlock:
> 
> because:
>  scsi_queue_rq() -> scsi_host_queue_ready() -> scsi_host_in_recovery() is FALSE

That is scsi_host_queue_ready() is FALSE because scsi_host_in_recovery() is TRUE

> 
> because:
>  scsi_schedule_eh() has done:
> 	    scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
> 	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0)
> 
> 
> Some questions for thought:
> 
> Won't any holder of host_sem deadlock if it tries to do SCSI requests
> and the error handler is waiting on host_sem?
> 
> Won't runtime resume deadlock if it is initiated by the error handler?
> 
> 
> Regards
> Adrian
> 

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Bart Van Assche]

On 8/28/21 02:47, Adrian Hunter wrote:
> There is a deadlock that seems to be related to this patch, because now
> requests are blocked while the error handler waits on the host_sem.

Hi Adrian,

Some but not all of the issues mentioned below have been introduced by 
patch 16/18. Anyway, thank you for having shared your concerns.

> Example:
> 
> ufshcd_err_handler() races with ufshcd_wl_suspend() for host_sem.
> ufshcd_wl_suspend() wins the race but now PM requests deadlock:
> 
> because:
>   scsi_queue_rq() -> scsi_host_queue_ready() -> scsi_host_in_recovery() is FALSE
> 
> because:
>   scsi_schedule_eh() has done:
> 	    scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
> 	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0)
> 
> 
> Some questions for thought:
> 
> Won't any holder of host_sem deadlock if it tries to do SCSI requests
> and the error handler is waiting on host_sem?
> 
> Won't runtime resume deadlock if it is initiated by the error handler?

My understanding is that host_sem is used for the following purposes:
- To prevent that sysfs attributes are read or written after shutdown
   has started (hba->shutting_down).
- To serialize sysfs attribute access, clock scaling, error handling,
   the ufshcd_probe_hba() call from ufshcd_async_scan() and hibernation.

I propose to make the following changes:
- Instead of checking the value of hba->shutting_down from inside sysfs
   attribute callbacks, remove sysfs attributes before starting shutdown.
   That will remove the need to check hba->shutting_down from inside
   sysfs attribute callbacks.
- Leave out the host_sem down() and up() calls from ufshcd_wl_suspend()
   and ufshcd_wl_resume(). Serializing hibernation against e.g. sysfs
   attribute access is not the responsibility of a SCSI LLD - this is the
   responsibility of the power management core.
- Split host_sem. I don't see how else to address the potential deadlock
   between the error handler and runtime resume.

Do you agree with the above?

Thanks,

Bart.

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Adrian Hunter]

On 30/08/21 1:18 am, Bart Van Assche wrote:
> On 8/28/21 02:47, Adrian Hunter wrote:
>> There is a deadlock that seems to be related to this patch, because now
>> requests are blocked while the error handler waits on the host_sem.
> 
> Hi Adrian,
> 
> Some but not all of the issues mentioned below have been introduced by patch 16/18. Anyway, thank you for having shared your concerns.
> 
>> Example:
>>
>> ufshcd_err_handler() races with ufshcd_wl_suspend() for host_sem.
>> ufshcd_wl_suspend() wins the race but now PM requests deadlock:
>>
>> because:
>>   scsi_queue_rq() -> scsi_host_queue_ready() -> scsi_host_in_recovery() is FALSE
>>
>> because:
>>   scsi_schedule_eh() has done:
>>         scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
>>         scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0)
>>
>>
>> Some questions for thought:
>>
>> Won't any holder of host_sem deadlock if it tries to do SCSI requests
>> and the error handler is waiting on host_sem?
>>
>> Won't runtime resume deadlock if it is initiated by the error handler?
> 
> My understanding is that host_sem is used for the following purposes:
> - To prevent that sysfs attributes are read or written after shutdown
>   has started (hba->shutting_down).
> - To serialize sysfs attribute access, clock scaling, error handling,
>   the ufshcd_probe_hba() call from ufshcd_async_scan() and hibernation.
> 
> I propose to make the following changes:
> - Instead of checking the value of hba->shutting_down from inside sysfs
>   attribute callbacks, remove sysfs attributes before starting shutdown.
>   That will remove the need to check hba->shutting_down from inside
>   sysfs attribute callbacks.
> - Leave out the host_sem down() and up() calls from ufshcd_wl_suspend()
>   and ufshcd_wl_resume(). Serializing hibernation against e.g. sysfs
>   attribute access is not the responsibility of a SCSI LLD - this is the
>   responsibility of the power management core.
> - Split host_sem. I don't see how else to address the potential deadlock
>   between the error handler and runtime resume.
> 
> Do you agree with the above?

Looking some more:

sysfs and debugfs use direct access, so there is probably not a problem
there.

bsg also uses direct access but doesn't appear to have synchronization
so there is maybe a gap there.  That is an existing problem.

As an aside, the current synchronization for direct access doesn't make
complete sense because the lock (host_sem) remains held across retries
(e.g. ufshcd_query_descriptor_retry) preventing error handling between
retries.  That is an existing problem.

ufshcd_wl_suspend() and ufshcd_wl_shutdown() could wait for error handling
and then disable it somehow. ufshcd_wl_resume() would have to enable it.

That leaves runtime PM.  Since the error handler can block runtime resume,
it cannot wait for runtime resume, it must exit.  Another complication is
that the PM workqueue (pm_wq) gets frozen early during system suspend, so
requesting an asynchronous runtime resume won't necessarily make any
progress.

How does splitting the host_sem address the potential deadlock
between the error handler and runtime resume?

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Adrian Hunter]

On 31/08/21 10:24 am, Adrian Hunter wrote:
> On 30/08/21 1:18 am, Bart Van Assche wrote:
>> On 8/28/21 02:47, Adrian Hunter wrote:
>>> There is a deadlock that seems to be related to this patch, because now
>>> requests are blocked while the error handler waits on the host_sem.
>>
>> Hi Adrian,
>>
>> Some but not all of the issues mentioned below have been introduced by patch 16/18. Anyway, thank you for having shared your concerns.
>>
>>> Example:
>>>
>>> ufshcd_err_handler() races with ufshcd_wl_suspend() for host_sem.
>>> ufshcd_wl_suspend() wins the race but now PM requests deadlock:
>>>
>>> because:
>>>   scsi_queue_rq() -> scsi_host_queue_ready() -> scsi_host_in_recovery() is FALSE
>>>
>>> because:
>>>   scsi_schedule_eh() has done:
>>>         scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
>>>         scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0)
>>>
>>>
>>> Some questions for thought:
>>>
>>> Won't any holder of host_sem deadlock if it tries to do SCSI requests
>>> and the error handler is waiting on host_sem?
>>>
>>> Won't runtime resume deadlock if it is initiated by the error handler?
>>
>> My understanding is that host_sem is used for the following purposes:
>> - To prevent that sysfs attributes are read or written after shutdown
>>   has started (hba->shutting_down).
>> - To serialize sysfs attribute access, clock scaling, error handling,
>>   the ufshcd_probe_hba() call from ufshcd_async_scan() and hibernation.
>>
>> I propose to make the following changes:
>> - Instead of checking the value of hba->shutting_down from inside sysfs
>>   attribute callbacks, remove sysfs attributes before starting shutdown.
>>   That will remove the need to check hba->shutting_down from inside
>>   sysfs attribute callbacks.
>> - Leave out the host_sem down() and up() calls from ufshcd_wl_suspend()
>>   and ufshcd_wl_resume(). Serializing hibernation against e.g. sysfs
>>   attribute access is not the responsibility of a SCSI LLD - this is the
>>   responsibility of the power management core.
>> - Split host_sem. I don't see how else to address the potential deadlock
>>   between the error handler and runtime resume.
>>
>> Do you agree with the above?
> 
> Looking some more:
> 
> sysfs and debugfs use direct access, so there is probably not a problem
> there.

Except with runtime pm, but might be OK if ufshcd_rpm_get_sync() is moved
before down(&hba->host_sem).

> 
> bsg also uses direct access but doesn't appear to have synchronization
> so there is maybe a gap there.  That is an existing problem.
> 
> As an aside, the current synchronization for direct access doesn't make
> complete sense because the lock (host_sem) remains held across retries
> (e.g. ufshcd_query_descriptor_retry) preventing error handling between
> retries.  That is an existing problem.
> 
> ufshcd_wl_suspend() and ufshcd_wl_shutdown() could wait for error handling
> and then disable it somehow. ufshcd_wl_resume() would have to enable it.
> 
> That leaves runtime PM.  Since the error handler can block runtime resume,
> it cannot wait for runtime resume, it must exit.  Another complication is
> that the PM workqueue (pm_wq) gets frozen early during system suspend, so
> requesting an asynchronous runtime resume won't necessarily make any
> progress.
> 
> How does splitting the host_sem address the potential deadlock
> between the error handler and runtime resume?
> 

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Bart Van Assche]

On 8/31/21 12:24 AM, Adrian Hunter wrote:
> How does splitting the host_sem address the potential deadlock
> between the error handler and runtime resume?

Hmm ... how do runtime resume and the error handler deadlock? If 
shost->eh_noresume == 0 then scsi_error_handler() will call 
scsi_autopm_get_host() before invoking the eh_strategy_handler callback. 
The definition of scsi_autopm_get_host() is as follows:

int scsi_autopm_get_host(struct Scsi_Host *shost)
{
	int	err;

	err = pm_runtime_get_sync(&shost->shost_gendev);
	if (err < 0 && err !=-EACCES)
		pm_runtime_put_sync(&shost->shost_gendev);
	else
		err = 0;
	return err;
}

The power management operations used for shost_gendev instances are 
defined by scsi_bus_pm_ops (see also scsi_host_alloc()). The following 
function is the runtime resume function referenced by scsi_bus_pm_ops 
and skips shost_gendevs since these are not SCSI devices:

static int scsi_runtime_resume(struct device *dev)
{
	int err = 0;

	dev_dbg(dev, "scsi_runtime_resume\n");
	if (scsi_is_sdev_device(dev))
		err = sdev_runtime_resume(dev);

	/* Insert hooks here for targets, hosts, and transport classes*/

	return err;
}

In addition to the above function the runtime resume callback of the UFS 
platform device is also invoked. I think all these functions call 
ufshcd_runtime_resume(). As far as I can see ufshcd_runtime_resume() 
does not touch host_sem?

Bart.

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Adrian Hunter]

On 31/08/21 8:18 pm, Bart Van Assche wrote:
> On 8/31/21 12:24 AM, Adrian Hunter wrote:
>> How does splitting the host_sem address the potential deadlock
>> between the error handler and runtime resume?
> 
> Hmm ... how do runtime resume and the error handler deadlock? If shost->eh_noresume == 0 then scsi_error_handler() will call scsi_autopm_get_host() before invoking the eh_strategy_handler callback. The definition of scsi_autopm_get_host() is as follows:
> 
> int scsi_autopm_get_host(struct Scsi_Host *shost)
> {
>     int    err;
> 
>     err = pm_runtime_get_sync(&shost->shost_gendev);
>     if (err < 0 && err !=-EACCES)
>         pm_runtime_put_sync(&shost->shost_gendev);
>     else
>         err = 0;
>     return err;
> }

That resumes the host, but the problem is with resuming the UFS device.
The UFS device can be resumed by ufshcd_err_handling_prepare(),
notably before it calls ufshcd_scsi_block_requests()

> 
> The power management operations used for shost_gendev instances are defined by scsi_bus_pm_ops (see also scsi_host_alloc()). The following function is the runtime resume function referenced by scsi_bus_pm_ops and skips shost_gendevs since these are not SCSI devices:
> 
> static int scsi_runtime_resume(struct device *dev)
> {
>     int err = 0;
> 
>     dev_dbg(dev, "scsi_runtime_resume\n");
>     if (scsi_is_sdev_device(dev))
>         err = sdev_runtime_resume(dev);
> 
>     /* Insert hooks here for targets, hosts, and transport classes*/
> 
>     return err;
> }
> 
> In addition to the above function the runtime resume callback of the UFS platform device is also invoked. I think all these functions call ufshcd_runtime_resume(). As far as I can see ufshcd_runtime_resume() does not touch host_sem?

No it doesn't use host_sem.  The problem is with issuing requests to a blocked queue.
If the UFS device is in SLEEP state, runtime resume will try to do a
SCSI request to change to ACTIVE state.  That will block while the error
handler is running.  So if the error handler is waiting on runtime resume,
deadlock.

Here is an example:

First instrument debugfs stats file to trigger SCSI error handling:

From: Adrian Hunter <adrian.hunter@intel.com>
Date: Wed, 1 Sep 2021 09:16:34 +0300
Subject: [PATCH] HACK: scsi: ufs: Hack debugfs stats to do scsi_schedule_eh()

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufs-debugfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-debugfs.c b/drivers/scsi/ufs/ufs-debugfs.c
index 4e1ff209b9336..614ba42203a54 100644
--- a/drivers/scsi/ufs/ufs-debugfs.c
+++ b/drivers/scsi/ufs/ufs-debugfs.c
@@ -3,6 +3,8 @@
 
 #include <linux/debugfs.h>
 
+#include <scsi/scsi_transport.h>
+#include "../scsi_transport_api.h"
 #include "ufs-debugfs.h"
 #include "ufshcd.h"
 
@@ -40,6 +42,10 @@ static int ufs_debugfs_stats_show(struct seq_file *s, void *data)
 	PRT("Host Resets: %llu\n", HOST_RESET);
 	PRT("SCSI command aborts: %llu\n", ABORT);
 #undef PRT
+	seq_printf(s, "\n%s: Calling scsi_schedule_eh\n\n", __func__);
+	dev_info(hba->dev, "%s: Calling scsi_schedule_eh\n", __func__);
+	hba->force_reset = true;
+	scsi_schedule_eh(hba->host);
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(ufs_debugfs_stats);
-- 
2.25.1


Then set an rpm_lvl to SLEEP:

# echo 2 > /sys/bus/pci/drivers/ufshcd/0000\:00\:12.5/rpm_lvl
# grep -H . /sys/bus/pci/drivers/ufshcd/0000\:00\:12.5/*pm*
/sys/bus/pci/drivers/ufshcd/0000:00:12.5/rpm_lvl:2
/sys/bus/pci/drivers/ufshcd/0000:00:12.5/rpm_target_dev_state:SLEEP
/sys/bus/pci/drivers/ufshcd/0000:00:12.5/rpm_target_link_state:ACTIVE
/sys/bus/pci/drivers/ufshcd/0000:00:12.5/spm_lvl:5
/sys/bus/pci/drivers/ufshcd/0000:00:12.5/spm_target_dev_state:POWERDOWN
/sys/bus/pci/drivers/ufshcd/0000:00:12.5/spm_target_link_state:OFF


I have to do a little IO to make sure the new rpm_lvl
has been used to runtime suspend:

# dd if=/dev/sdb of=/dev/null bs=4096 count=1 &
# 1+0 records in
1+0 records out
4096 bytes (4.1 kB, 4.0 KiB) copied, 0.202648 s, 20.2 kB/


Then trigger the error handler while runtime suspended:

# cat /sys/kernel/debug/ufshcd/0000\:00\:12.5/stats
PHY Adapter Layer errors (except LINERESET): 0
Data Link Layer errors: 0
Network Layer errors: 0
Transport Layer errors: 0
Generic DME errors: 0
Auto-hibernate errors: 0
IS Fatal errors (CEFES, SBFES, HCFES, DFES): 0
DME Link Startup errors: 0
PM Resume errors: 0
PM Suspend errors : 0
Logical Unit Resets: 0
Host Resets: 1
SCSI command aborts: 0

ufs_debugfs_stats_show: Calling scsi_schedule_eh


And show blocked tasks:

# echo w > /proc/sysrq-trigger
# dmesg | tail -29
[  269.223247] sysrq: Show Blocked State
[  269.224452] task:scsi_eh_1       state:D stack:13936 pid:  258 ppid:     2 flags:0x00004000
[  269.225318] Call Trace:
[  269.226133]  __schedule+0x26c/0x6c0
[  269.227265]  schedule+0x3f/0xa0
[  269.228472]  schedule_timeout+0x209/0x290
[  269.228872]  ? blk_mq_sched_dispatch_requests+0x2b/0x50
[  269.229247]  io_schedule_timeout+0x14/0x40
[  269.229825] wait_for_completion_io+0x81/0xe0
[  269.230273]  blk_execute_rq+0x64/0xd0
[  269.230868]  __scsi_execute+0x109/0x260
[  269.231301] ufshcd_set_dev_pwr_mode+0xe2/0x1c0 [ufshcd_core]
[  269.231754]  __ufshcd_wl_resume+0x96/0x220 [ufshcd_core]
[  269.232146] ufshcd_wl_runtime_resume+0x28/0xd0 [ufshcd_core]
[  269.232756]  scsi_runtime_resume+0x76/0xb0
[  269.233499]  ? scsi_autopm_put_device+0x20/0x20
[  269.234171]  __rpm_callback+0x3b/0x110
[  269.235095]  ? scsi_autopm_put_device+0x20/0x20
[  269.235988]  rpm_callback+0x54/0x60
[  269.236607]  rpm_resume+0x503/0x700
[  269.237134]  ? __pm_runtime_idle+0x4c/0xe0
[  269.237822]  __pm_runtime_resume+0x45/0x70
[  269.238376] ufshcd_err_handler+0x112/0x9f0 [ufshcd_core]
[  269.238928]  ? __pm_runtime_resume+0x53/0x70
[  269.239392]  scsi_error_handler+0xa8/0x3a0
[  269.239832]  ? scsi_eh_get_sense+0x140/0x140
[  269.240266]  kthread+0x11f/0x140
[  269.240860]  ? set_kthread_struct+0x40/0x40
[  269.241275]  ret_from_fork+0x1f/0x30
# 

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Bart Van Assche]

On 9/1/21 12:42 AM, Adrian Hunter wrote:
> No it doesn't use host_sem.  The problem is with issuing requests to a blocked queue.
> If the UFS device is in SLEEP state, runtime resume will try to do a
> SCSI request to change to ACTIVE state.  That will block while the error
> handler is running.  So if the error handler is waiting on runtime resume,
> deadlock.

Please define "UFS device". Does this refer to the physical device or to 
a LUN?

I agree that suspending or resuming a LUN involves executing a SCSI 
command. See also __ufshcd_wl_suspend() and __ufshcd_wl_resume(). These 
functions are used to suspend or resume a LUN and not to suspend or 
resume the UFS device.

However, I don't see how the above scenario would lead to a deadlock? 
The UFS error handler (ufshcd_err_handler()) works at the link level and 
may resume the SCSI host and/or UFS device (hba->host and hba->dev). The 
UFS error handler must not try to resume any of the LUNs since that 
involves executing SCSI commands.

Bart.

Re: [PATCH v3 16/18] scsi: ufs: Synchronize SCSI and UFS error handling

[Adrian Hunter]

On 1/09/21 11:46 pm, Bart Van Assche wrote:
> On 9/1/21 12:42 AM, Adrian Hunter wrote:
>> No it doesn't use host_sem.  The problem is with issuing requests to a blocked queue.
>> If the UFS device is in SLEEP state, runtime resume will try to do a
>> SCSI request to change to ACTIVE state.  That will block while the error
>> handler is running.  So if the error handler is waiting on runtime resume,
>> deadlock.
> 
> Please define "UFS device". Does this refer to the physical device or to a LUN?

UFS Device WLUN aka UFS Device Well-known LUN aka LUN 49488. It is in the spec.

> 
> I agree that suspending or resuming a LUN involves executing a SCSI command. See also __ufshcd_wl_suspend() and __ufshcd_wl_resume(). These functions are used to suspend or resume a LUN and not to suspend or resume the UFS device.

__ufshcd_wl_suspend() and __ufshcd_wl_resume() are for the UFS Device WLUN (what the wl stands for).  All other LUNs are device link consumers of it.

> 
> However, I don't see how the above scenario would lead to a deadlock? The UFS error handler (ufshcd_err_handler()) works at the link level and may resume the SCSI host and/or UFS device (hba->host and hba->dev). The UFS error handler must not try to resume any of the LUNs since that involves executing SCSI commands.

A detailed example was provided.

[PATCH v3 17/18] scsi: ufs: Retry aborted SCSI commands instead of completing these successfully

[Bart Van Assche]

Neither SAM nor the UFS standard require that the UFS controller fills in
the completion status of commands that have been aborted (LUN RESET aborts
pending commands). Hence do not rely on the completion status provided by
the UFS controller for aborted commands but instead ask the SCSI core to
retry SCSI commands that have been aborted.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8d87fb214281..3cfbc467f7c0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5207,10 +5207,12 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 /**
  * __ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
- * @completed_reqs: requests to complete
+ * @completed_reqs: bitmask that indicates which requests to complete
+ * @retry_requests: whether to ask the SCSI core to retry completed requests
  */
 static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
-					unsigned long completed_reqs)
+					unsigned long completed_reqs,
+					bool retry_requests)
 {
 	struct ufshcd_lrb *lrbp;
 	struct scsi_cmnd *cmd;
@@ -5226,7 +5228,8 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 			if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
 				ufshcd_update_monitor(hba, lrbp);
 			ufshcd_add_command_trace(hba, index, UFS_CMD_COMP);
-			result = ufshcd_transfer_rsp_status(hba, lrbp);
+			result = retry_requests ? DID_BUS_BUSY << 16 :
+				ufshcd_transfer_rsp_status(hba, lrbp);
 			scsi_dma_unmap(cmd);
 			cmd->result = result;
 			/* Mark completed command as NULL in LRB */
@@ -5252,12 +5255,14 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 /**
  * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
+ * @retry_requests: whether or not to ask to retry requests
  *
  * Returns
  *  IRQ_HANDLED - If interrupt is valid
  *  IRQ_NONE    - If invalid interrupt
  */
-static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
+static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba,
+					     bool retry_requests)
 {
 	unsigned long completed_reqs, flags;
 	u32 tr_doorbell;
@@ -5283,7 +5288,8 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 
 	if (completed_reqs) {
-		__ufshcd_transfer_req_compl(hba, completed_reqs);
+		__ufshcd_transfer_req_compl(hba, completed_reqs,
+					    retry_requests);
 		return IRQ_HANDLED;
 	} else {
 		return IRQ_NONE;
@@ -5762,7 +5768,13 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 /* Complete requests that have door-bell cleared */
 static void ufshcd_complete_requests(struct ufs_hba *hba)
 {
-	ufshcd_transfer_req_compl(hba);
+	ufshcd_transfer_req_compl(hba, /*retry_requests=*/false);
+	ufshcd_tmc_handler(hba);
+}
+
+static void ufshcd_retry_aborted_requests(struct ufs_hba *hba)
+{
+	ufshcd_transfer_req_compl(hba, /*retry_requests=*/true);
 	ufshcd_tmc_handler(hba);
 }
 
@@ -6082,8 +6094,7 @@ static void ufshcd_err_handler(struct Scsi_Host *host)
 	}
 
 lock_skip_pending_xfer_clear:
-	/* Complete the requests that are cleared by s/w */
-	ufshcd_complete_requests(hba);
+	ufshcd_retry_aborted_requests(hba);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->silence_err_logs = false;
@@ -6384,7 +6395,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 		retval |= ufshcd_tmc_handler(hba);
 
 	if (intr_status & UTP_TRANSFER_REQ_COMPL)
-		retval |= ufshcd_transfer_req_compl(hba);
+		retval |= ufshcd_transfer_req_compl(hba, /*retry_requests=*/false);
 
 	return retval;
 }
@@ -6803,7 +6814,7 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
 			err = ufshcd_clear_cmd(hba, pos);
 			if (err)
 				break;
-			__ufshcd_transfer_req_compl(hba, pos);
+			__ufshcd_transfer_req_compl(hba, pos, /*retry_requests=*/true);
 		}
 	}
 
@@ -6965,7 +6976,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		dev_err(hba->dev,
 		"%s: cmd was completed, but without a notifying intr, tag = %d",
 		__func__, tag);
-		__ufshcd_transfer_req_compl(hba, 1UL << tag);
+		__ufshcd_transfer_req_compl(hba, 1UL << tag, /*retry_requests=*/false);
 		goto release;
 	}
 
@@ -7032,7 +7043,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	 */
 	ufshcd_hba_stop(hba);
 	hba->silence_err_logs = true;
-	ufshcd_complete_requests(hba);
+	ufshcd_retry_aborted_requests(hba);
 	hba->silence_err_logs = false;
 
 	/* scale up clocks to max frequency before full reinitialization */

Re: [PATCH v3 17/18] scsi: ufs: Retry aborted SCSI commands instead of completing these successfully

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Neither SAM nor the UFS standard require that the UFS controller
> fills in
> 
> the completion status of commands that have been aborted (LUN RESET
> aborts
> 
> pending commands). Hence do not rely on the completion status
> provided by
> 
> the UFS controller for aborted commands but instead ask the SCSI core
> to
> 
> retry SCSI commands that have been aborted.
> 
> 
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> 
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> 
> Cc: Can Guo <cang@codeaurora.org>
> 
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> 
> Cc: Avri Altman <avri.altman@wdc.com>
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 
> ---

Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v3 18/18] scsi: ufs: Add fault injection support

[Bart Van Assche]

Make it easier to test the UFS error handler and abort handler.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/Kconfig               |  7 +++
 drivers/scsi/ufs/Makefile              |  1 +
 drivers/scsi/ufs/ufs-fault-injection.c | 70 ++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-fault-injection.h | 24 +++++++++
 drivers/scsi/ufs/ufshcd.c              |  8 +++
 5 files changed, 110 insertions(+)
 create mode 100644 drivers/scsi/ufs/ufs-fault-injection.c
 create mode 100644 drivers/scsi/ufs/ufs-fault-injection.h

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index 2d137953e7b4..4272d7365595 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -183,3 +183,10 @@ config SCSI_UFS_CRYPTO
 	  Enabling this makes it possible for the kernel to use the crypto
 	  capabilities of the UFS device (if present) to perform crypto
 	  operations on data being transferred to/from the device.
+
+config SCSI_UFS_FAULT_INJECTION
+       bool "UFS Fault Injection Support"
+       depends on SCSI_UFSHCD && FAULT_INJECTION
+       help
+         Enable fault injection support in the UFS driver. This makes it easier
+	 to test the UFS error handler and abort handler.
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 06f3a3fe4a44..006d50236079 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -8,6 +8,7 @@ ufshcd-core-y				+= ufshcd.o ufs-sysfs.o
 ufshcd-core-$(CONFIG_DEBUG_FS)		+= ufs-debugfs.o
 ufshcd-core-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
 ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO)	+= ufshcd-crypto.o
+ufshcd-core-$(CONFIG_SCSI_UFS_FAULT_INJECTION) += ufs-fault-injection.o
 
 obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o tc-dwc-g210.o
 obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-dwc-g210.o
diff --git a/drivers/scsi/ufs/ufs-fault-injection.c b/drivers/scsi/ufs/ufs-fault-injection.c
new file mode 100644
index 000000000000..7ac7c4e7ff83
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-fault-injection.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/kconfig.h>
+#include <linux/types.h>
+#include <linux/fault-inject.h>
+#include <linux/module.h>
+#include "ufs-fault-injection.h"
+
+static int ufs_fault_get(char *buffer, const struct kernel_param *kp);
+static int ufs_fault_set(const char *val, const struct kernel_param *kp);
+
+static const struct kernel_param_ops ufs_fault_ops = {
+	.get = ufs_fault_get,
+	.set = ufs_fault_set,
+};
+
+enum { FAULT_INJ_STR_SIZE = 80 };
+
+/*
+ * For more details about fault injection, please refer to
+ * Documentation/fault-injection/fault-injection.rst.
+ */
+static char g_trigger_eh_str[FAULT_INJ_STR_SIZE];
+module_param_cb(trigger_eh, &ufs_fault_ops, g_trigger_eh_str, 0644);
+MODULE_PARM_DESC(trigger_eh,
+	"Fault injection. trigger_eh=<interval>,<probability>,<space>,<times>");
+static DECLARE_FAULT_ATTR(ufs_trigger_eh_attr);
+
+static char g_timeout_str[FAULT_INJ_STR_SIZE];
+module_param_cb(timeout, &ufs_fault_ops, g_timeout_str, 0644);
+MODULE_PARM_DESC(timeout,
+	"Fault injection. timeout=<interval>,<probability>,<space>,<times>");
+static DECLARE_FAULT_ATTR(ufs_timeout_attr);
+
+static int ufs_fault_get(char *buffer, const struct kernel_param *kp)
+{
+	const char *fault_str = kp->arg;
+
+	return sysfs_emit(buffer, "%s\n", fault_str);
+}
+
+static int ufs_fault_set(const char *val, const struct kernel_param *kp)
+{
+	struct fault_attr *attr = NULL;
+
+	if (kp->arg == g_trigger_eh_str)
+		attr = &ufs_trigger_eh_attr;
+	else if (kp->arg == g_timeout_str)
+		attr = &ufs_timeout_attr;
+
+	if (WARN_ON_ONCE(!attr))
+		return -EINVAL;
+
+	if (!setup_fault_attr(attr, (char *)val))
+		return -EINVAL;
+
+	strlcpy(kp->arg, val, FAULT_INJ_STR_SIZE);
+
+	return 0;
+}
+
+bool ufs_trigger_eh(void)
+{
+	return should_fail(&ufs_trigger_eh_attr, 1);
+}
+
+bool ufs_fail_completion(void)
+{
+	return should_fail(&ufs_timeout_attr, 1);
+}
diff --git a/drivers/scsi/ufs/ufs-fault-injection.h b/drivers/scsi/ufs/ufs-fault-injection.h
new file mode 100644
index 000000000000..6d0cd8e10c87
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-fault-injection.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _UFS_FAULT_INJECTION_H
+#define _UFS_FAULT_INJECTION_H
+
+#include <linux/kconfig.h>
+#include <linux/types.h>
+
+#ifdef CONFIG_SCSI_UFS_FAULT_INJECTION
+bool ufs_trigger_eh(void);
+bool ufs_fail_completion(void);
+#else
+static inline bool ufs_trigger_eh(void)
+{
+	return false;
+}
+
+static inline bool ufs_fail_completion(void)
+{
+	return false;
+}
+#endif
+
+#endif /* _UFS_FAULT_INJECTION_H */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3cfbc467f7c0..e6ceab4563ba 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -24,6 +24,7 @@
 #include "unipro.h"
 #include "ufs-sysfs.h"
 #include "ufs-debugfs.h"
+#include "ufs-fault-injection.h"
 #include "ufs_bsg.h"
 #include "ufshcd-crypto.h"
 #include <asm/unaligned.h>
@@ -2750,6 +2751,10 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	ufshcd_send_command(hba, tag);
 out:
 	up_read(&hba->clk_scaling_lock);
+
+	if (ufs_trigger_eh())
+		scsi_schedule_eh(hba->host);
+
 	return err;
 }
 
@@ -5278,6 +5283,9 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba,
 	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
 		ufshcd_reset_intr_aggr(hba);
 
+	if (ufs_fail_completion())
+		return IRQ_HANDLED;
+
 	spin_lock_irqsave(&hba->outstanding_lock, flags);
 	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	completed_reqs = ~tr_doorbell & hba->outstanding_reqs;

Re: [PATCH v3 18/18] scsi: ufs: Add fault injection support

[Bean Huo]

On Wed, 2021-07-21 at 20:34 -0700, Bart Van Assche wrote:
> Make it easier to test the UFS error handler and abort handler.
> 
> 
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Acked-by: Bean Huo <beanhuo@micron.com>

RE: [PATCH v3 06/18] scsi: ufs: Remove ufshcd_valid_tag()

[Daejun Park]

Hi Bart,

> @@ -6979,24 +6966,15 @@ static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
>   */
>  static int ufshcd_abort(struct scsi_cmnd *cmd)
>  {
> -        struct Scsi_Host *host;
> -        struct ufs_hba *hba;
> +        struct Scsi_Host *host = cmd->device->host;
> +        struct ufs_hba *hba = shost_priv(host);
> +        unsigned int tag = cmd->request->tag;
> +        struct ufshcd_lrb *lrbp = &hba->lrb[tag];

If tag < 0, lrbp will be assigned incorrect pointer.

>          unsigned long flags;
> -        unsigned int tag;
>          int err = 0;
> -        struct ufshcd_lrb *lrbp;
>          u32 reg;
>  
> -        host = cmd->device->host;
> -        hba = shost_priv(host);
> -        tag = cmd->request->tag;
> -        lrbp = &hba->lrb[tag];
> -        if (!ufshcd_valid_tag(hba, tag)) {
> -                dev_err(hba->dev,
> -                        "%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
> -                        __func__, tag, cmd, cmd->request);
> -                BUG();
> -        }
> +        WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
>  
>          ufshcd_hold(hba, false);
>          reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);

Thanks,
Daejun

Re: [PATCH v3 06/18] scsi: ufs: Remove ufshcd_valid_tag()

[Bart Van Assche]

On 7/27/21 11:48 PM, Daejun Park wrote:
>> @@ -6979,24 +6966,15 @@ static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
>>    */
>>   static int ufshcd_abort(struct scsi_cmnd *cmd)
>>   {
>> -        struct Scsi_Host *host;
>> -        struct ufs_hba *hba;
>> +        struct Scsi_Host *host = cmd->device->host;
>> +        struct ufs_hba *hba = shost_priv(host);
>> +        unsigned int tag = cmd->request->tag;
>> +        struct ufshcd_lrb *lrbp = &hba->lrb[tag];
> 
> If tag < 0, lrbp will be assigned incorrect pointer.

That shouldn't hurt since lrbp is only used after it has been verified 
that tag >= 0.

Thanks,

Bart.

RE:(2) [PATCH v3 06/18] scsi: ufs: Remove ufshcd_valid_tag()

[Daejun Park]

Hi Bart,

>On 7/27/21 11:48 PM, Daejun Park wrote:
>>> @@ -6979,24 +6966,15 @@ static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
>>>    */
>>>   static int ufshcd_abort(struct scsi_cmnd *cmd)
>>>   {
>>> -        struct Scsi_Host *host;
>>> -        struct ufs_hba *hba;
>>> +        struct Scsi_Host *host = cmd->device->host;
>>> +        struct ufs_hba *hba = shost_priv(host);
>>> +        unsigned int tag = cmd->request->tag;
>>> +        struct ufshcd_lrb *lrbp = &hba->lrb[tag];
>> 
>> If tag < 0, lrbp will be assigned incorrect pointer.
> 
>That shouldn't hurt since lrbp is only used after it has been verified 
>that tag >= 0.
OK, I got it.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>
> 
>Thanks,
> 
>Bart.
> 
> 
> 

RE: [PATCH v3 08/18] scsi: ufs: Improve static type checking for the host controller state

[Keoseong Park]

Hi Bart,

>Assign a name to the enumeration type for UFS host controller states and
>remove the default clause from switch statements on this enumeration type
>to make the compiler warn about unhandled enumeration labels.
>
>Reviewed-by: Avri Altman <avri.altman@wdc.com>
>Cc: Can Guo <cang@codeaurora.org>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks good to me.

Reviewed-by: Keoseong Park <keosung.park@samsung.com>

RE: [PATCH v3 01/18] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()

[Daejun Park]

Hi Bart,

>If param_offset > buff_len then the memcpy() statement in
>ufshcd_read_desc_param() corrupts memory since it copies
>256 + buff_len - param_offset bytes into a buffer with size buff_len.
>Since param_offset < 256 this results in writing past the bound of the
>output buffer.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 02/18] scsi: ufs: Reduce power management code duplication

[Daejun Park]

Hi Bart,

>Move the dev_get_drvdata() calls into the ufshcd_{system,runtime}_*()
>functions. Remove ufshcd_runtime_idle() since it is empty. This patch
>does not change any functionality.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 03/18] scsi: ufs: Only include power management code if necessary

[Daejun Park]

Hi Bart,
 
>This patch slightly reduces the UFS driver size if built with power
>management support disabled.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

Re: [PATCH v3 03/18] scsi: ufs: Only include power management code if necessary

[Stanley Chu]

Hi Bart,

>
> >This patch slightly reduces the UFS driver size if built with power
> >management support disabled.
>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

RE: [PATCH v3 04/18] scsi: ufs: Rename the second ufshcd_probe_hba() argument

[Daejun Park]

Hi Bart,

>Rename the second argument of ufshcd_probe_hba() such that the name of
>that argument reflects its purpose instead of how the function is called.
>See also commit 1b9e21412f72 ("scsi: ufs: Split ufshcd_probe_hba() based
>on its called flow").

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 05/18] scsi: ufs: Use DECLARE_COMPLETION_ONSTACK() where appropriate

[Daejun Park]

Hi Bart,

>From Documentation/scheduler/completion.rst: "When a completion is declared
>as a local variable within a function, then the initialization should
>always use DECLARE_COMPLETION_ONSTACK() explicitly, not just to make
>lockdep happy, but also to make it clear that limited scope had been
>considered and is intentional."

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 07/18] scsi: ufs: Verify UIC locking requirements at runtime

[Daejun Park]

Hi Bart,

>Instead of documenting the locking requirements of the UIC code as comments,
>use lockdep_assert_held() such that lockdep verifies the lockdep
>requirements at runtime if lockdep is enabled.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 08/18] scsi: ufs: Improve static type checking for the host controller state

[Daejun Park]

Hi Bart,

>Assign a name to the enumeration type for UFS host controller states and
>remove the default clause from switch statements on this enumeration type
>to make the compiler warn about unhandled enumeration labels.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 09/18] scsi: ufs: Remove several wmb() calls

[Daejun Park]

Hi Bart,

>From arch/arm/include/asm/io.h
> 
>  #define __iowmb() wmb()
>  [ ... ]
>  #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
> 
>From Documentation/memory-barriers.txt: "Note that, when using writel(), a
>prior wmb() is not needed to guarantee that the cache coherent memory
>writes have completed before writing to the MMIO region."
> 
>In other words, calling wmb() before writel() is not necessary. Hence
>remove the wmb() calls that precede a writel() call. Remove the wmb()
>calls that precede a ufshcd_send_command() call since the latter function
>uses writel(). Remove the wmb() call from ufshcd_wait_for_dev_cmd()
>since the following chain of events guarantees that the CPU will see
>up-to-date LRB values:
>* UFS controller writes to host memory.
>* UFS controller posts completion interrupt after the memory writes from
>  the previous step are visible to the CPU.
>* complete(hba->dev_cmd.complete) is called from the UFS interrupt handler.
>* The wait_for_completion(hba->dev_cmd.complete) call in
>  ufshcd_wait_for_dev_cmd() returns.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 12/18] scsi: ufs: Optimize serialization of setup_xfer_req() calls

[Daejun Park]

Hi Bart,

>Reduce the number of times the host lock is taken in the hot path.
>Additionally, inline ufshcd_vops_setup_xfer_req() because that function is
>too short to keep it.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

RE: [PATCH v3 13/18] scsi: ufs: Optimize SCSI command processing

[Daejun Park]

Hi Bart,

>Use a spinlock to protect hba->outstanding_reqs instead of using atomic
>operations to update this member variable.
> 
>This patch is a performance improvement because it reduces the number of
>atomic operations in the hot path (test_and_clear_bit()) and because it
>reduces the lock contention on the SCSI host lock. On my test setup this
>patch improves IOPS by about 1%.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

Re: [PATCH v3 00/18] UFS patches for kernel v5.15

[Martin K. Petersen]

Bart,

> Please consider the patches in this series for kernel v5.15.

Applied to 5.15/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 00/18] UFS patches for kernel v5.15

[Martin K. Petersen]

On Wed, 21 Jul 2021 20:34:21 -0700, Bart Van Assche wrote:

> Please consider the patches in this series for kernel v5.15.
> 
> Thank you,
> 
> Bart.
> 
> Changes compared to v2:
> - Included a stack corruption fix.
> - Dropped patch "Remove a local variable" and added patches "Revert "Utilize
>   Transfer Request List Completion Notification Register"" and "Optimize
>   serialization of setup_xfer_req() calls".
> - Added patch "Optimize serialization of setup_xfer_req() calls".
> 
> [...]

Applied to 5.15/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering