[PATCH] fault-inject: Declare the second argument of setup_fault_attr() const

[PATCH] fault-inject: Declare the second argument of setup_fault_attr() const

[Bart Van Assche]

This patch makes it possible to pass a const char * argument to
setup_fault_attr() without having to cast away constness.

Cc: Akinobu Mita <akinobu.mita@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/fault-inject.h | 2 +-
 lib/fault-inject.c           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h
index e525f6957c49..afc649f0102b 100644
--- a/include/linux/fault-inject.h
+++ b/include/linux/fault-inject.h
@@ -42,7 +42,7 @@ struct fault_attr {
 	}
 
 #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER
-int setup_fault_attr(struct fault_attr *attr, char *str);
+int setup_fault_attr(struct fault_attr *attr, const char *str);
 bool should_fail(struct fault_attr *attr, ssize_t size);
 
 #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index ce12621b4275..45520151b32d 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -15,7 +15,7 @@
  * setup_fault_attr() is a helper function for various __setup handlers, so it
  * returns 0 on error, because that is what __setup handlers do.
  */
-int setup_fault_attr(struct fault_attr *attr, char *str)
+int setup_fault_attr(struct fault_attr *attr, const char *str)
 {
 	unsigned long probability;
 	unsigned long interval;

[PATCH v2 00/19] UFS patches for kernel v5.15

[Bart Van Assche]

Hi Martin,

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

Thanks,

Bart.

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 (19):
  scsi: Fix the documentation of the scsi_execute() time parameter
  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: Rename __ufshcd_transfer_req_compl()
  scsi: ufs: Remove a local variable
  scsi: ufs: Fix a race in the completion path
  scsi: ufs: Use the doorbell register instead of the UTRLCNR register
  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/scsi_lib.c                |   2 +-
 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 |  67 ++++
 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              | 485 +++++++++++--------------
 drivers/scsi/ufs/ufshcd.h              |  48 ++-
 17 files changed, 370 insertions(+), 451 deletions(-)
 create mode 100644 drivers/scsi/ufs/ufs-fault-injection.c
 create mode 100644 drivers/scsi/ufs/ufs-fault-injection.h

[PATCH v2 01/19] scsi: Fix the documentation of the scsi_execute() time parameter

[Bart Van Assche]

The unit of the scsi_execute() timeout parameter is 1/HZ seconds instead of
one second, just like the timeouts used in the block layer. Fix the
documentation header above the definition of the scsi_execute() macro.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: John Garry <john.garry@huawei.com>
Fixes: "[SCSI] use scatter lists for all block pc requests and simplify hw handlers" # v2.6.16.28
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7184f93dfe15..4b56e06faa5e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -194,7 +194,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
  * @bufflen:	len of buffer
  * @sense:	optional sense buffer
  * @sshdr:	optional decoded sense header
- * @timeout:	request timeout in seconds
+ * @timeout:	request timeout in HZ
  * @retries:	number of times to retry request
  * @flags:	flags for ->cmd_flags
  * @rq_flags:	flags for ->rq_flags

Re: [PATCH v2 01/19] scsi: Fix the documentation of the scsi_execute() time parameter

[Hannes Reinecke]

On 7/9/21 10:26 PM, Bart Van Assche wrote:
> The unit of the scsi_execute() timeout parameter is 1/HZ seconds instead of
> one second, just like the timeouts used in the block layer. Fix the
> documentation header above the definition of the scsi_execute() macro.
> 
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: John Garry <john.garry@huawei.com>
> Fixes: "[SCSI] use scatter lists for all block pc requests and simplify hw handlers" # v2.6.16.28
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_lib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7184f93dfe15..4b56e06faa5e 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -194,7 +194,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
>    * @bufflen:	len of buffer
>    * @sense:	optional sense buffer
>    * @sshdr:	optional decoded sense header
> - * @timeout:	request timeout in seconds
> + * @timeout:	request timeout in HZ
>    * @retries:	number of times to retry request
>    * @flags:	flags for ->cmd_flags
>    * @rq_flags:	flags for ->rq_flags
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v2 01/19] scsi: Fix the documentation of the scsi_execute() time parameter

[Martin K. Petersen]

Bart,

> The unit of the scsi_execute() timeout parameter is 1/HZ seconds
> instead of one second, just like the timeouts used in the block
> layer. Fix the documentation header above the definition of the
> scsi_execute() macro.

Applied to 5.14/scsi-fixes, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v2 02/19] 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>
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 526b911655fe..3b4117ddb3c8 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -569,11 +569,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 32ae67e40086..fe6e678faa2c 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -1123,11 +1123,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 5b31de83a63a..a34aa6d486c7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9204,15 +9204,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();
 
