[PATCH v2 0/4] UFS host controller driver patches

[PATCH v2 0/4] UFS host controller driver patches

[Bart Van Assche]

Hi Martin,

Please consider these four UFS host controller driver patches for the next
merge window.

Thanks,

Bart.

Changes compared to v1:
- Added a comment in patch 4/4 as requested by Adrian Hunter.

Bart Van Assche (4):
  scsi: ufs: Increase the START STOP UNIT timeout from one to ten
    seconds
  scsi: ufs: Fix handling of lrbp->cmd
  scsi: ufs: Move ufshcd_wl_shutdown()
  scsi: ufs: Simplify driver shutdown

 drivers/ufs/core/ufshcd.c             | 65 ++++++++++-----------------
 drivers/ufs/host/cdns-pltfrm.c        |  1 -
 drivers/ufs/host/tc-dwc-g210-pci.c    | 10 -----
 drivers/ufs/host/tc-dwc-g210-pltfrm.c |  1 -
 drivers/ufs/host/ufs-exynos.c         |  1 -
 drivers/ufs/host/ufs-hisi.c           |  1 -
 drivers/ufs/host/ufs-mediatek.c       |  1 -
 drivers/ufs/host/ufs-qcom.c           |  1 -
 drivers/ufs/host/ufs-sprd.c           |  1 -
 drivers/ufs/host/ufshcd-pci.c         | 10 -----
 drivers/ufs/host/ufshcd-pltfrm.c      |  6 ---
 drivers/ufs/host/ufshcd-pltfrm.h      |  1 -
 include/ufs/ufshcd.h                  |  1 -
 13 files changed, 24 insertions(+), 76 deletions(-)

[PATCH v2 1/4] scsi: ufs: Increase the START STOP UNIT timeout from one to ten seconds

[Bart Van Assche]

One UFS vendor asked to increase the UFS timeout from 1 s to 3 s.
Another UFS vendor asked to increase the UFS timeout from 1 s to 10 s.
Hence this patch that increases the UFS timeout to 10 s. This patch can
cause the total timeout to exceed 20 s, the Android shutdown timeout.
This is fine since the loop around ufshcd_execute_start_stop() exists to
deal with unit attentions and because unit attentions are reported
quickly.

Fixes: dcd5b7637c6d ("scsi: ufs: Reduce the START STOP UNIT timeout")
Fixes: 8f2c96420c6e ("scsi: ufs: core: Reduce the power mode change timeout")
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4f14a8f72da6..37337d411466 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9167,7 +9167,8 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
 	};
 
 	return scsi_execute_cmd(sdev, cdb, REQ_OP_DRV_IN, /*buffer=*/NULL,
-			/*bufflen=*/0, /*timeout=*/HZ, /*retries=*/0, &args);
+			/*bufflen=*/0, /*timeout=*/10 * HZ, /*retries=*/0,
+			&args);
 }
 
 /**

[PATCH v2 2/4] scsi: ufs: Fix handling of lrbp->cmd

[Bart Van Assche]

ufshcd_queuecommand() may be called two times in a row for a SCSI
command before it is completed. Hence make the following changes:
- In the functions that submit a command, do not check the old value of
  lrbp->cmd nor clear lrbp->cmd in error paths.
- In ufshcd_release_scsi_cmd(), do not clear lrbp->cmd.

See also scsi_send_eh_cmnd().

This patch prevents that the following appears if a command times out:

WARNING: at drivers/ufs/core/ufshcd.c:2965 ufshcd_queuecommand+0x6f8/0x9a8
Call trace:
 ufshcd_queuecommand+0x6f8/0x9a8
 scsi_send_eh_cmnd+0x2c0/0x960
 scsi_eh_test_devices+0x100/0x314
 scsi_eh_ready_devs+0xd90/0x114c
 scsi_error_handler+0x2b4/0xb70
 kthread+0x16c/0x1e0

Fixes: 5a0b0cb9bee7 ("[SCSI] ufs: Add support for sending NOP OUT UPIU")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 37337d411466..68d9e24fac98 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2928,7 +2928,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 		(hba->clk_gating.state != CLKS_ON));
 
 	lrbp = &hba->lrb[tag];
-	WARN_ON(lrbp->cmd);
 	lrbp->cmd = cmd;
 	lrbp->task_tag = tag;
 	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
@@ -2944,7 +2943,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	err = ufshcd_map_sg(hba, lrbp);
 	if (err) {
-		lrbp->cmd = NULL;
 		ufshcd_release(hba);
 		goto out;
 	}
@@ -5405,7 +5403,6 @@ static void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
 	struct scsi_cmnd *cmd = lrbp->cmd;
 
 	scsi_dma_unmap(cmd);
-	lrbp->cmd = NULL;	/* Mark the command as completed. */
 	ufshcd_release(hba);
 	ufshcd_clk_scaling_update_busy(hba);
 }
