[PATCH 1/2] scsi: ufs: retry START_STOP on UNIT_ATTENTION

[PATCH 1/2] scsi: ufs: retry START_STOP on UNIT_ATTENTION

[Jaegeuk Kim]

Commit 57d104c153d3 ("ufs: add UFS power management support") made the UFS
driver submit a REQUEST SENSE command before submitting a power management
command to a WLUN to clear the POWER ON unit attention. Instead of
submitting a REQUEST SENSE command before submitting a power management
command, retry the power management command until it succeeds.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 drivers/scsi/ufs/ufshcd.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3841ab49f556..1f21d371e231 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8630,7 +8630,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	struct scsi_sense_hdr sshdr;
 	struct scsi_device *sdp;
 	unsigned long flags;
-	int ret;
+	int ret, retries;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	sdp = hba->sdev_ufs_device;
@@ -8665,8 +8665,14 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	 * callbacks hence set the RQF_PM flag so that it doesn't resume the
 	 * already suspended childs.
 	 */
-	ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
-			START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
+	for (retries = 3; retries > 0; --retries) {
+		ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
+				START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
+		if (!scsi_status_is_check_condition(ret) ||
+				!scsi_sense_valid(&sshdr) ||
+				sshdr.sense_key != UNIT_ATTENTION)
+			break;
+	}
 	if (ret) {
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",
-- 
2.33.0.800.g4c38ced690-goog

[PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Jaegeuk Kim]

From: Bart Van Assche <bvanassche@google.com>

Commit aa53f580e67b ("scsi: ufs: Minor adjustments to error handling")
introduced a ufshcd_clear_ua_wluns() call in
ufshcd_err_handling_unprepare(). As explained in detail by Adrian Hunter,
this can trigger a deadlock. Avoid that deadlock by removing the code that
clears the unit attention. This is safe because the only software that
relies on clearing unit attentions is the Android Trusty software and
because support for handling unit attentions has been added in the Trusty software.

See also https://lore.kernel.org/linux-scsi/20210930124224.114031-2-adrian.hunter@intel.com/

Cc: Adrian Hunter <adrian.hunter@intel.com>
Fixes: aa53f580e67b ("scsi: ufs: Minor adjustments to error handling")
Signed-off-by: Bart Van Assche <bvanassche@google.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 drivers/scsi/ufs/ufshcd.c | 176 +-------------------------------------
 drivers/scsi/ufs/ufshcd.h |   3 -
 2 files changed, 1 insertion(+), 178 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1f21d371e231..4add5e990de9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -224,7 +224,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
 static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
 static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
 static void ufshcd_hba_exit(struct ufs_hba *hba);
-static int ufshcd_clear_ua_wluns(struct ufs_hba *hba);
 static int ufshcd_probe_hba(struct ufs_hba *hba, bool async);
 static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
 static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
@@ -4109,8 +4108,6 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
 	if (ret)
 		dev_err(hba->dev, "%s: link recovery failed, err %d",
 			__func__, ret);
-	else
-		ufshcd_clear_ua_wluns(hba);
 
 	return ret;
 }
@@ -5974,7 +5971,6 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 	ufshcd_release(hba);
 	if (ufshcd_is_clkscaling_supported(hba))
 		ufshcd_clk_scaling_suspend(hba, false);
-	ufshcd_clear_ua_wluns(hba);
 	ufshcd_rpm_put(hba);
 }
 
@@ -7907,8 +7903,6 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 	if (ret)
 		goto out;
 
-	ufshcd_clear_ua_wluns(hba);
-
 	/* Initialize devfreq after UFS device is detected */
 	if (ufshcd_is_clkscaling_supported(hba)) {
 		memcpy(&hba->clk_scaling.saved_pwr_info.info,
@@ -7934,116 +7928,6 @@ 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)\n", __func__, error);
-	kfree(rq->end_io_data);
-	blk_put_request(rq);
-}
-
-static int
-ufshcd_request_sense_async(struct ufs_hba *hba, struct scsi_device *sdev)
-{
-	/*
-	 * Some UFS devices clear unit attention condition only if the sense
-	 * size used (UFS_SENSE_SIZE in this case) is non-zero.
-	 */
-	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, UFS_SENSE_SIZE, 0};
-	struct scsi_request *rq;
-	struct request *req;
-	char *buffer;
-	int ret;
-
-	buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
-	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN,
-			      /*flags=*/BLK_MQ_REQ_PM);
-	if (IS_ERR(req)) {
-		ret = PTR_ERR(req);
-		goto out_free;
-	}
-
-	ret = blk_rq_map_kern(sdev->request_queue, req,
-			      buffer, UFS_SENSE_SIZE, GFP_NOIO);
-	if (ret)
-		goto out_put;
-
-	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;
-	req->end_io_data = buffer;
-
-	blk_execute_rq_nowait(/*bd_disk=*/NULL, req, /*at_head=*/true,
-			      ufshcd_request_sense_done);
-	return 0;
-
-out_put:
-	blk_put_request(req);
-out_free:
-	kfree(buffer);
-	return ret;
-}
-
-static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
-{
-	struct scsi_device *sdp;
-	unsigned long flags;
-	int ret = 0;
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (wlun == UFS_UPIU_UFS_DEVICE_WLUN)
-		sdp = hba->sdev_ufs_device;
-	else if (wlun == UFS_UPIU_RPMB_WLUN)
-		sdp = hba->sdev_rpmb;
-	else
-		BUG();
-	if (sdp) {
-		ret = scsi_device_get(sdp);
-		if (!ret && !scsi_device_online(sdp)) {
-			ret = -ENODEV;
-			scsi_device_put(sdp);
-		}
-	} else {
-		ret = -ENODEV;
-	}
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	if (ret)
-		goto out_err;
-
-	ret = ufshcd_request_sense_async(hba, sdp);
-	scsi_device_put(sdp);
-out_err:
-	if (ret)
-		dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
-				__func__, wlun, ret);
-	return ret;
-}
-
-static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
-{
-	int ret = 0;
-
-	if (!hba->wlun_dev_clr_ua)
-		goto out;
-
-	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
-	if (!ret)
-		ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
-	if (!ret)
-		hba->wlun_dev_clr_ua = false;
-out:
-	if (ret)
-		dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
-				__func__, ret);
-	return ret;
-}
-
 /**
  * ufshcd_probe_hba - probe hba to detect device and initialize it
  * @hba: per-adapter instance
@@ -8094,8 +7978,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 	/* UFS device is also active now */
 	ufshcd_set_ufs_dev_active(hba);
 	ufshcd_force_reset_auto_bkops(hba);