@@ -9229,16 +9231,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;
@@ -9255,15 +9260,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();
 
@@ -9278,7 +9284,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:
@@ -9286,8 +9292,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();
 
@@ -9300,12 +9307,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 c98d540ac044..dc75426c609f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1009,11 +1009,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 v2 02/19] scsi: ufs: Reduce power management code duplication

[Bean Huo]

On Fri, 2021-07-09 at 13:26 -0700, Bart Van Assche wrote:
> 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>
> 
> 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 v2 03/19] 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>
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 a34aa6d486c7..37302a8b3937 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8736,6 +8736,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;
@@ -8763,6 +8764,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)
 {
@@ -9165,6 +9167,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
@@ -9202,7 +9205,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.
@@ -9258,7 +9263,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.
@@ -9306,6 +9313,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 dc75426c609f..79f6c261dfff 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1009,10 +1009,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,

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

[Bean Huo]

On Fri, 2021-07-09 at 13:26 -0700, Bart Van Assche wrote:
> This patch slightly reduces the UFS driver size if built with power
> 
> management support disabled.
> 
> 
> 
> 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 v2 04/19] 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 37302a8b3937..86ca9e1ce5aa 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7964,13 +7964,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;
@@ -8002,7 +8002,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;

[PATCH v2 05/19] 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>
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 86ca9e1ce5aa..2148e123e9db 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2949,11 +2949,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);
 
@@ -2975,7 +2975,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);
@@ -3980,14 +3979,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);
@@ -6660,11 +6658,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);
@@ -6682,7 +6680,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;

Re: [PATCH v2 05/19] scsi: ufs: Use DECLARE_COMPLETION_ONSTACK() where appropriate

[Bean Huo]

On Fri, 2021-07-09 at 13:26 -0700, Bart Van Assche wrote:
> 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>
> 
> 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 v2 06/19] 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.

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 2148e123e9db..fa52117fdb3e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -253,11 +253,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) {
@@ -2701,20 +2696,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;
@@ -2968,7 +2955,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);
 
 	if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
 		err = -EBUSY;
@@ -6673,7 +6660,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;
@@ -6975,24 +6962,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);

Re: [PATCH v2 06/19] scsi: ufs: Remove ufshcd_valid_tag()

[Bean Huo]

On Fri, 2021-07-09 at 13:26 -0700, Bart Van Assche wrote:
> 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.
> 
> 
> 
> CC: Avri Altman <avri.altman@wdc.com>
> 
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Bart,
you need to rebase this patch.

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

[PATCH v2 07/19] 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>
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 fa52117fdb3e..bf47ef41326e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2242,15 +2242,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;
@@ -2268,11 +2268,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
@@ -2281,6 +2280,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;
@@ -2310,14 +2311,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 79f6c261dfff..0dfb5e97ec0d 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

Re: [PATCH v2 07/19] scsi: ufs: Verify UIC locking requirements at runtime

[Bean Huo]

On Fri, 2021-07-09 at 13:26 -0700, Bart Van Assche wrote:
> 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>
> 
> 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 v2 08/19] 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 bf47ef41326e..1585eea27200 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -128,15 +128,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),
@@ -2737,12 +2728,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 0dfb5e97ec0d..f8766e8f3cac 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 v2 09/19] 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 1585eea27200..4bed4791720a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2770,8 +2770,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:
@@ -2881,8 +2879,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)) {
@@ -2958,8 +2954,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);
@@ -6517,9 +6511,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();
@@ -6691,8 +6682,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 v2 10/19] 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 4bed4791720a..81a27104308d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -743,16 +743,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
@@ -2900,7 +2890,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;

[PATCH v2 11/19] scsi: ufs: Rename __ufshcd_transfer_req_compl()

[Bart Van Assche]

Before patch "scsi: ufs: Optimize host lock on transfer requests send/compl
paths", the host lock was held around __ufshcd_transfer_req_compl() and the
double underscore prefix indicated this. Since the SCSI host lock is no
longer held around __ufshcd_transfer_req_compl() calls, remove the double
underscore prefix.

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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 81a27104308d..0cb84a744dad 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5183,12 +5183,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
+ * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
  * @completed_reqs: requests to complete
  */
-static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
-					unsigned long completed_reqs)
+static void ufshcd_transfer_req_compl(struct ufs_hba *hba,
+				      unsigned long completed_reqs)
 {
 	struct ufshcd_lrb *lrbp;
 	struct scsi_cmnd *cmd;
@@ -5273,7 +5273,7 @@ static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
 	}
 
 	if (completed_reqs) {
-		__ufshcd_transfer_req_compl(hba, completed_reqs);
+		ufshcd_transfer_req_compl(hba, completed_reqs);
 		return IRQ_HANDLED;
 	} else {
 		return IRQ_NONE;
@@ -6812,7 +6812,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);
 		}
 	}
 