@@ -7020,7 +7017,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	down_read(&hba->clk_scaling_lock);
 
 	lrbp = &hba->lrb[tag];
-	WARN_ON(lrbp->cmd);
 	lrbp->cmd = NULL;
 	lrbp->task_tag = tag;
 	lrbp->lun = 0;
@@ -7192,7 +7188,6 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 	down_read(&hba->clk_scaling_lock);
 
 	lrbp = &hba->lrb[tag];
-	WARN_ON(lrbp->cmd);
 	lrbp->cmd = NULL;
 	lrbp->task_tag = tag;
 	lrbp->lun = UFS_UPIU_RPMB_WLUN;

[PATCH v2 3/4] scsi: ufs: Move ufshcd_wl_shutdown()

[Bart Van Assche]

Move the definition of ufshcd_wl_shutdown() to make the next patch in
this series easier to review.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 44 +++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 68d9e24fac98..0f426d46d91e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9746,28 +9746,6 @@ static int ufshcd_wl_resume(struct device *dev)
 }
 #endif
 
-static void ufshcd_wl_shutdown(struct device *dev)
-{
-	struct scsi_device *sdev = to_scsi_device(dev);
-	struct ufs_hba *hba;
-
-	hba = shost_priv(sdev->host);
-
-	down(&hba->host_sem);
-	hba->shutting_down = true;
-	up(&hba->host_sem);
-
-	/* Turn on everything while shutting down */
-	ufshcd_rpm_get_sync(hba);
-	scsi_device_quiesce(sdev);
-	shost_for_each_device(sdev, hba->host) {
-		if (sdev == hba->ufs_device_wlun)
-			continue;
-		scsi_device_quiesce(sdev);
-	}
-	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
-}
-
 /**
  * ufshcd_suspend - helper function for suspend operations
  * @hba: per adapter instance
@@ -9952,6 +9930,28 @@ int ufshcd_runtime_resume(struct device *dev)
 EXPORT_SYMBOL(ufshcd_runtime_resume);
 #endif /* CONFIG_PM */
 
+static void ufshcd_wl_shutdown(struct device *dev)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufs_hba *hba;
+
+	hba = shost_priv(sdev->host);
+
+	down(&hba->host_sem);
+	hba->shutting_down = true;
+	up(&hba->host_sem);
+
+	/* Turn on everything while shutting down */
+	ufshcd_rpm_get_sync(hba);
+	scsi_device_quiesce(sdev);
+	shost_for_each_device(sdev, hba->host) {
+		if (sdev == hba->ufs_device_wlun)
+			continue;
+		scsi_device_quiesce(sdev);
+	}
+	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
+}
+
 /**
  * ufshcd_shutdown - shutdown routine
  * @hba: per adapter instance

[PATCH v2 4/4] scsi: ufs: Simplify driver shutdown

[Bart Van Assche]

All UFS host drivers call ufshcd_shutdown(). Hence, instead of calling
ufshcd_shutdown() from the host driver .shutdown() callback, inline that
function into ufshcd_wl_shutdown().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c             | 23 +++++------------------
 drivers/ufs/host/cdns-pltfrm.c        |  1 -
 drivers/ufs/host/tc-dwc-g210-pci.c    | 10 ----------
 drivers/ufs/host/tc-dwc-g210-pltfrm.c |  1 -
 drivers/ufs/host/ufs-exynos.c         |  1 -
 drivers/ufs/host/ufs-hisi.c           |  1 -
 drivers/ufs/host/ufs-mediatek.c       |  1 -
 drivers/ufs/host/ufs-qcom.c           |  1 -
 drivers/ufs/host/ufs-sprd.c           |  1 -
 drivers/ufs/host/ufshcd-pci.c         | 10 ----------
 drivers/ufs/host/ufshcd-pltfrm.c      |  6 ------
 drivers/ufs/host/ufshcd-pltfrm.h      |  1 -
 include/ufs/ufshcd.h                  |  1 -
 13 files changed, 5 insertions(+), 53 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0f426d46d91e..7ee150d67d49 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9933,9 +9933,7 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
 static void ufshcd_wl_shutdown(struct device *dev)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
-	struct ufs_hba *hba;
-
-	hba = shost_priv(sdev->host);
+	struct ufs_hba *hba = shost_priv(sdev->host);
 
 	down(&hba->host_sem);
 	hba->shutting_down = true;
@@ -9950,27 +9948,16 @@ static void ufshcd_wl_shutdown(struct device *dev)
 		scsi_device_quiesce(sdev);
 	}
 	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
-}
 
-/**
- * ufshcd_shutdown - shutdown routine
- * @hba: per adapter instance
- *
- * This function would turn off both UFS device and UFS hba
- * regulators. It would also disable clocks.
- *
- * Returns 0 always to allow force shutdown even in case of errors.
- */
-int ufshcd_shutdown(struct ufs_hba *hba)
-{
+	/*
+	 * Next, turn off the UFS controller and the UFS regulators. Disable
+	 * clocks.
+	 */
 	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
 		ufshcd_suspend(hba);
 
 	hba->is_powered = false;
-	/* allow force shutdown even in case of errors */
-	return 0;
 }