-	hba->wlun_dev_clr_ua = true;
-	hba->wlun_rpmb_clr_ua = true;
 
 	/* Gear up to HS gear if supported */
 	if (hba->max_pwr_info.is_valid) {
@@ -8655,8 +8537,6 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	 * handling context.
 	 */
 	hba->host->eh_noresume = 1;
-	if (hba->wlun_dev_clr_ua)
-		ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
 
 	cmd[4] = pwr_mode << 4;
 
@@ -9819,49 +9699,6 @@ static struct scsi_driver ufs_dev_wlun_template = {
 	},
 };
 
-static int ufshcd_rpmb_probe(struct device *dev)
-{
-	return is_rpmb_wlun(to_scsi_device(dev)) ? 0 : -ENODEV;
-}
-
-static inline int ufshcd_clear_rpmb_uac(struct ufs_hba *hba)
-{
-	int ret = 0;
-
-	if (!hba->wlun_rpmb_clr_ua)
-		return 0;
-	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
-	if (!ret)
-		hba->wlun_rpmb_clr_ua = 0;
-	return ret;
-}
-
-#ifdef CONFIG_PM
-static int ufshcd_rpmb_resume(struct device *dev)
-{
-	struct ufs_hba *hba = wlun_dev_to_hba(dev);
-
-	if (hba->sdev_rpmb)
-		ufshcd_clear_rpmb_uac(hba);
-	return 0;
-}
-#endif
-
-static const struct dev_pm_ops ufs_rpmb_pm_ops = {
-	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_resume, NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
-};
-
-/* ufs_rpmb_wlun_template - Describes UFS RPMB WLUN. Used only to send UAC. */
-static struct scsi_driver ufs_rpmb_wlun_template = {
-	.gendrv = {
-		.name = "ufs_rpmb_wlun",
-		.owner = THIS_MODULE,
-		.probe = ufshcd_rpmb_probe,
-		.pm = &ufs_rpmb_pm_ops,
-	},
-};
-
 static int __init ufshcd_core_init(void)
 {
 	int ret;
@@ -9870,24 +9707,13 @@ static int __init ufshcd_core_init(void)
 
 	ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
 	if (ret)
-		goto debugfs_exit;
-
-	ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
-	if (ret)
-		goto unregister;
-
-	return ret;
-unregister:
-	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
-debugfs_exit:
-	ufs_debugfs_exit();
+		ufs_debugfs_exit();
 	return ret;
 }
 
 static void __exit ufshcd_core_exit(void)
 {
 	ufs_debugfs_exit();
-	scsi_unregister_driver(&ufs_rpmb_wlun_template.gendrv);
 	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
 }
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 52ea6f350b18..b414491a8240 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -865,9 +865,6 @@ struct ufs_hba {
 	struct ufs_vreg_info vreg_info;
 	struct list_head clk_list_head;
 
-	bool wlun_dev_clr_ua;
-	bool wlun_rpmb_clr_ua;
-
 	/* Number of requests aborts */
 	int req_abort_count;
 
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Adrian Hunter]

On 30/09/2021 22:52, Jaegeuk Kim wrote:
> From: Bart Van Assche <bvanassche@google.com>
> 
> Commit aa53f580e67b ("scsi: ufs: Minor adjustments to error handling")
> introduced a ufshcd_clear_ua_wluns() call in
> ufshcd_err_handling_unprepare(). As explained in detail by Adrian Hunter,
> this can trigger a deadlock. Avoid that deadlock by removing the code that
> clears the unit attention. This is safe because the only software that
> relies on clearing unit attentions is the Android Trusty software and

Did you test this? Because AFAIK it won't work for the UFS device WLUN.

UAC must also be cleared for the UFS device WLUN otherwise there will
be an error in ufshcd_set_dev_pwr_mode().