@@ -6987,7 +6987,7 @@ 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));
+		ufshcd_transfer_req_compl(hba, 1UL << tag);
 		set_bit(tag, &hba->outstanding_reqs);
 		spin_lock_irqsave(host->host_lock, flags);
 		hba->force_reset = true;
@@ -7004,7 +7004,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 
 	if (!err) {
 cleanup:
-		__ufshcd_transfer_req_compl(hba, (1UL << tag));
+		ufshcd_transfer_req_compl(hba, 1UL << tag);
 out:
 		err = SUCCESS;
 	} else {

[PATCH v2 12/19] scsi: ufs: Remove a local variable

[Bart Van Assche]

Remove the local variable 'utrlcnr'. This patch does not change any
functionality.

Cc: 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 | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0cb84a744dad..83a32b71240e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5240,7 +5240,7 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba,
  */
 static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
 {
-	unsigned long completed_reqs = 0;
+	unsigned long completed_reqs;
 
 	/* Resetting interrupt aggregation counters first and reading the
 	 * DOOR_BELL afterward allows us to handle all the completed requests.
@@ -5254,14 +5254,11 @@ static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
 		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,
+		completed_reqs = ufshcd_readl(hba,
+					      REG_UTP_TRANSFER_REQ_LIST_COMPL);
+		if (completed_reqs)
+			ufshcd_writel(hba, completed_reqs,
 				      REG_UTP_TRANSFER_REQ_LIST_COMPL);
-			completed_reqs = utrlcnr;
-		}
 	} else {
 		unsigned long flags;
 		u32 tr_doorbell;

[PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Bart Van Assche]

The following unlikely races can be triggered by the completion path
(ufshcd_trc_handler()):
- After the UTRLCNR register has been read from interrupt context and
  before it is cleared, the UFS error handler reads the UTRLCNR register.
  Hold the SCSI host lock until the UTRLCNR register has been cleared to
  prevent that this register is accessed from another CPU before it has
  been cleared.
- After the doorbell register has been read and before outstanding_reqs
  is cleared, the error handler reads the doorbell register. This can also
  result in double completions. Fix this by clearing outstanding_reqs
  before calling ufshcd_transfer_req_compl().

Due to this change ufshcd_trc_handler() no longer updates outstanding_reqs
atomically. Hence protect all other outstanding_reqs changes with the SCSI
host lock.

This patch is a performance improvement because it reduces the number of
atomic operations in the hot path (test_and_clear_bit()).

See also commit a45f937110fa ("scsi: ufs: Optimize host lock on transfer
requests send/compl paths").

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 | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 83a32b71240e..996b95ab74aa 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2088,6 +2088,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);
@@ -2096,19 +2097,12 @@ 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);
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	/* Make sure that doorbell is committed immediately */
 	wmb();
 }
@@ -2890,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->host->host_lock, flags);
+		__clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
 	}
 
 	return err;
