[PATCH 0/4] UFS host controller driver patches

[PATCH 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. Patches 1/4 and 2/4 have been posted before while the other two
patches are new.

Thanks,

Bart.

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             | 63 ++++++++++-----------------
 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, 22 insertions(+), 76 deletions(-)

[PATCH 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>
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 9434328ba323..0e2a0656858a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9182,7 +9182,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 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 0e2a0656858a..c691ddf09698 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2952,7 +2952,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);
@@ -2968,7 +2967,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;
 	}
@@ -5429,7 +5427,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);
 }
@@ -7044,7 +7041,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;
@@ -7216,7 +7212,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 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 c691ddf09698..215a7f713b46 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9761,28 +9761,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
@@ -9967,6 +9945,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 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             | 15 ---------------
 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, 50 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 215a7f713b46..7b1e7d7091ff 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9965,27 +9965,12 @@ 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)
-{
 	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 73e217260390..e89b625d3c5a 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1650,7 +1650,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 721ae4cd3702..d6da1efb0212 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1278,7 +1278,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 4/4] scsi: ufs: Simplify driver shutdown

[Adrian Hunter]

On 26/04/23 02:29, 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().

Pedantically, this seems like a bit of a layering violation: the child
is doing the parent's shutdown.  It assumes host shutdown is not needed
if ufs_device_wlun does not exist or did not successfully probe.

At least it could use a comment in the code.

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c             | 15 ---------------
>  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, 50 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 215a7f713b46..7b1e7d7091ff 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9965,27 +9965,12 @@ 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)
> -{
>  	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 73e217260390..e89b625d3c5a 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1650,7 +1650,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 721ae4cd3702..d6da1efb0212 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1278,7 +1278,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 1/4] scsi: ufs: Increase the START STOP UNIT timeout from one to ten seconds

[Bean Huo]

On Tue, 2023-04-25 at 16:29 -0700, Bart Van Assche wrote:
> 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>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

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

[Bean Huo]

On Tue, 2023-04-25 at 16:29 -0700, 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:
> 

Bart, 

lrbp->cmd will always be non-NULL after this slot in the queue has been
used once?

 
> 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 0e2a0656858a..c691ddf09698 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2952,7 +2952,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);
> @@ -2968,7 +2967,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;
>         }
> @@ -5429,7 +5427,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);
>  }
> @@ -7044,7 +7041,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;
> @@ -7216,7 +7212,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;

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

[Bart Van Assche]

On 4/26/23 05:48, Bean Huo wrote:
> lrbp->cmd will always be non-NULL after this slot in the queue has been
> used once?

Hi Bean,

The reserved slot is used for device commands, UPIU commands and also for
RPMB commands. The other slots are used for SCSI commands. So lrbp->cmd
could be set after the lrbp array has been allocated instead of when a
command is queued. I haven't done that because in the near future I would
like to remove the lrbp->cmd pointer. This is possible by setting the
.cmd_size member in the SCSI host template to sizeof(struct ufshcd_lrb).
Setting that member causes the SCSI core to allocate additional memory
at the end of each struct scsi_cmnd instance.

Thanks,

Bart.