> because support for handling unit attentions has been added in the Trusty software.
> 
> See also https://lore.kernel.org/linux-scsi/20210930124224.114031-2-adrian.hunter@intel.com/
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Fixes: aa53f580e67b ("scsi: ufs: Minor adjustments to error handling")
> Signed-off-by: Bart Van Assche <bvanassche@google.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 176 +-------------------------------------
>  drivers/scsi/ufs/ufshcd.h |   3 -
>  2 files changed, 1 insertion(+), 178 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1f21d371e231..4add5e990de9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -224,7 +224,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
>  static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
>  static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
>  static void ufshcd_hba_exit(struct ufs_hba *hba);
> -static int ufshcd_clear_ua_wluns(struct ufs_hba *hba);
>  static int ufshcd_probe_hba(struct ufs_hba *hba, bool async);
>  static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
>  static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
> @@ -4109,8 +4108,6 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
>  	if (ret)
>  		dev_err(hba->dev, "%s: link recovery failed, err %d",
>  			__func__, ret);
> -	else
> -		ufshcd_clear_ua_wluns(hba);
>  
>  	return ret;
>  }
> @@ -5974,7 +5971,6 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>  	ufshcd_release(hba);
>  	if (ufshcd_is_clkscaling_supported(hba))
>  		ufshcd_clk_scaling_suspend(hba, false);
> -	ufshcd_clear_ua_wluns(hba);
>  	ufshcd_rpm_put(hba);
>  }
>  
> @@ -7907,8 +7903,6 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>  	if (ret)
>  		goto out;
>  
> -	ufshcd_clear_ua_wluns(hba);
> -
>  	/* Initialize devfreq after UFS device is detected */
>  	if (ufshcd_is_clkscaling_supported(hba)) {
>  		memcpy(&hba->clk_scaling.saved_pwr_info.info,
> @@ -7934,116 +7928,6 @@ 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)\n", __func__, error);
> -	kfree(rq->end_io_data);
> -	blk_put_request(rq);
> -}
> -
> -static int
> -ufshcd_request_sense_async(struct ufs_hba *hba, struct scsi_device *sdev)
> -{
> -	/*
> -	 * Some UFS devices clear unit attention condition only if the sense
> -	 * size used (UFS_SENSE_SIZE in this case) is non-zero.
> -	 */
> -	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, UFS_SENSE_SIZE, 0};
> -	struct scsi_request *rq;
> -	struct request *req;
> -	char *buffer;
> -	int ret;
> -
> -	buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN,
> -			      /*flags=*/BLK_MQ_REQ_PM);
> -	if (IS_ERR(req)) {
> -		ret = PTR_ERR(req);
> -		goto out_free;
> -	}
> -
> -	ret = blk_rq_map_kern(sdev->request_queue, req,
> -			      buffer, UFS_SENSE_SIZE, GFP_NOIO);
> -	if (ret)
> -		goto out_put;
> -
> -	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;
> -	req->end_io_data = buffer;
> -
> -	blk_execute_rq_nowait(/*bd_disk=*/NULL, req, /*at_head=*/true,
> -			      ufshcd_request_sense_done);
> -	return 0;
> -
> -out_put:
> -	blk_put_request(req);
> -out_free:
> -	kfree(buffer);
> -	return ret;
> -}
> -
> -static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
> -{
> -	struct scsi_device *sdp;
> -	unsigned long flags;
> -	int ret = 0;
> -
> -	spin_lock_irqsave(hba->host->host_lock, flags);
> -	if (wlun == UFS_UPIU_UFS_DEVICE_WLUN)
> -		sdp = hba->sdev_ufs_device;
> -	else if (wlun == UFS_UPIU_RPMB_WLUN)
> -		sdp = hba->sdev_rpmb;
> -	else
> -		BUG();
> -	if (sdp) {
> -		ret = scsi_device_get(sdp);
> -		if (!ret && !scsi_device_online(sdp)) {
> -			ret = -ENODEV;
> -			scsi_device_put(sdp);
> -		}
> -	} else {
> -		ret = -ENODEV;
> -	}
> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
> -	if (ret)
> -		goto out_err;
> -
> -	ret = ufshcd_request_sense_async(hba, sdp);
> -	scsi_device_put(sdp);
> -out_err:
> -	if (ret)
> -		dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
> -				__func__, wlun, ret);
> -	return ret;
> -}
> -
> -static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
> -{
> -	int ret = 0;
> -
> -	if (!hba->wlun_dev_clr_ua)
> -		goto out;
> -
> -	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
> -	if (!ret)
> -		ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> -	if (!ret)
> -		hba->wlun_dev_clr_ua = false;
> -out:
> -	if (ret)
> -		dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
> -				__func__, ret);
> -	return ret;
> -}
> -
>  /**
>   * ufshcd_probe_hba - probe hba to detect device and initialize it
>   * @hba: per-adapter instance
> @@ -8094,8 +7978,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>  	/* UFS device is also active now */
>  	ufshcd_set_ufs_dev_active(hba);
>  	ufshcd_force_reset_auto_bkops(hba);
> -	hba->wlun_dev_clr_ua = true;
> -	hba->wlun_rpmb_clr_ua = true;
>  
>  	/* Gear up to HS gear if supported */
>  	if (hba->max_pwr_info.is_valid) {
> @@ -8655,8 +8537,6 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  	 * handling context.
>  	 */
>  	hba->host->eh_noresume = 1;
> -	if (hba->wlun_dev_clr_ua)
> -		ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
>  
>  	cmd[4] = pwr_mode << 4;
>  
> @@ -9819,49 +9699,6 @@ static struct scsi_driver ufs_dev_wlun_template = {
>  	},
>  };
>  
> -static int ufshcd_rpmb_probe(struct device *dev)
> -{
> -	return is_rpmb_wlun(to_scsi_device(dev)) ? 0 : -ENODEV;
> -}
> -
> -static inline int ufshcd_clear_rpmb_uac(struct ufs_hba *hba)
> -{
> -	int ret = 0;
> -
> -	if (!hba->wlun_rpmb_clr_ua)
> -		return 0;
> -	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
> -	if (!ret)
> -		hba->wlun_rpmb_clr_ua = 0;
> -	return ret;
> -}
> -
> -#ifdef CONFIG_PM
> -static int ufshcd_rpmb_resume(struct device *dev)
> -{
> -	struct ufs_hba *hba = wlun_dev_to_hba(dev);
> -
> -	if (hba->sdev_rpmb)
> -		ufshcd_clear_rpmb_uac(hba);
> -	return 0;
> -}
> -#endif
> -
> -static const struct dev_pm_ops ufs_rpmb_pm_ops = {
> -	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_resume, NULL)
> -	SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
> -};
> -
> -/* ufs_rpmb_wlun_template - Describes UFS RPMB WLUN. Used only to send UAC. */
> -static struct scsi_driver ufs_rpmb_wlun_template = {
> -	.gendrv = {
> -		.name = "ufs_rpmb_wlun",
> -		.owner = THIS_MODULE,
> -		.probe = ufshcd_rpmb_probe,
> -		.pm = &ufs_rpmb_pm_ops,
> -	},
> -};
> -
>  static int __init ufshcd_core_init(void)
>  {
>  	int ret;
> @@ -9870,24 +9707,13 @@ static int __init ufshcd_core_init(void)
>  
>  	ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
>  	if (ret)
> -		goto debugfs_exit;
> -
> -	ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
> -	if (ret)
> -		goto unregister;
> -
> -	return ret;
> -unregister:
> -	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
> -debugfs_exit:
> -	ufs_debugfs_exit();
> +		ufs_debugfs_exit();
>  	return ret;
>  }
>  
>  static void __exit ufshcd_core_exit(void)
>  {
>  	ufs_debugfs_exit();
> -	scsi_unregister_driver(&ufs_rpmb_wlun_template.gendrv);
>  	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
>  }
>  
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 52ea6f350b18..b414491a8240 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -865,9 +865,6 @@ struct ufs_hba {
>  	struct ufs_vreg_info vreg_info;
>  	struct list_head clk_list_head;
>  
> -	bool wlun_dev_clr_ua;
> -	bool wlun_rpmb_clr_ua;
> -
>  	/* Number of requests aborts */
>  	int req_abort_count;
>  
> 

Re: [PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Adrian Hunter]

On 01/10/2021 07:58, Adrian Hunter wrote:
> On 30/09/2021 22:52, Jaegeuk Kim wrote:
>> From: Bart Van Assche <bvanassche@google.com>
>>
>> Commit aa53f580e67b ("scsi: ufs: Minor adjustments to error handling")
>> introduced a ufshcd_clear_ua_wluns() call in
>> ufshcd_err_handling_unprepare(). As explained in detail by Adrian Hunter,
>> this can trigger a deadlock. Avoid that deadlock by removing the code that
>> clears the unit attention. This is safe because the only software that
>> relies on clearing unit attentions is the Android Trusty software and
> 
> Did you test this? Because AFAIK it won't work for the UFS device WLUN.
> 
> UAC must also be cleared for the UFS device WLUN otherwise there will
> be an error in ufshcd_set_dev_pwr_mode().

Ok, I see now you took care of that in patch 1.  That's cool, but you
you didn't cc me on patch 1.  I think people have raised the issue
before of being cc'ed on only a part of a patchset - don't know what
the conclusion was, but in this case it was not ideal.

I would also suggest cc'ing more UFS driver contributors.

In any case, I suggest amending the commit message of patch 1 to say
why the change is being made, and also in this patch, add to the
commit message that it depends on the "retry START_STOP on UNIT_ATTENTION"
patch.

Finally, there is another thing to change.  The reason
ufshcd_suspend_prepare() does a runtime resume of sdev_rpmb is because the
UAC clear would wait for an async runtime resume, which will never happen
during system suspend because the PM workqueue gets frozen.  So with the
removal of UAC clear, ufshcd_suspend_prepare() and ufshcd_resume_complete()
should be updated also, to leave rpmb alone.


> 
>> because support for handling unit attentions has been added in the Trusty software.
>>
>> See also https://lore.kernel.org/linux-scsi/20210930124224.114031-2-adrian.hunter@intel.com/
>>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Fixes: aa53f580e67b ("scsi: ufs: Minor adjustments to error handling")
>> Signed-off-by: Bart Van Assche <bvanassche@google.com>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 176 +-------------------------------------
>>  drivers/scsi/ufs/ufshcd.h |   3 -
>>  2 files changed, 1 insertion(+), 178 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 1f21d371e231..4add5e990de9 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -224,7 +224,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
>>  static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
>>  static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
>>  static void ufshcd_hba_exit(struct ufs_hba *hba);
>> -static int ufshcd_clear_ua_wluns(struct ufs_hba *hba);
>>  static int ufshcd_probe_hba(struct ufs_hba *hba, bool async);
>>  static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
>>  static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
>> @@ -4109,8 +4108,6 @@ int ufshcd_link_recovery(struct ufs_hba *hba)
>>  	if (ret)
>>  		dev_err(hba->dev, "%s: link recovery failed, err %d",
>>  			__func__, ret);
>> -	else
>> -		ufshcd_clear_ua_wluns(hba);
>>  
>>  	return ret;
>>  }
>> @@ -5974,7 +5971,6 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>>  	ufshcd_release(hba);
>>  	if (ufshcd_is_clkscaling_supported(hba))
>>  		ufshcd_clk_scaling_suspend(hba, false);
>> -	ufshcd_clear_ua_wluns(hba);
>>  	ufshcd_rpm_put(hba);
>>  }
>>  
>> @@ -7907,8 +7903,6 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>>  	if (ret)
>>  		goto out;
>>  
>> -	ufshcd_clear_ua_wluns(hba);
>> -
>>  	/* Initialize devfreq after UFS device is detected */
>>  	if (ufshcd_is_clkscaling_supported(hba)) {
>>  		memcpy(&hba->clk_scaling.saved_pwr_info.info,
>> @@ -7934,116 +7928,6 @@ 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)\n", __func__, error);
>> -	kfree(rq->end_io_data);
>> -	blk_put_request(rq);
>> -}
>> -
>> -static int
>> -ufshcd_request_sense_async(struct ufs_hba *hba, struct scsi_device *sdev)
>> -{
>> -	/*
>> -	 * Some UFS devices clear unit attention condition only if the sense
>> -	 * size used (UFS_SENSE_SIZE in this case) is non-zero.
>> -	 */
>> -	static const u8 cmd[6] = {REQUEST_SENSE, 0, 0, 0, UFS_SENSE_SIZE, 0};
>> -	struct scsi_request *rq;
>> -	struct request *req;
>> -	char *buffer;
>> -	int ret;
>> -
>> -	buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
>> -	if (!buffer)
>> -		return -ENOMEM;
>> -
>> -	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN,
>> -			      /*flags=*/BLK_MQ_REQ_PM);
>> -	if (IS_ERR(req)) {
>> -		ret = PTR_ERR(req);
>> -		goto out_free;
>> -	}
>> -
>> -	ret = blk_rq_map_kern(sdev->request_queue, req,
>> -			      buffer, UFS_SENSE_SIZE, GFP_NOIO);
>> -	if (ret)
>> -		goto out_put;
>> -
>> -	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;
>> -	req->end_io_data = buffer;
>> -
>> -	blk_execute_rq_nowait(/*bd_disk=*/NULL, req, /*at_head=*/true,
>> -			      ufshcd_request_sense_done);
>> -	return 0;
>> -
>> -out_put:
>> -	blk_put_request(req);
>> -out_free:
>> -	kfree(buffer);
>> -	return ret;
>> -}
>> -
>> -static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
>> -{
>> -	struct scsi_device *sdp;
>> -	unsigned long flags;
>> -	int ret = 0;
>> -
>> -	spin_lock_irqsave(hba->host->host_lock, flags);
>> -	if (wlun == UFS_UPIU_UFS_DEVICE_WLUN)
>> -		sdp = hba->sdev_ufs_device;
>> -	else if (wlun == UFS_UPIU_RPMB_WLUN)
>> -		sdp = hba->sdev_rpmb;
>> -	else
>> -		BUG();
>> -	if (sdp) {
>> -		ret = scsi_device_get(sdp);
>> -		if (!ret && !scsi_device_online(sdp)) {
>> -			ret = -ENODEV;
>> -			scsi_device_put(sdp);
>> -		}
>> -	} else {
>> -		ret = -ENODEV;
>> -	}
>> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
>> -	if (ret)
>> -		goto out_err;
>> -
>> -	ret = ufshcd_request_sense_async(hba, sdp);
>> -	scsi_device_put(sdp);
>> -out_err:
>> -	if (ret)
>> -		dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
>> -				__func__, wlun, ret);
>> -	return ret;
>> -}
>> -
>> -static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
>> -{
>> -	int ret = 0;
>> -
>> -	if (!hba->wlun_dev_clr_ua)
>> -		goto out;
>> -
>> -	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
>> -	if (!ret)
>> -		ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
>> -	if (!ret)
>> -		hba->wlun_dev_clr_ua = false;
>> -out:
>> -	if (ret)
>> -		dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
>> -				__func__, ret);
>> -	return ret;
>> -}
>> -
>>  /**
>>   * ufshcd_probe_hba - probe hba to detect device and initialize it
>>   * @hba: per-adapter instance
>> @@ -8094,8 +7978,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>>  	/* UFS device is also active now */
>>  	ufshcd_set_ufs_dev_active(hba);
>>  	ufshcd_force_reset_auto_bkops(hba);
>> -	hba->wlun_dev_clr_ua = true;
>> -	hba->wlun_rpmb_clr_ua = true;
>>  
>>  	/* Gear up to HS gear if supported */
>>  	if (hba->max_pwr_info.is_valid) {
>> @@ -8655,8 +8537,6 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>>  	 * handling context.
>>  	 */
>>  	hba->host->eh_noresume = 1;
>> -	if (hba->wlun_dev_clr_ua)
>> -		ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
>>  
>>  	cmd[4] = pwr_mode << 4;
>>  
>> @@ -9819,49 +9699,6 @@ static struct scsi_driver ufs_dev_wlun_template = {
>>  	},
>>  };
>>  
>> -static int ufshcd_rpmb_probe(struct device *dev)
>> -{
>> -	return is_rpmb_wlun(to_scsi_device(dev)) ? 0 : -ENODEV;
>> -}
>> -
>> -static inline int ufshcd_clear_rpmb_uac(struct ufs_hba *hba)
>> -{
>> -	int ret = 0;
>> -
>> -	if (!hba->wlun_rpmb_clr_ua)
>> -		return 0;
>> -	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
>> -	if (!ret)
>> -		hba->wlun_rpmb_clr_ua = 0;
>> -	return ret;
>> -}
>> -
>> -#ifdef CONFIG_PM
>> -static int ufshcd_rpmb_resume(struct device *dev)
>> -{
>> -	struct ufs_hba *hba = wlun_dev_to_hba(dev);
>> -
>> -	if (hba->sdev_rpmb)
>> -		ufshcd_clear_rpmb_uac(hba);
>> -	return 0;
>> -}
>> -#endif
>> -
>> -static const struct dev_pm_ops ufs_rpmb_pm_ops = {
>> -	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_resume, NULL)
>> -	SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
>> -};
>> -
>> -/* ufs_rpmb_wlun_template - Describes UFS RPMB WLUN. Used only to send UAC. */
>> -static struct scsi_driver ufs_rpmb_wlun_template = {
>> -	.gendrv = {
>> -		.name = "ufs_rpmb_wlun",
>> -		.owner = THIS_MODULE,
>> -		.probe = ufshcd_rpmb_probe,
>> -		.pm = &ufs_rpmb_pm_ops,
>> -	},
>> -};
>> -
>>  static int __init ufshcd_core_init(void)
>>  {
>>  	int ret;
>> @@ -9870,24 +9707,13 @@ static int __init ufshcd_core_init(void)
>>  
>>  	ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
>>  	if (ret)
>> -		goto debugfs_exit;
>> -
>> -	ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
>> -	if (ret)
>> -		goto unregister;
>> -
>> -	return ret;
>> -unregister:
>> -	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
>> -debugfs_exit:
>> -	ufs_debugfs_exit();
>> +		ufs_debugfs_exit();
>>  	return ret;
>>  }
>>  
>>  static void __exit ufshcd_core_exit(void)
>>  {
>>  	ufs_debugfs_exit();
>> -	scsi_unregister_driver(&ufs_rpmb_wlun_template.gendrv);
>>  	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
>>  }
>>  
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 52ea6f350b18..b414491a8240 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -865,9 +865,6 @@ struct ufs_hba {
>>  	struct ufs_vreg_info vreg_info;
>>  	struct list_head clk_list_head;
>>  
>> -	bool wlun_dev_clr_ua;
>> -	bool wlun_rpmb_clr_ua;
>> -
>>  	/* Number of requests aborts */
>>  	int req_abort_count;
>>  
>>
> 

Re: [PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Bart Van Assche]

On 9/30/21 11:52 PM, Adrian Hunter wrote:
> Finally, there is another thing to change.  The reason
> ufshcd_suspend_prepare() does a runtime resume of sdev_rpmb is because the
> UAC clear would wait for an async runtime resume, which will never happen
> during system suspend because the PM workqueue gets frozen.  So with the
> removal of UAC clear, ufshcd_suspend_prepare() and ufshcd_resume_complete()
> should be updated also, to leave rpmb alone.

Is the following change what you have in mind?


diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0a28cc4c09d8..0743f54e55f9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9648,10 +9648,6 @@ void ufshcd_resume_complete(struct device *dev)
  		ufshcd_rpm_put(hba);
  		hba->complete_put = false;
  	}