@@ -5197,8 +5193,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;
@@ -5241,6 +5235,7 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba,
 static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
 {
 	unsigned long completed_reqs;
+	unsigned long flags;
 
 	/* Resetting interrupt aggregation counters first and reading the
 	 * DOOR_BELL afterward allows us to handle all the completed requests.
@@ -5253,6 +5248,7 @@ 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);
 
+	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (use_utrlcnr) {
 		completed_reqs = ufshcd_readl(hba,
 					      REG_UTP_TRANSFER_REQ_LIST_COMPL);
@@ -5260,14 +5256,16 @@ static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
 			ufshcd_writel(hba, completed_reqs,
 				      REG_UTP_TRANSFER_REQ_LIST_COMPL);
 	} 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);
+		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->host->host_lock, flags);
 
 	if (completed_reqs) {
 		ufshcd_transfer_req_compl(hba, completed_reqs);

RE: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Avri Altman]

> 
> The following unlikely races can be triggered by the completion path
> (ufshcd_trc_handler()):
> - After the UTRLCNR register has been read from interrupt context and
>   before it is cleared, the UFS error handler reads the UTRLCNR register.
>   Hold the SCSI host lock until the UTRLCNR register has been cleared to
>   prevent that this register is accessed from another CPU before it has
>   been cleared.
> - After the doorbell register has been read and before outstanding_reqs
>   is cleared, the error handler reads the doorbell register. This can also
>   result in double completions. Fix this by clearing outstanding_reqs
>   before calling ufshcd_transfer_req_compl().
> 
> Due to this change ufshcd_trc_handler() no longer updates
> outstanding_reqs
> atomically. Hence protect all other outstanding_reqs changes with the SCSI
> host lock.
But isn't the whole point of REG_UTP_TRANSFER_REQ_LIST_COMPL is to eliminate the host lock
As a source of contention?

> 
> This patch is a performance improvement because it reduces the number of
> atomic operations in the hot path (test_and_clear_bit()).
Both Can & Stanley reported a performance improvement of RR with "Optimize host lock..".
Can those short numerical studies can be repeated with this patch?

Thanks,
Avri

> 
> See also commit a45f937110fa ("scsi: ufs: Optimize host lock on transfer
> requests send/compl paths").
> 
> 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 | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 83a32b71240e..996b95ab74aa 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2088,6 +2088,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);
> @@ -2096,19 +2097,12 @@ 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);
> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> +       ufshcd_writel(hba, 1 << task_tag,
> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>         /* Make sure that doorbell is committed immediately */
>         wmb();
>  }
> @@ -2890,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->host->host_lock, flags);
> +               __clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
> +               spin_unlock_irqrestore(hba->host->host_lock, flags);
>         }
> 
>         return err;
> @@ -5197,8 +5193,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;
> @@ -5241,6 +5235,7 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>  static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
>  {
>         unsigned long completed_reqs;
> +       unsigned long flags;
> 
>         /* Resetting interrupt aggregation counters first and reading the
>          * DOOR_BELL afterward allows us to handle all the completed requests.
> @@ -5253,6 +5248,7 @@ 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);
> 
> +       spin_lock_irqsave(hba->host->host_lock, flags);
>         if (use_utrlcnr) {
>                 completed_reqs = ufshcd_readl(hba,
>                                               REG_UTP_TRANSFER_REQ_LIST_COMPL);
> @@ -5260,14 +5256,16 @@ static irqreturn_t ufshcd_trc_handler(struct
> ufs_hba *hba, bool use_utrlcnr)
>                         ufshcd_writel(hba, completed_reqs,
>                                       REG_UTP_TRANSFER_REQ_LIST_COMPL);
>         } 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);
> +               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->host->host_lock, flags);
> 
>         if (completed_reqs) {
>                 ufshcd_transfer_req_compl(hba, completed_reqs);

RE: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Avri Altman]

One more small comment.

Thanks,
Avri

> >
> > The following unlikely races can be triggered by the completion path
> > (ufshcd_trc_handler()):
> > - After the UTRLCNR register has been read from interrupt context and
> >   before it is cleared, the UFS error handler reads the UTRLCNR register.
> >   Hold the SCSI host lock until the UTRLCNR register has been cleared to
> >   prevent that this register is accessed from another CPU before it has
> >   been cleared.
> > - After the doorbell register has been read and before outstanding_reqs
> >   is cleared, the error handler reads the doorbell register. This can also
> >   result in double completions. Fix this by clearing outstanding_reqs
> >   before calling ufshcd_transfer_req_compl().
> >
> > Due to this change ufshcd_trc_handler() no longer updates
> > outstanding_reqs
> > atomically. Hence protect all other outstanding_reqs changes with the SCSI
> > host lock.
> But isn't the whole point of using REG_UTP_TRANSFER_REQ_LIST_COMPL is to
> eliminate the host lock
> As a source of contention?
> 
> >
> > This patch is a performance improvement because it reduces the number
> of
> > atomic operations in the hot path (test_and_clear_bit()).
> Both Can & Stanley reported a performance improvement of RR with
> "Optimize host lock..".
> Can those short numerical studies can be repeated with this patch?
Please use rq_affinity = 2 this time.

Thanks,
Avri

> 
> Thanks,
> Avri
> 
> >
> > See also commit a45f937110fa ("scsi: ufs: Optimize host lock on transfer
> > requests send/compl paths").
> >
> > 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 | 36 +++++++++++++++++-------------------
> >  1 file changed, 17 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 83a32b71240e..996b95ab74aa 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -2088,6 +2088,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);
> > @@ -2096,19 +2097,12 @@ 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);
> > +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +
> > +       ufshcd_writel(hba, 1 << task_tag,
> > REG_UTP_TRANSFER_REQ_DOOR_BELL);
> >         /* Make sure that doorbell is committed immediately */
> >         wmb();
> >  }
> > @@ -2890,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->host->host_lock, flags);
> > +               __clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
> > +               spin_unlock_irqrestore(hba->host->host_lock, flags);
> >         }
> >
> >         return err;
> > @@ -5197,8 +5193,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;
> > @@ -5241,6 +5235,7 @@ static void ufshcd_transfer_req_compl(struct
> > ufs_hba *hba,
> >  static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool
> use_utrlcnr)
> >  {
> >         unsigned long completed_reqs;
> > +       unsigned long flags;
> >
> >         /* Resetting interrupt aggregation counters first and reading the
> >          * DOOR_BELL afterward allows us to handle all the completed
> requests.
> > @@ -5253,6 +5248,7 @@ 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);
> >
> > +       spin_lock_irqsave(hba->host->host_lock, flags);
> >         if (use_utrlcnr) {
> >                 completed_reqs = ufshcd_readl(hba,
> >                                               REG_UTP_TRANSFER_REQ_LIST_COMPL);
> > @@ -5260,14 +5256,16 @@ static irqreturn_t ufshcd_trc_handler(struct
> > ufs_hba *hba, bool use_utrlcnr)
> >                         ufshcd_writel(hba, completed_reqs,
> >                                       REG_UTP_TRANSFER_REQ_LIST_COMPL);
> >         } 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);
> > +               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->host->host_lock, flags);
> >
> >         if (completed_reqs) {
> >                 ufshcd_transfer_req_compl(hba, completed_reqs);

Re: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Bart Van Assche]

On 7/11/21 5:29 AM, Avri Altman wrote:
>>
>> The following unlikely races can be triggered by the completion path
>> (ufshcd_trc_handler()):
>> - After the UTRLCNR register has been read from interrupt context and
>>    before it is cleared, the UFS error handler reads the UTRLCNR register.
>>    Hold the SCSI host lock until the UTRLCNR register has been cleared to
>>    prevent that this register is accessed from another CPU before it has
>>    been cleared.
>> - After the doorbell register has been read and before outstanding_reqs
>>    is cleared, the error handler reads the doorbell register. This can also
>>    result in double completions. Fix this by clearing outstanding_reqs
>>    before calling ufshcd_transfer_req_compl().
>>
>> Due to this change ufshcd_trc_handler() no longer updates
>> outstanding_reqs
>> atomically. Hence protect all other outstanding_reqs changes with the SCSI
>> host lock.
> But isn't the whole point of REG_UTP_TRANSFER_REQ_LIST_COMPL is to eliminate the host lock
> As a source of contention?

How about avoiding contention by introducing a new spinlock to protect 
hba->outstanding_reqs?

>> This patch is a performance improvement because it reduces the number of
>> atomic operations in the hot path (test_and_clear_bit()).
> Both Can & Stanley reported a performance improvement of RR with "Optimize host lock..".
> Can those short numerical studies can be repeated with this patch?

I will measure the performance impact of this patch for rq_affinity=2 as 
soon as I have the time. As you may know we are close to an internal 
deadline.

Thanks,

Bart.

Re: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Bart Van Assche]

On 7/13/21 9:49 AM, Bart Van Assche wrote:
> On 7/11/21 5:29 AM, Avri Altman wrote:
>>> This patch is a performance improvement because it reduces the number of
>>> atomic operations in the hot path (test_and_clear_bit()).
>> Both Can & Stanley reported a performance improvement of RR with 
>> "Optimize host lock..".
>> Can those short numerical studies can be repeated with this patch?
> 
> I will measure the performance impact of this patch for rq_affinity=2 as 
> soon as I have the time. As you may know we are close to an internal 
> deadline.

(replying to my own email)

Hi Avri,

The performance I measure with the current upstream UFS driver is 61.0 K 
IOPS. With a variant of this patch (outstanding_reqs protected with a 
new spinlock instead of the host lock), I see 62.0 K IOPS. In other 
words, this patch realizes a small performance improvement. This is what 
I had expected since this patch reduces the number of atomic operations 
involved in updating outstanding_reqs.

Thanks,

Bart.

RE: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Avri Altman]

 
> On 7/13/21 9:49 AM, Bart Van Assche wrote:
> > On 7/11/21 5:29 AM, Avri Altman wrote:
> >>> This patch is a performance improvement because it reduces the
> >>> number of atomic operations in the hot path (test_and_clear_bit()).
> >> Both Can & Stanley reported a performance improvement of RR with
> >> "Optimize host lock..".
> >> Can those short numerical studies can be repeated with this patch?
> >
> > I will measure the performance impact of this patch for rq_affinity=2
> > as soon as I have the time. As you may know we are close to an
> > internal deadline.
> 
> (replying to my own email)
> 
> Hi Avri,
> 
> The performance I measure with the current upstream UFS driver is 61.0 K IOPS.
> With a variant of this patch (outstanding_reqs protected with a new spinlock
> instead of the host lock), I see 62.0 K IOPS. In other words, this patch realizes a
> small performance improvement. This is what I had expected since this patch
> reduces the number of atomic operations involved in updating
> outstanding_reqs.
Thank you for taking the time and running this.
But does your platform make use of REG_UTP_TRANSFER_REQ_LIST_COMPL?
With 60k IOPS I suspect it doesn't, and the comparison is irrelevant.

Thanks,
Avri 

Re: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Asutosh Das (asd)]

On 7/16/2021 5:39 AM, Avri Altman wrote:
>   
>> On 7/13/21 9:49 AM, Bart Van Assche wrote:
>>> On 7/11/21 5:29 AM, Avri Altman wrote:
>>>>> This patch is a performance improvement because it reduces the
>>>>> number of atomic operations in the hot path (test_and_clear_bit()).
>>>> Both Can & Stanley reported a performance improvement of RR with
>>>> "Optimize host lock..".
>>>> Can those short numerical studies can be repeated with this patch?
>>>
>>> I will measure the performance impact of this patch for rq_affinity=2
>>> as soon as I have the time. As you may know we are close to an
>>> internal deadline.
>>
>> (replying to my own email)
>>
>> Hi Avri,
>>
>> The performance I measure with the current upstream UFS driver is 61.0 K IOPS.
>> With a variant of this patch (outstanding_reqs protected with a new spinlock
>> instead of the host lock), I see 62.0 K IOPS. In other words, this patch realizes a
>> small performance improvement. This is what I had expected since this patch
>> reduces the number of atomic operations involved in updating
>> outstanding_reqs.
> Thank you for taking the time and running this.
> But does your platform make use of REG_UTP_TRANSFER_REQ_LIST_COMPL?
> With 60k IOPS I suspect it doesn't, and the comparison is irrelevant.
> 
> Thanks,
> Avri
> 
I agree. We saw substantial improvement with RR and RW too with the 
'Optimize host lock change'.

Hi Bart,
Is it possible to check the performance data with these changes on 
Android, say using Androbench?


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

Re: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Bart Van Assche]

On 7/16/21 9:26 AM, Asutosh Das (asd) wrote:
> I agree. We saw substantial improvement with RR and RW too with the 
> 'Optimize host lock change'.

Recent UFS driver patches introduced three changes:
(1) Use the UTRLCNR register instead of the doorbell register in the 
completion path.
(2) Use atomic instructions instead of the host lock for updating the 
outstanding_reqs structure member.
(3) Reduce lock contention on the SCSI host lock.

My patch preserves (3) so it should preserve the performance 
improvements that are the result of eliminating lock contention for 
outstanding_reqs updates.

Thanks,

Bart.

Re: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Bart Van Assche]

On 7/16/21 9:54 AM, Bart Van Assche wrote:
> On 7/16/21 9:26 AM, Asutosh Das (asd) wrote:
>> I agree. We saw substantial improvement with RR and RW too with the 
>> 'Optimize host lock change'.
> 
> Recent UFS driver patches introduced three changes:
> (1) Use the UTRLCNR register instead of the doorbell register in the 
> completion path.
> (2) Use atomic instructions instead of the host lock for updating the 
> outstanding_reqs structure member.
> (3) Reduce lock contention on the SCSI host lock.
> 
> My patch preserves (3) so it should preserve the performance 
> improvements that are the result of eliminating lock contention for 
> outstanding_reqs updates.

For clarity, this is the patch for which I reported a 1% performance improvement:

Subject: [PATCH] ufs: Fix a race in the completion path

The following unlikely races can be triggered by the completion path
(ufshcd_trc_handler()):
- After the UTRLCNR register has been read from interrupt context and
   before it is cleared, the UFS error handler reads the UTRLCNR register.
   Hold the SCSI host lock until the UTRLCNR register has been cleared to
   prevent that this register is accessed from another CPU before it has
   been cleared.
- After the doorbell register has been read and before outstanding_reqs
   is cleared, the error handler reads the doorbell register. This can also
   result in double completions. Fix this by clearing outstanding_reqs
   before calling ufshcd_transfer_req_compl().

Due to this change ufshcd_trc_handler() no longer updates outstanding_reqs
atomically. Hence protect all other outstanding_reqs changes with the SCSI
host lock.

This patch is a performance improvement because it reduces the number of
atomic operations in the hot path (test_and_clear_bit()).

See also commit a45f937110fa ("scsi: ufs: Optimize host lock on transfer
requests send/compl paths").

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 |  2 ++
  2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0cb84a744dad..7b8d3928fed8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2088,6 +2088,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);
@@ -2096,19 +2097,12 @@ 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->outstanding_lock, flags);
+	__set_bit(task_tag, &hba->outstanding_reqs);
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+
+	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
  	/* Make sure that doorbell is committed immediately */
  	wmb();
  }