-EXPORT_SYMBOL(ufshcd_shutdown);
 
 /**
  * ufshcd_remove - de-allocate SCSI host and host memory space
diff --git a/drivers/ufs/host/cdns-pltfrm.c b/drivers/ufs/host/cdns-pltfrm.c
index e05c0ae64eea..26761425a76c 100644
--- a/drivers/ufs/host/cdns-pltfrm.c
+++ b/drivers/ufs/host/cdns-pltfrm.c
@@ -328,7 +328,6 @@ static const struct dev_pm_ops cdns_ufs_dev_pm_ops = {
 static struct platform_driver cdns_ufs_pltfrm_driver = {
 	.probe	= cdns_ufs_pltfrm_probe,
 	.remove	= cdns_ufs_pltfrm_remove,
-	.shutdown = ufshcd_pltfrm_shutdown,
 	.driver	= {
 		.name   = "cdns-ufshcd",
 		.pm     = &cdns_ufs_dev_pm_ops,
diff --git a/drivers/ufs/host/tc-dwc-g210-pci.c b/drivers/ufs/host/tc-dwc-g210-pci.c
index 92b8ad4b58fe..f96fe5855841 100644
--- a/drivers/ufs/host/tc-dwc-g210-pci.c
+++ b/drivers/ufs/host/tc-dwc-g210-pci.c
@@ -32,15 +32,6 @@ static struct ufs_hba_variant_ops tc_dwc_g210_pci_hba_vops = {
 	.link_startup_notify	= ufshcd_dwc_link_startup_notify,
 };
 
-/**
- * tc_dwc_g210_pci_shutdown - main function to put the controller in reset state
- * @pdev: pointer to PCI device handle
- */
-static void tc_dwc_g210_pci_shutdown(struct pci_dev *pdev)
-{
-	ufshcd_shutdown((struct ufs_hba *)pci_get_drvdata(pdev));
-}
-
 /**
  * tc_dwc_g210_pci_remove - de-allocate PCI/SCSI host and host memory space
  *		data structure memory
@@ -137,7 +128,6 @@ static struct pci_driver tc_dwc_g210_pci_driver = {
 	.id_table = tc_dwc_g210_pci_tbl,
 	.probe = tc_dwc_g210_pci_probe,
 	.remove = tc_dwc_g210_pci_remove,
-	.shutdown = tc_dwc_g210_pci_shutdown,
 	.driver = {
 		.pm = &tc_dwc_g210_pci_pm_ops
 	},
diff --git a/drivers/ufs/host/tc-dwc-g210-pltfrm.c b/drivers/ufs/host/tc-dwc-g210-pltfrm.c
index f15a84d0c176..4d5389dd9585 100644
--- a/drivers/ufs/host/tc-dwc-g210-pltfrm.c
+++ b/drivers/ufs/host/tc-dwc-g210-pltfrm.c
@@ -92,7 +92,6 @@ static const struct dev_pm_ops tc_dwc_g210_pltfm_pm_ops = {
 static struct platform_driver tc_dwc_g210_pltfm_driver = {
 	.probe		= tc_dwc_g210_pltfm_probe,
 	.remove		= tc_dwc_g210_pltfm_remove,
-	.shutdown = ufshcd_pltfrm_shutdown,
 	.driver		= {
 		.name	= "tc-dwc-g210-pltfm",
 		.pm	= &tc_dwc_g210_pltfm_pm_ops,
diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 0bf5390739e1..f41056f57fd7 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1757,7 +1757,6 @@ static const struct dev_pm_ops exynos_ufs_pm_ops = {
 static struct platform_driver exynos_ufs_pltform = {
 	.probe	= exynos_ufs_probe,
 	.remove	= exynos_ufs_remove,
-	.shutdown = ufshcd_pltfrm_shutdown,
 	.driver	= {
 		.name	= "exynos-ufshc",
 		.pm	= &exynos_ufs_pm_ops,
diff --git a/drivers/ufs/host/ufs-hisi.c b/drivers/ufs/host/ufs-hisi.c
index 4c423eba8aa9..18b72e2e68c1 100644
--- a/drivers/ufs/host/ufs-hisi.c
+++ b/drivers/ufs/host/ufs-hisi.c
@@ -593,7 +593,6 @@ static const struct dev_pm_ops ufs_hisi_pm_ops = {
 static struct platform_driver ufs_hisi_pltform = {
 	.probe	= ufs_hisi_probe,
 	.remove	= ufs_hisi_remove,
-	.shutdown = ufshcd_pltfrm_shutdown,
 	.driver	= {
 		.name	= "ufshcd-hisi",
 		.pm	= &ufs_hisi_pm_ops,
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index a054810e321d..33b301649757 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1647,7 +1647,6 @@ static const struct dev_pm_ops ufs_mtk_pm_ops = {
 static struct platform_driver ufs_mtk_pltform = {
 	.probe      = ufs_mtk_probe,
 	.remove     = ufs_mtk_remove,
-	.shutdown   = ufshcd_pltfrm_shutdown,
 	.driver = {
 		.name   = "ufshcd-mtk",
 		.pm     = &ufs_mtk_pm_ops,
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 82d02e7f3b4f..059de74dfea3 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1723,7 +1723,6 @@ static const struct dev_pm_ops ufs_qcom_pm_ops = {
 static struct platform_driver ufs_qcom_pltform = {
 	.probe	= ufs_qcom_probe,
 	.remove	= ufs_qcom_remove,
-	.shutdown = ufshcd_pltfrm_shutdown,
 	.driver	= {
 		.name	= "ufshcd-qcom",
 		.pm	= &ufs_qcom_pm_ops,
diff --git a/drivers/ufs/host/ufs-sprd.c b/drivers/ufs/host/ufs-sprd.c
index 051f3f40d92c..2bad75dd6d58 100644
--- a/drivers/ufs/host/ufs-sprd.c
+++ b/drivers/ufs/host/ufs-sprd.c
@@ -444,7 +444,6 @@ static const struct dev_pm_ops ufs_sprd_pm_ops = {
 static struct platform_driver ufs_sprd_pltform = {
 	.probe = ufs_sprd_probe,
 	.remove = ufs_sprd_remove,
-	.shutdown = ufshcd_pltfrm_shutdown,
 	.driver = {
 		.name = "ufshcd-sprd",
 		.pm = &ufs_sprd_pm_ops,
diff --git a/drivers/ufs/host/ufshcd-pci.c b/drivers/ufs/host/ufshcd-pci.c
index 9c911787f84c..38276dac8e52 100644
--- a/drivers/ufs/host/ufshcd-pci.c
+++ b/drivers/ufs/host/ufshcd-pci.c
@@ -504,15 +504,6 @@ static int ufshcd_pci_restore(struct device *dev)
 }
 #endif
 
-/**
- * ufshcd_pci_shutdown - main function to put the controller in reset state
- * @pdev: pointer to PCI device handle
- */
-static void ufshcd_pci_shutdown(struct pci_dev *pdev)
-{
-	ufshcd_shutdown((struct ufs_hba *)pci_get_drvdata(pdev));
-}
-
 /**
  * ufshcd_pci_remove - de-allocate PCI/SCSI host and host memory space
  *		data structure memory
@@ -618,7 +609,6 @@ static struct pci_driver ufshcd_pci_driver = {
 	.id_table = ufshcd_pci_tbl,
 	.probe = ufshcd_pci_probe,
 	.remove = ufshcd_pci_remove,
-	.shutdown = ufshcd_pci_shutdown,
 	.driver = {
 		.pm = &ufshcd_pci_pm_ops
 	},
diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index 5739ff007828..0b7430033047 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -190,12 +190,6 @@ static int ufshcd_parse_regulator_info(struct ufs_hba *hba)
 	return err;
 }
 
-void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
-{
-	ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
-}
-EXPORT_SYMBOL_GPL(ufshcd_pltfrm_shutdown);
-
 static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
 {
 	struct device *dev = hba->dev;
diff --git a/drivers/ufs/host/ufshcd-pltfrm.h b/drivers/ufs/host/ufshcd-pltfrm.h
index 2e4ba2bfbcad..2df108f4ac13 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.h
+++ b/drivers/ufs/host/ufshcd-pltfrm.h
@@ -31,7 +31,6 @@ int ufshcd_get_pwr_dev_param(const struct ufs_dev_params *dev_param,
 void ufshcd_init_pwr_dev_param(struct ufs_dev_params *dev_param);
 int ufshcd_pltfrm_init(struct platform_device *pdev,
 		       const struct ufs_hba_variant_ops *vops);
-void ufshcd_pltfrm_shutdown(struct platform_device *pdev);
 int ufshcd_populate_vreg(struct device *dev, const char *name,
 			 struct ufs_vreg **out_vreg);
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index f7553293ba98..db2e669985d5 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1277,7 +1277,6 @@ extern int ufshcd_system_freeze(struct device *dev);
 extern int ufshcd_system_thaw(struct device *dev);
 extern int ufshcd_system_restore(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 0/4] UFS host controller driver patches

[Bart Van Assche]

On 5/17/23 15:31, Bart Van Assche wrote:
> Please consider these four UFS host controller driver patches for the next
> merge window.

Also for this patch series, please help with reviewing this series.

Thanks,

Bart.

Re: [PATCH v2 2/4] scsi: ufs: Fix handling of lrbp->cmd

[Adrian Hunter]

On 18/05/23 01:31, Bart Van Assche wrote:
> ufshcd_queuecommand() may be called two times in a row for a SCSI
> command before it is completed. Hence make the following changes:
> - In the functions that submit a command, do not check the old value of
>   lrbp->cmd nor clear lrbp->cmd in error paths.
> - In ufshcd_release_scsi_cmd(), do not clear lrbp->cmd.
> 
> See also scsi_send_eh_cmnd().
> 
> This patch prevents that the following appears if a command times out:
> 
> WARNING: at drivers/ufs/core/ufshcd.c:2965 ufshcd_queuecommand+0x6f8/0x9a8
> Call trace:
>  ufshcd_queuecommand+0x6f8/0x9a8
>  scsi_send_eh_cmnd+0x2c0/0x960
>  scsi_eh_test_devices+0x100/0x314
>  scsi_eh_ready_devs+0xd90/0x114c
>  scsi_error_handler+0x2b4/0xb70
>  kthread+0x16c/0x1e0
> 
> Fixes: 5a0b0cb9bee7 ("[SCSI] ufs: Add support for sending NOP OUT UPIU")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

A couple of minor comments, nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/ufs/core/ufshcd.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 37337d411466..68d9e24fac98 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2928,7 +2928,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  		(hba->clk_gating.state != CLKS_ON));
>  
>  	lrbp = &hba->lrb[tag];
> -	WARN_ON(lrbp->cmd);

AFAICT eh uses the same struct i.e. lrbp->cmd => lrbp->cmd == cmd in that case

>  	lrbp->cmd = cmd;
>  	lrbp->task_tag = tag;
>  	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
> @@ -2944,7 +2943,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  
>  	err = ufshcd_map_sg(hba, lrbp);
>  	if (err) {
> -		lrbp->cmd = NULL;
>  		ufshcd_release(hba);
>  		goto out;
>  	}
> @@ -5405,7 +5403,6 @@ static void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
>  	struct scsi_cmnd *cmd = lrbp->cmd;
>  
>  	scsi_dma_unmap(cmd);
> -	lrbp->cmd = NULL;	/* Mark the command as completed. */
>  	ufshcd_release(hba);
>  	ufshcd_clk_scaling_update_busy(hba);
>  }
> @@ -7020,7 +7017,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>  	down_read(&hba->clk_scaling_lock);
>  
>  	lrbp = &hba->lrb[tag];
> -	WARN_ON(lrbp->cmd);
>  	lrbp->cmd = NULL;
>  	lrbp->task_tag = tag;
>  	lrbp->lun = 0;
> @@ -7192,7 +7188,6 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
>  	down_read(&hba->clk_scaling_lock);
>  
>  	lrbp = &hba->lrb[tag];
> -	WARN_ON(lrbp->cmd);
>  	lrbp->cmd = NULL;
>  	lrbp->task_tag = tag;
>  	lrbp->lun = UFS_UPIU_RPMB_WLUN;
> 