-	if (hba->rpmb_complete_put) {
-		ufshcd_rpmb_rpm_put(hba);
-		hba->rpmb_complete_put = false;
-	}
  }
  EXPORT_SYMBOL_GPL(ufshcd_resume_complete);

@@ -9674,10 +9670,6 @@ int ufshcd_suspend_prepare(struct device *dev)
  		}
  		hba->complete_put = true;
  	}
-	if (hba->sdev_rpmb) {
-		ufshcd_rpmb_rpm_get_sync(hba);
-		hba->rpmb_complete_put = true;
-	}
  	return 0;
  }
  EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 86b615023ecb..5ecfcd8cae0a 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -921,7 +921,6 @@ struct ufs_hba {
  #endif
  	u32 luns_avail;
  	bool complete_put;
-	bool rpmb_complete_put;
  };

  /* Returns true if clocks can be gated. Otherwise false */



Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Douglas Gilbert]

On 2021-10-01 12:59 p.m., Bart Van Assche wrote:
> On 9/30/21 11:52 PM, Adrian Hunter wrote:
>> Finally, there is another thing to change.  The reason
>> ufshcd_suspend_prepare() does a runtime resume of sdev_rpmb is because the
>> UAC clear would wait for an async runtime resume, which will never happen
>> during system suspend because the PM workqueue gets frozen.  So with the
>> removal of UAC clear, ufshcd_suspend_prepare() and ufshcd_resume_complete()
>> should be updated also, to leave rpmb alone.