@@ -2890,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;
@@ -5197,8 +5193,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;
@@ -5241,6 +5235,7 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba,
  static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
  {
  	unsigned long completed_reqs = 0;
+	unsigned long flags;

  	/* Resetting interrupt aggregation counters first and reading the
  	 * DOOR_BELL afterward allows us to handle all the completed requests.
@@ -5253,24 +5248,24 @@ 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);

+	spin_lock_irqsave(&hba->outstanding_lock, flags);
  	if (use_utrlcnr) {
-		u32 utrlcnr;
-
-		utrlcnr = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_LIST_COMPL);
-		if (utrlcnr) {
-			ufshcd_writel(hba, utrlcnr,
+		completed_reqs = ufshcd_readl(hba,
+					      REG_UTP_TRANSFER_REQ_LIST_COMPL);
+		if (completed_reqs)
+			ufshcd_writel(hba, completed_reqs,
  				      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);
+		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);
@@ -9357,10 +9352,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 f8766e8f3cac..e47a796bc114 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 v2 13/19] scsi: ufs: Fix a race in the completion path

[Bart Van Assche]

On 7/16/21 5:39 AM, Avri Altman wrote:
> But does your platform make use of REG_UTP_TRANSFER_REQ_LIST_COMPL?
> With 60k IOPS I suspect it doesn't, and the comparison is irrelevant.

Yes, my test setup supports the UTRLCNR register (let's use the acronyms 
from the UFS specification). However, it is not clear to me why that 
register has been introduced? Using that register in the completion path 
requires one register read + one register write while using the doorbell 
register in the completion path involves a single register read. My 
understanding is that register reads and writes are much slower than 
DRAM reads or writes. Does this mean that using the doorbell register in 
the compleiton path is always faster than using the UTRLCNR register?

Thanks,

Bart.

[PATCH v2 14/19] scsi: ufs: Use the doorbell register instead of the UTRLCNR register

[Bart Van Assche]

Using the UTRLCNR register implies performing two MMIO accesses in the hot
path while reading the doorbell register only involves a single MMIO
operation. Hence do not use the UTRLNCR register.

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 | 2 +-
 drivers/scsi/ufs/ufshcd.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 996b95ab74aa..becd9e2829f4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6388,7 +6388,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_trc_handler(hba, ufshcd_use_utrlcnr(hba));
 
 	return retval;
 }
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index f8766e8f3cac..b3d9b487846f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1184,9 +1184,9 @@ 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)
+static inline bool ufshcd_use_utrlcnr(struct ufs_hba *hba)
 {
-	return (hba->ufs_version >= ufshci_version(3, 0));
+	return false;
 }
 
 static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,

RE: [PATCH v2 14/19] scsi: ufs: Use the doorbell register instead of the UTRLCNR register

[Avri Altman]

Using the UTRLCNR register implies performing two MMIO accesses in the hot path while reading the doorbell register only involves a single MMIO operation. Hence do not use the UTRLNCR register.

Isn't this patch, and the one before, practically reverting
6f7151729647 (scsi: ufs: Utilize Transfer Request List Completion Notification Register)?
Wouldn't it be simpler then just revert it in #13, and add whatever is needed in #14?

Thanks,
Avri

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 | 2 +-
 drivers/scsi/ufs/ufshcd.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 996b95ab74aa..becd9e2829f4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6388,7 +6388,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_trc_handler(hba, 
+ ufshcd_use_utrlcnr(hba));

        return retval;
 }
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index f8766e8f3cac..b3d9b487846f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1184,9 +1184,9 @@ 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)
+static inline bool ufshcd_use_utrlcnr(struct ufs_hba *hba)
 {
-       return (hba->ufs_version >= ufshci_version(3, 0));
+       return false;
 }

 static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,

[PATCH v2 15/19] 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 | 62 +++++++++++++--------------------------
 1 file changed, 20 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index becd9e2829f4..ea3c2d052123 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;
@@ -2926,11 +2917,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	tag = req->tag;
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
-	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);
@@ -6625,11 +6611,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	tag = req->tag;
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
-	if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
-		err = -EBUSY;
-		goto out;
-	}
-
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	lrbp->cmd = NULL;
@@ -6928,19 +6909,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, res;
 	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 */
@@ -6969,7 +6950,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;
 	}
 
 	/*
@@ -6982,36 +6964,32 @@ 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 {
-		dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
+	res = ufshcd_try_to_abort_task(hba, tag);
+	if (res) {
+		dev_err(hba->dev, "%s: failed with err %d\n", __func__, res);
 		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;
 }

[PATCH v2 16/19] 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 ea3c2d052123..ae04c7ed9766 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_SCSI_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

[PATCH v2 17/19] 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 ae04c7ed9766..ae21240a6548 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"
@@ -233,7 +235,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,
@@ -3901,6 +3902,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.
@@ -3921,6 +3951,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;
@@ -3990,10 +4021,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;
@@ -5808,27 +5843,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);
@@ -5962,11 +5976,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;
@@ -5974,10 +5988,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;
@@ -6291,7 +6304,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;
 	}
 	/*
@@ -6303,6 +6315,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;
 }
 
@@ -6960,15 +6976,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;
@@ -9366,7 +9386,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,
@@ -9420,17 +9439,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 b3d9b487846f..b821851a62eb 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -715,8 +715,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
@@ -818,8 +816,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 */