Currently the reserved tag is not used for SCSI cmds but
there is also a WARN_ON(lrbp->cmd) in ufshcd_exec_dev_cmd()

Re: [PATCH v2 3/4] scsi: ufs: Move ufshcd_wl_shutdown()

[Adrian Hunter]

On 18/05/23 01:31, Bart Van Assche wrote:
> Move the definition of ufshcd_wl_shutdown() to make the next patch in
> this series easier to review.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/ufs/core/ufshcd.c | 44 +++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 68d9e24fac98..0f426d46d91e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9746,28 +9746,6 @@ static int ufshcd_wl_resume(struct device *dev)
>  }
>  #endif
>  
> -static void ufshcd_wl_shutdown(struct device *dev)
> -{
> -	struct scsi_device *sdev = to_scsi_device(dev);
> -	struct ufs_hba *hba;
> -
> -	hba = shost_priv(sdev->host);
> -
> -	down(&hba->host_sem);
> -	hba->shutting_down = true;
> -	up(&hba->host_sem);
> -
> -	/* Turn on everything while shutting down */
> -	ufshcd_rpm_get_sync(hba);
> -	scsi_device_quiesce(sdev);
> -	shost_for_each_device(sdev, hba->host) {
> -		if (sdev == hba->ufs_device_wlun)
> -			continue;
> -		scsi_device_quiesce(sdev);
> -	}
> -	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> -}
> -
>  /**
>   * ufshcd_suspend - helper function for suspend operations
>   * @hba: per adapter instance
> @@ -9952,6 +9930,28 @@ int ufshcd_runtime_resume(struct device *dev)
>  EXPORT_SYMBOL(ufshcd_runtime_resume);
>  #endif /* CONFIG_PM */
>  
> +static void ufshcd_wl_shutdown(struct device *dev)
> +{
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	struct ufs_hba *hba;
> +
> +	hba = shost_priv(sdev->host);
> +
> +	down(&hba->host_sem);
> +	hba->shutting_down = true;
> +	up(&hba->host_sem);
> +
> +	/* Turn on everything while shutting down */
> +	ufshcd_rpm_get_sync(hba);
> +	scsi_device_quiesce(sdev);
> +	shost_for_each_device(sdev, hba->host) {
> +		if (sdev == hba->ufs_device_wlun)
> +			continue;
> +		scsi_device_quiesce(sdev);
> +	}
> +	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> +}
> +
>  /**
>   * ufshcd_shutdown - shutdown routine
>   * @hba: per adapter instance
> 

Re: [PATCH v2 4/4] scsi: ufs: Simplify driver shutdown

[Adrian Hunter]

On 18/05/23 01:31, Bart Van Assche wrote:
> All UFS host drivers call ufshcd_shutdown(). Hence, instead of calling
> ufshcd_shutdown() from the host driver .shutdown() callback, inline that
> function into ufshcd_wl_shutdown().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>


> ---
>  drivers/ufs/core/ufshcd.c             | 23 +++++------------------
>  drivers/ufs/host/cdns-pltfrm.c        |  1 -
>  drivers/ufs/host/tc-dwc-g210-pci.c    | 10 ----------
>  drivers/ufs/host/tc-dwc-g210-pltfrm.c |  1 -
>  drivers/ufs/host/ufs-exynos.c         |  1 -
>  drivers/ufs/host/ufs-hisi.c           |  1 -
>  drivers/ufs/host/ufs-mediatek.c       |  1 -
>  drivers/ufs/host/ufs-qcom.c           |  1 -
>  drivers/ufs/host/ufs-sprd.c           |  1 -
>  drivers/ufs/host/ufshcd-pci.c         | 10 ----------
>  drivers/ufs/host/ufshcd-pltfrm.c      |  6 ------
>  drivers/ufs/host/ufshcd-pltfrm.h      |  1 -
>  include/ufs/ufshcd.h                  |  1 -
>  13 files changed, 5 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0f426d46d91e..7ee150d67d49 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9933,9 +9933,7 @@ EXPORT_SYMBOL(ufshcd_runtime_resume);
>  static void ufshcd_wl_shutdown(struct device *dev)
>  {
>  	struct scsi_device *sdev = to_scsi_device(dev);
> -	struct ufs_hba *hba;
> -
> -	hba = shost_priv(sdev->host);
> +	struct ufs_hba *hba = shost_priv(sdev->host);
>  
>  	down(&hba->host_sem);
>  	hba->shutting_down = true;
> @@ -9950,27 +9948,16 @@ static void ufshcd_wl_shutdown(struct device *dev)
>  		scsi_device_quiesce(sdev);
>  	}
>  	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> -}
>  
> -/**
> - * ufshcd_shutdown - shutdown routine
> - * @hba: per adapter instance
> - *
> - * This function would turn off both UFS device and UFS hba
> - * regulators. It would also disable clocks.
> - *
> - * Returns 0 always to allow force shutdown even in case of errors.
> - */
> -int ufshcd_shutdown(struct ufs_hba *hba)
> -{
> +	/*
> +	 * Next, turn off the UFS controller and the UFS regulators. Disable
> +	 * clocks.
> +	 */
>  	if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
>  		ufshcd_suspend(hba);
>  
>  	hba->is_powered = false;
> -	/* allow force shutdown even in case of errors */
> -	return 0;
>  }
> -EXPORT_SYMBOL(ufshcd_shutdown);
>  
>  /**
>   * ufshcd_remove - de-allocate SCSI host and host memory space
> diff --git a/drivers/ufs/host/cdns-pltfrm.c b/drivers/ufs/host/cdns-pltfrm.c
> index e05c0ae64eea..26761425a76c 100644
> --- a/drivers/ufs/host/cdns-pltfrm.c
> +++ b/drivers/ufs/host/cdns-pltfrm.c
> @@ -328,7 +328,6 @@ static const struct dev_pm_ops cdns_ufs_dev_pm_ops = {
>  static struct platform_driver cdns_ufs_pltfrm_driver = {
>  	.probe	= cdns_ufs_pltfrm_probe,
>  	.remove	= cdns_ufs_pltfrm_remove,
> -	.shutdown = ufshcd_pltfrm_shutdown,
>  	.driver	= {
>  		.name   = "cdns-ufshcd",
>  		.pm     = &cdns_ufs_dev_pm_ops,
> diff --git a/drivers/ufs/host/tc-dwc-g210-pci.c b/drivers/ufs/host/tc-dwc-g210-pci.c
> index 92b8ad4b58fe..f96fe5855841 100644
> --- a/drivers/ufs/host/tc-dwc-g210-pci.c
> +++ b/drivers/ufs/host/tc-dwc-g210-pci.c
> @@ -32,15 +32,6 @@ static struct ufs_hba_variant_ops tc_dwc_g210_pci_hba_vops = {
>  	.link_startup_notify	= ufshcd_dwc_link_startup_notify,
>  };
>  
> -/**
> - * tc_dwc_g210_pci_shutdown - main function to put the controller in reset state
> - * @pdev: pointer to PCI device handle
> - */
> -static void tc_dwc_g210_pci_shutdown(struct pci_dev *pdev)
> -{
> -	ufshcd_shutdown((struct ufs_hba *)pci_get_drvdata(pdev));
> -}
> -
>  /**
>   * tc_dwc_g210_pci_remove - de-allocate PCI/SCSI host and host memory space
>   *		data structure memory
> @@ -137,7 +128,6 @@ static struct pci_driver tc_dwc_g210_pci_driver = {
>  	.id_table = tc_dwc_g210_pci_tbl,
>  	.probe = tc_dwc_g210_pci_probe,
>  	.remove = tc_dwc_g210_pci_remove,
> -	.shutdown = tc_dwc_g210_pci_shutdown,
>  	.driver = {
>  		.pm = &tc_dwc_g210_pci_pm_ops
>  	},
> diff --git a/drivers/ufs/host/tc-dwc-g210-pltfrm.c b/drivers/ufs/host/tc-dwc-g210-pltfrm.c
> index f15a84d0c176..4d5389dd9585 100644
> --- a/drivers/ufs/host/tc-dwc-g210-pltfrm.c
> +++ b/drivers/ufs/host/tc-dwc-g210-pltfrm.c
> @@ -92,7 +92,6 @@ static const struct dev_pm_ops tc_dwc_g210_pltfm_pm_ops = {
>  static struct platform_driver tc_dwc_g210_pltfm_driver = {
>  	.probe		= tc_dwc_g210_pltfm_probe,
>  	.remove		= tc_dwc_g210_pltfm_remove,
> -	.shutdown = ufshcd_pltfrm_shutdown,
>  	.driver		= {
>  		.name	= "tc-dwc-g210-pltfm",
>  		.pm	= &tc_dwc_g210_pltfm_pm_ops,
> diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
> index 0bf5390739e1..f41056f57fd7 100644
> --- a/drivers/ufs/host/ufs-exynos.c
> +++ b/drivers/ufs/host/ufs-exynos.c
> @@ -1757,7 +1757,6 @@ static const struct dev_pm_ops exynos_ufs_pm_ops = {
>  static struct platform_driver exynos_ufs_pltform = {
>  	.probe	= exynos_ufs_probe,
>  	.remove	= exynos_ufs_remove,
> -	.shutdown = ufshcd_pltfrm_shutdown,
>  	.driver	= {
>  		.name	= "exynos-ufshc",
>  		.pm	= &exynos_ufs_pm_ops,
> diff --git a/drivers/ufs/host/ufs-hisi.c b/drivers/ufs/host/ufs-hisi.c
> index 4c423eba8aa9..18b72e2e68c1 100644
> --- a/drivers/ufs/host/ufs-hisi.c
> +++ b/drivers/ufs/host/ufs-hisi.c
> @@ -593,7 +593,6 @@ static const struct dev_pm_ops ufs_hisi_pm_ops = {
>  static struct platform_driver ufs_hisi_pltform = {
>  	.probe	= ufs_hisi_probe,
>  	.remove	= ufs_hisi_remove,
> -	.shutdown = ufshcd_pltfrm_shutdown,
>  	.driver	= {
>  		.name	= "ufshcd-hisi",
>  		.pm	= &ufs_hisi_pm_ops,
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index a054810e321d..33b301649757 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1647,7 +1647,6 @@ static const struct dev_pm_ops ufs_mtk_pm_ops = {
>  static struct platform_driver ufs_mtk_pltform = {
>  	.probe      = ufs_mtk_probe,
>  	.remove     = ufs_mtk_remove,
> -	.shutdown   = ufshcd_pltfrm_shutdown,
>  	.driver = {
>  		.name   = "ufshcd-mtk",
>  		.pm     = &ufs_mtk_pm_ops,
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 82d02e7f3b4f..059de74dfea3 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1723,7 +1723,6 @@ static const struct dev_pm_ops ufs_qcom_pm_ops = {
>  static struct platform_driver ufs_qcom_pltform = {
>  	.probe	= ufs_qcom_probe,
>  	.remove	= ufs_qcom_remove,
> -	.shutdown = ufshcd_pltfrm_shutdown,
>  	.driver	= {
>  		.name	= "ufshcd-qcom",
>  		.pm	= &ufs_qcom_pm_ops,
> diff --git a/drivers/ufs/host/ufs-sprd.c b/drivers/ufs/host/ufs-sprd.c
> index 051f3f40d92c..2bad75dd6d58 100644
> --- a/drivers/ufs/host/ufs-sprd.c
> +++ b/drivers/ufs/host/ufs-sprd.c
> @@ -444,7 +444,6 @@ static const struct dev_pm_ops ufs_sprd_pm_ops = {
>  static struct platform_driver ufs_sprd_pltform = {
>  	.probe = ufs_sprd_probe,
>  	.remove = ufs_sprd_remove,
> -	.shutdown = ufshcd_pltfrm_shutdown,
>  	.driver = {
>  		.name = "ufshcd-sprd",
>  		.pm = &ufs_sprd_pm_ops,
> diff --git a/drivers/ufs/host/ufshcd-pci.c b/drivers/ufs/host/ufshcd-pci.c
> index 9c911787f84c..38276dac8e52 100644
> --- a/drivers/ufs/host/ufshcd-pci.c
> +++ b/drivers/ufs/host/ufshcd-pci.c
> @@ -504,15 +504,6 @@ static int ufshcd_pci_restore(struct device *dev)
>  }
>  #endif
>  
> -/**
> - * ufshcd_pci_shutdown - main function to put the controller in reset state
> - * @pdev: pointer to PCI device handle
> - */
> -static void ufshcd_pci_shutdown(struct pci_dev *pdev)
> -{
> -	ufshcd_shutdown((struct ufs_hba *)pci_get_drvdata(pdev));
> -}
> -
>  /**
>   * ufshcd_pci_remove - de-allocate PCI/SCSI host and host memory space
>   *		data structure memory
> @@ -618,7 +609,6 @@ static struct pci_driver ufshcd_pci_driver = {
>  	.id_table = ufshcd_pci_tbl,
>  	.probe = ufshcd_pci_probe,
>  	.remove = ufshcd_pci_remove,
> -	.shutdown = ufshcd_pci_shutdown,
>  	.driver = {
>  		.pm = &ufshcd_pci_pm_ops
>  	},
> diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
> index 5739ff007828..0b7430033047 100644
> --- a/drivers/ufs/host/ufshcd-pltfrm.c
> +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> @@ -190,12 +190,6 @@ static int ufshcd_parse_regulator_info(struct ufs_hba *hba)
>  	return err;
>  }
>  
> -void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
> -{
> -	ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
> -}
> -EXPORT_SYMBOL_GPL(ufshcd_pltfrm_shutdown);
> -
>  static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
>  {
>  	struct device *dev = hba->dev;
> diff --git a/drivers/ufs/host/ufshcd-pltfrm.h b/drivers/ufs/host/ufshcd-pltfrm.h
> index 2e4ba2bfbcad..2df108f4ac13 100644
> --- a/drivers/ufs/host/ufshcd-pltfrm.h
> +++ b/drivers/ufs/host/ufshcd-pltfrm.h
> @@ -31,7 +31,6 @@ int ufshcd_get_pwr_dev_param(const struct ufs_dev_params *dev_param,
>  void ufshcd_init_pwr_dev_param(struct ufs_dev_params *dev_param);
>  int ufshcd_pltfrm_init(struct platform_device *pdev,
>  		       const struct ufs_hba_variant_ops *vops);
> -void ufshcd_pltfrm_shutdown(struct platform_device *pdev);
>  int ufshcd_populate_vreg(struct device *dev, const char *name,
>  			 struct ufs_vreg **out_vreg);
>  
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index f7553293ba98..db2e669985d5 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1277,7 +1277,6 @@ extern int ufshcd_system_freeze(struct device *dev);
>  extern int ufshcd_system_thaw(struct device *dev);
>  extern int ufshcd_system_restore(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 2/4] scsi: ufs: Fix handling of lrbp->cmd

[Bart Van Assche]

On 5/24/23 05:21, Adrian Hunter wrote:
> Currently the reserved tag is not used for SCSI cmds but
> there is also a WARN_ON(lrbp->cmd) in ufshcd_exec_dev_cmd()

Hi Adrian,

Thanks for the review. I will change that WARN_ON() statement into 
"lrbp->cmd = NULL" to make ufshcd_exec_dev_cmd() consistent with the 
other functions that use the reserved slot.

Bart.