Somewhat related ...

Since there was some confusion among the members of T10 of what precisely
the RPM bit meant, in SPC-6 revision (draft), a new "HOT PLUGGABLE" two
bit field was introduced into the standard INQUIRY response:

                 Table 151 — HOT PLUGGABLE field

Code   Description
00b    No information is provided regarding whether SCSI target device is hot
        pluggable.
01b    The SCSI target device is designed to be removed from a SCSI domain as
        a single object (i.e., concurrent removal of the SCSI target ports,
        logical units, and all other objects contained in that SCSI target
        device (see SAM-6)) while that SCSI domain continues to operate for
        all other SCSI target devices, if any, in that SCSI domain.
10b    The SCSI target device is not designed to be removed from a SCSI
        domain while that SCSI domain continues to operate.
11b    Reserved

That field is bits 5 and 4 of byte 1 of the response.

Perhaps we should be adding provision for this new field.

Doug Gilbert

> 
> Is the following change what you have in mind?
> 
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0a28cc4c09d8..0743f54e55f9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -9648,10 +9648,6 @@ void ufshcd_resume_complete(struct device *dev)
>           ufshcd_rpm_put(hba);
>           hba->complete_put = false;
>       }
> -    if (hba->rpmb_complete_put) {
> -        ufshcd_rpmb_rpm_put(hba);
> -        hba->rpmb_complete_put = false;
> -    }
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
> 
> @@ -9674,10 +9670,6 @@ int ufshcd_suspend_prepare(struct device *dev)
>           }
>           hba->complete_put = true;
>       }
> -    if (hba->sdev_rpmb) {
> -        ufshcd_rpmb_rpm_get_sync(hba);
> -        hba->rpmb_complete_put = true;
> -    }
>       return 0;
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 86b615023ecb..5ecfcd8cae0a 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -921,7 +921,6 @@ struct ufs_hba {
>   #endif
>       u32 luns_avail;
>       bool complete_put;
> -    bool rpmb_complete_put;
>   };
> 
>   /* Returns true if clocks can be gated. Otherwise false */
> 
> 
> 
> Thanks,
> 
> Bart.