[PATCH v2 18/19] 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 | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ae21240a6548..46126964669d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5202,10 +5202,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;
@@ -5221,7 +5223,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 */
@@ -5247,13 +5250,15 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba,
 /**
  * ufshcd_trc_handler - handle transfer requests completion
  * @hba: per adapter instance
+ * @retry_requests: whether or not to ask to retry requests
  * @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_trc_handler(struct ufs_hba *hba, bool retry_requests,
+				      bool use_utrlcnr)
 {
 	unsigned long completed_reqs;
 	unsigned long flags;
@@ -5289,7 +5294,7 @@ static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
 	spin_unlock_irqrestore(hba->host->host_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;
@@ -5768,7 +5773,14 @@ 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_trc_handler(hba, /*retry_requests=*/false,
+			   /*use_utrlcnr=*/false);
+	ufshcd_tmc_handler(hba);
+}
+
+static void ufshcd_retry_aborted_requests(struct ufs_hba *hba)
+{
+	ufshcd_trc_handler(hba, /*retry_requests=*/true, /*use_utrlcnr=*/false);
 	ufshcd_tmc_handler(hba);
 }
 
@@ -6088,8 +6100,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;
@@ -6390,7 +6401,8 @@ 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_use_utrlcnr(hba));
+		retval |= ufshcd_trc_handler(hba, /*retry_requests=*/false,
+					     ufshcd_use_utrlcnr(hba));
 
 	return retval;
 }