Re: [PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Bart Van Assche]

On 10/1/21 10:21 AM, Douglas Gilbert wrote:
> On 2021-10-01 12:59 p.m., Bart Van Assche wrote:
>> On 9/30/21 11:52 PM, Adrian Hunter wrote:
>>> Finally, there is another thing to change.  The reason
>>> ufshcd_suspend_prepare() does a runtime resume of sdev_rpmb is because the
>>> UAC clear would wait for an async runtime resume, which will never happen
>>> during system suspend because the PM workqueue gets frozen.  So with the
>>> removal of UAC clear, ufshcd_suspend_prepare() and ufshcd_resume_complete()
>>> should be updated also, to leave rpmb alone.
> 
> Somewhat related ...
> 
> Since there was some confusion among the members of T10 of what precisely
> the RPM bit meant, in SPC-6 revision (draft), a new "HOT PLUGGABLE" two
> bit field was introduced into the standard INQUIRY response:
> 
>                  Table 151 — HOT PLUGGABLE field
> 
> Code   Description
> 00b    No information is provided regarding whether SCSI target device is hot
>         pluggable.
> 01b    The SCSI target device is designed to be removed from a SCSI domain as
>         a single object (i.e., concurrent removal of the SCSI target ports,
>         logical units, and all other objects contained in that SCSI target
>         device (see SAM-6)) while that SCSI domain continues to operate for
>         all other SCSI target devices, if any, in that SCSI domain.
> 10b    The SCSI target device is not designed to be removed from a SCSI
>         domain while that SCSI domain continues to operate.
> 11b    Reserved
> 
> That field is bits 5 and 4 of byte 1 of the response.
> 
> Perhaps we should be adding provision for this new field.