@@ -6804,7 +6816,8 @@ 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);
 		}
 	}
 
@@ -6966,7 +6979,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);
-		ufshcd_transfer_req_compl(hba, 1UL << tag);
+		ufshcd_transfer_req_compl(hba, 1UL << tag,
+					  /*retry_requests=*/false);
 		goto release;
 	}
 
@@ -7032,7 +7046,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 */

[PATCH v2 19/19] scsi: ufs: Add fault injection support

[Bart Van Assche]

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

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 | 67 ++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-fault-injection.h | 24 +++++++++
 drivers/scsi/ufs/ufshcd.c              |  8 +++
 5 files changed, 107 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..f1e126a70019
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-fault-injection.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/kconfig.h>
+#include <linux/module.h>
+#include <linux/fault-inject.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_ATTR_SIZE = 80 };
+
+/*
+ * For more details about fault injection, please refer to
+ * Documentation/fault-injection/fault-injection.rst.
+ */
+#define UFS_FAULT_ATTR(storage_class, name)				\
+storage_class char g_##name##_str[FAULT_ATTR_SIZE];			\
+module_param_cb(name, &ufs_fault_ops, g_##name##_str, 0644);		\
+MODULE_PARM_DESC(name,							\
+"Fault injection. " #name "=<interval>,<probability>,<space>,<times>");	\
+storage_class DECLARE_FAULT_ATTR(ufs_##name##_attr)
+
+UFS_FAULT_ATTR(static, trigger_eh);
+UFS_FAULT_ATTR(static, timeout);
+
+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_ATTR_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 46126964669d..686486c25412 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;
 }
 
@@ -5274,6 +5279,9 @@ static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool retry_requests,
 	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
 		ufshcd_reset_intr_aggr(hba);
 
+	if (ufs_fail_completion())
+		return IRQ_HANDLED;
+
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (use_utrlcnr) {
 		completed_reqs = ufshcd_readl(hba,

Re: [PATCH v2 19/19] scsi: ufs: Add fault injection support

[Randy Dunlap]

Hi Bart,

On 7/9/21 1:26 PM, Bart Van Assche wrote:
> 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

Nit: use one tab + 2 spaces above for indentation.

> +	 to test the UFS error handler and abort handler.

thanks.

Re: [PATCH v2 19/19] scsi: ufs: Add fault injection support

[Bart Van Assche]

On 7/9/21 2:56 PM, Randy Dunlap wrote:
> On 7/9/21 1:26 PM, Bart Van Assche wrote:
>> 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
> 
> Nit: use one tab + 2 spaces above for indentation.

I will change the indentation in the Kconfig file. Thanks for the feedback.

Bart.

Re: [PATCH] fault-inject: Declare the second argument of setup_fault_attr() const

[Bart Van Assche]

On 7/9/21 1:26 PM, Bart Van Assche wrote:
> This patch makes it possible to pass a const char * argument to
> setup_fault_attr() without having to cast away constness.

Please ignore this patch - it has already been posted to the LKML.

Bart.