Hi Doug,

It is not clear to me how hot-plugging is related to UFS devices? I am not aware
of any support for hot-plugging in the UFS driver. RPMB = Replay Protected Memory
Block. The definition of RPMB according to Wikipedia is "a means for a system to
store data to the specific memory area in an authenticated and replay protected
manner, and can only be read and written via successfully authenticated read and
write accesses". It is not clear to me how hot-plugging and RPMB are related? What
am I missing?

Thanks,

Bart.

Re: [PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Adrian Hunter]

On 01/10/2021 19:59, Bart Van Assche wrote:
> On 9/30/21 11:52 PM, Adrian Hunter wrote:
>> Finally, there is another thing to change.  The reason
>> ufshcd_suspend_prepare() does a runtime resume of sdev_rpmb is because the
>> UAC clear would wait for an async runtime resume, which will never happen
>> during system suspend because the PM workqueue gets frozen.  So with the
>> removal of UAC clear, ufshcd_suspend_prepare() and ufshcd_resume_complete()
>> should be updated also, to leave rpmb alone.
> 
> Is the following change what you have in mind?

Yes, exactly.

> 
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0a28cc4c09d8..0743f54e55f9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -9648,10 +9648,6 @@ void ufshcd_resume_complete(struct device *dev)
>          ufshcd_rpm_put(hba);
>          hba->complete_put = false;
>      }
> -    if (hba->rpmb_complete_put) {
> -        ufshcd_rpmb_rpm_put(hba);
> -        hba->rpmb_complete_put = false;
> -    }
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
> 
> @@ -9674,10 +9670,6 @@ int ufshcd_suspend_prepare(struct device *dev)
>          }
>          hba->complete_put = true;
>      }
> -    if (hba->sdev_rpmb) {
> -        ufshcd_rpmb_rpm_get_sync(hba);
> -        hba->rpmb_complete_put = true;
> -    }
>      return 0;
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 86b615023ecb..5ecfcd8cae0a 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -921,7 +921,6 @@ struct ufs_hba {
>  #endif
>      u32 luns_avail;
>      bool complete_put;
> -    bool rpmb_complete_put;
>  };
> 
>  /* Returns true if clocks can be gated. Otherwise false */
> 
> 
> 
> Thanks,
> 
> Bart.

Re: [PATCH 2/2] scsi: ufs: Stop clearing unit attentions

[Bart Van Assche]

On 9/30/21 9:58 PM, Adrian Hunter wrote:
> On 30/09/2021 22:52, Jaegeuk Kim wrote:
>> From: Bart Van Assche <bvanassche@google.com>
>>
>> Commit aa53f580e67b ("scsi: ufs: Minor adjustments to error handling")
>> introduced a ufshcd_clear_ua_wluns() call in
>> ufshcd_err_handling_unprepare(). As explained in detail by Adrian Hunter,
>> this can trigger a deadlock. Avoid that deadlock by removing the code that
>> clears the unit attention. This is safe because the only software that
>> relies on clearing unit attentions is the Android Trusty software and
> 
> Did you test this?

This patch series has been tested on an Android phone running the latest Trusty
software but with runtime power management disabled.

Bart.

Re: [PATCH 1/2] scsi: ufs: retry START_STOP on UNIT_ATTENTION

[Bart Van Assche]

On 9/30/21 12:52 PM, Jaegeuk Kim wrote:
> Commit 57d104c153d3 ("ufs: add UFS power management support") made the UFS
> driver submit a REQUEST SENSE command before submitting a power management
> command to a WLUN to clear the POWER ON unit attention. Instead of
> submitting a REQUEST SENSE command before submitting a power management
> command, retry the power management command until it succeeds.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 0/2 v2] kill clearing UA in UFS driver

[Jaegeuk Kim]

There are some issues reported and fixed when clearing UA on reset/PM flows.
Let's avoid any potential bugs entirely by letting user clear UA.

Bart Van Assche (1):
  scsi: ufs: Stop clearing unit attentions

Jaegeuk Kim (1):
  scsi: ufs: retry START_STOP on UNIT_ATTENTION

 drivers/scsi/ufs/ufshcd.c | 196 ++------------------------------------
 drivers/scsi/ufs/ufshcd.h |  14 ---
 2 files changed, 10 insertions(+), 200 deletions(-)

-- 
2.33.0.800.g4c38ced690-goog

[PATCH 1/2] scsi: ufs: retry START_STOP on UNIT_ATTENTION

[Jaegeuk Kim]

Commit 57d104c153d3 ("ufs: add UFS power management support") made the UFS
driver submit a REQUEST SENSE command before submitting a power management
command to a WLUN to clear the POWER ON unit attention. Instead of
submitting a REQUEST SENSE command before submitting a power management
command, retry the power management command until it succeeds.

This is the preparation to get rid of all UNIT ATTENTION code which should
be handled by users.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 drivers/scsi/ufs/ufshcd.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9faf02cbb9ad..7ec72de826e5 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8676,7 +8676,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	struct scsi_sense_hdr sshdr;
 	struct scsi_device *sdp;
 	unsigned long flags;
-	int ret;
+	int ret, retries;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	sdp = hba->sdev_ufs_device;
@@ -8711,8 +8711,14 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	 * callbacks hence set the RQF_PM flag so that it doesn't resume the
 	 * already suspended childs.
 	 */
-	ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
-			START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
+	for (retries = 3; retries > 0; --retries) {
+		ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
+				START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
+		if (!scsi_status_is_check_condition(ret) ||
+				!scsi_sense_valid(&sshdr) ||
+				sshdr.sense_key != UNIT_ATTENTION)
+			break;
+	}
 	if (ret) {
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",
-- 
2.33.0.800.g4c38ced690-goog