[PATCH V4 0/3] scsi: ufs: Let devices remain runtime suspended during system suspend

[PATCH V4 0/3] scsi: ufs: Let devices remain runtime suspended during system suspend

[Adrian Hunter]

Hi

UFS devices can remain runtime suspended at system suspend time,
if the conditions are right.  Add support for that, first fixing
the impediments.


Changes in V4:

      scsi: ufs: Fix error handler clear ua deadlock

	Do request-sense directly

Changes in V3:

      scsi: ufs: Fix error handler clear ua deadlock

	Correct commit message.
	Amend stable tags to add dependent cherry picks

Changes in V2:

    scsi: ufs: Let devices remain runtime suspended during system suspend

	The ufs-hisi driver uses different RPM and SPM, but it is made
	explicit by a new parameter to suspend prepare.


Adrian Hunter (3):
      scsi: ufs: Fix error handler clear ua deadlock
      scsi: ufs: Fix runtime PM dependencies getting broken
      scsi: ufs: Let devices remain runtime suspended during system suspend

 drivers/scsi/scsi_pm.c      |  16 ++-
 drivers/scsi/ufs/ufs-hisi.c |   8 +-
 drivers/scsi/ufs/ufs.h      |   3 +-
 drivers/scsi/ufs/ufshcd.c   | 255 ++++++++++++++++++++++++++++++++------------
 drivers/scsi/ufs/ufshcd.h   |  21 +++-
 include/scsi/scsi_device.h  |   1 +
 include/trace/events/ufs.h  |   5 +-
 7 files changed, 231 insertions(+), 78 deletions(-)


Regards
Adrian

[PATCH V4 1/3] scsi: ufs: Fix error handler clear ua deadlock

[Adrian Hunter]

There is no guarantee to be able to enter the queue if requests are
blocked. That is because freezing the queue will block entry to the
queue, but freezing also waits for outstanding requests which can make
no progress while the queue is blocked.

That situation can happen when the error handler issues requests to
clear unit attention condition. Requests are blocked by
scsi_schedule_eh() and not cleared until the error handler has run.
Requests can also be blocked if the ufshcd_state is
UFSHCD_STATE_EH_SCHEDULED_FATAL, which can happen as a result of
prior error handler activity. Requests cannot make progress when
ufshcd_state is UFSHCD_STATE_EH_SCHEDULED_FATAL, and only the error
handler can change that, so if the error handler is waiting to enter
the queue and blk_mq_freeze_queue() is waiting for outstanding requests,
they will deadlock.

Fix by issuing REQUEST_SENSE directly avoiding the SCSI queues, in
a similar fashion as other device commands.

Fixes: b294ff3e34490f ("scsi: ufs: core: Enable power management for wlun")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufs.h     |   3 +-
 drivers/scsi/ufs/ufshcd.c  | 201 ++++++++++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h  |   9 ++
 include/trace/events/ufs.h |   5 +-
 4 files changed, 160 insertions(+), 58 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 8c6b38b1b142..c60c6d7c62c1 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -612,7 +612,8 @@ struct ufs_dev_info {
 enum ufs_trace_str_t {
 	UFS_CMD_SEND, UFS_CMD_COMP, UFS_DEV_COMP,
 	UFS_QUERY_SEND, UFS_QUERY_COMP, UFS_QUERY_ERR,
-	UFS_TM_SEND, UFS_TM_COMP, UFS_TM_ERR
+	UFS_TM_SEND, UFS_TM_COMP, UFS_TM_ERR,
+	UFS_SENSE_SEND, UFS_SENSE_COMP, UFS_SENSE_ERR,
 };
 
 /*
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 67889d74761c..2802083e8c3f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2814,6 +2814,29 @@ ufshcd_check_query_response(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	return query_res->response;
 }
 
+static int
+ufshcd_check_sense_response(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
+{
+	int scsi_status;
+	int resp;
+
+	resp = ufshcd_get_req_rsp(lrbp->ucd_rsp_ptr);
+	if (resp != UPIU_TRANSACTION_RESPONSE) {
+		dev_err(hba->dev, "%s: Invalid cmd response: %#x\n",
+			__func__, resp);
+		return -EINVAL;
+	}
+
+	resp = ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr);
+	scsi_status = resp & MASK_SCSI_STATUS;
+	if (scsi_status != SAM_STAT_GOOD) {
+		dev_err(hba->dev, "%s: REQUEST_SENSE failed, status %#x\n",
+			__func__, scsi_status);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 /**
  * ufshcd_dev_cmd_completion() - handles device management command responses
  * @hba: per adapter instance
@@ -2847,6 +2870,12 @@ ufshcd_dev_cmd_completion(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		dev_err(hba->dev, "%s: Reject UPIU not fully implemented\n",
 				__func__);
 		break;
+	case UPIU_TRANSACTION_RESPONSE:
+		if (hba->dev_cmd.type == DEV_CMD_TYPE_SENSE) {
+			err = ufshcd_check_sense_response(hba, lrbp);
+			break;
+		}
+		fallthrough;
 	default:
 		err = -EINVAL;
 		dev_err(hba->dev, "%s: Invalid device management cmd response: %x\n",
@@ -3553,6 +3582,7 @@ static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba)
 static int ufshcd_memory_alloc(struct ufs_hba *hba)
 {
 	size_t utmrdl_size, utrdl_size, ucdl_size;
+	size_t extra, dev_cmd_buf_offs;
 
 	/* Allocate memory for UTP command descriptors */
 	ucdl_size = (sizeof(struct utp_transfer_cmd_desc) * hba->nutrs);
@@ -3590,13 +3620,16 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
 		goto out;
 	}
 
+	/* Double for alignment */
+	extra = UFS_DEV_CMD_BUF_SZ * 2;
+
 	/*
 	 * Allocate memory for UTP Task Management descriptors
 	 * UFSHCI requires 1024 byte alignment of UTMRD
 	 */
 	utmrdl_size = sizeof(struct utp_task_req_desc) * hba->nutmrs;
 	hba->utmrdl_base_addr = dmam_alloc_coherent(hba->dev,
-						    utmrdl_size,
+						    utmrdl_size + extra,
 						    &hba->utmrdl_dma_addr,
 						    GFP_KERNEL);
 	if (!hba->utmrdl_base_addr ||
@@ -3606,6 +3639,10 @@ static int ufshcd_memory_alloc(struct ufs_hba *hba)
 		goto out;
 	}
 
+	dev_cmd_buf_offs = round_up(utmrdl_size, UFS_DEV_CMD_BUF_SZ);
+	hba->dev_cmd.buf = (void *)hba->utmrdl_base_addr + dev_cmd_buf_offs;
+	hba->dev_cmd.buf_dma_addr = hba->utmrdl_dma_addr + dev_cmd_buf_offs;
+
 	/* Allocate memory for local reference block */
 	hba->lrb = devm_kcalloc(hba->dev,
 				hba->nutrs, sizeof(struct ufshcd_lrb),
@@ -5297,14 +5334,14 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 			cmd->scsi_done(cmd);
 			ufshcd_release(hba);
 			update_scaling = true;
-		} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
-			lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
-			if (hba->dev_cmd.complete) {
-				ufshcd_add_command_trace(hba, index,
-							 UFS_DEV_COMP);
-				complete(hba->dev_cmd.complete);
-				update_scaling = true;
-			}
+		} else if ((lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
+			    lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE ||
+			    (lrbp->command_type == UTP_CMD_TYPE_SCSI &&
+			     hba->dev_cmd.type == DEV_CMD_TYPE_SENSE)) &&
+			   hba->dev_cmd.complete) {
+			ufshcd_add_command_trace(hba, index, UFS_DEV_COMP);
+			complete(hba->dev_cmd.complete);
+			update_scaling = true;
 		}
 		if (update_scaling)
 			ufshcd_clk_scaling_update_busy(hba);
@@ -7934,60 +7971,110 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
 	return ret;
 }
 
-static void ufshcd_request_sense_done(struct request *rq, blk_status_t error)
+static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun)
 {
-	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_queue *q = hba->cmd_queue;
+	struct ufshcd_sg_entry *prd_table;
+	struct utp_upiu_req *ucd_req_ptr;
+	DECLARE_COMPLETION_ONSTACK(wait);
+	struct ufshcd_lrb *lrbp;
+	int timeout_ms = 1000;
 	struct request *req;
-	char *buffer;
-	int ret;
+	u8 upiu_flags;
+	int err = 0;
+	int tag;
 
-	buffer = kzalloc(UFS_SENSE_SIZE, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
+	ufshcd_hold(hba, false);
+	mutex_lock(&hba->dev_cmd.lock);
+	down_read(&hba->clk_scaling_lock);
 
-	req = blk_get_request(sdev->request_queue, REQ_OP_DRV_IN,
-			      /*flags=*/BLK_MQ_REQ_PM);
+	/* The command queue cannot be frozen */
+	req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
 	if (IS_ERR(req)) {
-		ret = PTR_ERR(req);
-		goto out_free;
+		err = PTR_ERR(req);
+		goto out_unlock;
+	}
+	tag = req->tag;
+	if (WARN_ONCE(tag < 0, "Invalid tag %d\n", tag) ||
+	    unlikely(test_bit(tag, &hba->outstanding_reqs))) {
+		err = -EBUSY;
+		goto out;
 	}
 
-	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;
+	lrbp = &hba->lrb[tag];
+	WARN_ON(lrbp->cmd);
+	lrbp->cmd = NULL;
+	lrbp->sense_bufflen = 0;
+	lrbp->sense_buffer = NULL;
+	lrbp->task_tag = tag;
+	lrbp->lun = wlun;
+	lrbp->intr_cmd = true;
+	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
+	hba->dev_cmd.type = DEV_CMD_TYPE_SENSE;
+
+	if (hba->ufs_version <= ufshci_version(1, 1))
+		lrbp->command_type = UTP_CMD_TYPE_SCSI;
+	else
+		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
+
+	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_FROM_DEVICE);
+
+	/* Prepare UPIU */
+	ucd_req_ptr = lrbp->ucd_req_ptr;
+
+	/* command descriptor fields */
+	ucd_req_ptr->header.dword_0 =
+		UPIU_HEADER_DWORD(UPIU_TRANSACTION_COMMAND, upiu_flags,
+				  lrbp->lun, lrbp->task_tag);
+	ucd_req_ptr->header.dword_1 =
+		UPIU_HEADER_DWORD(UPIU_COMMAND_SET_TYPE_SCSI, 0, 0, 0);
+
+	/* Total EHS length and Data segment length will be zero */
+	ucd_req_ptr->header.dword_2 = 0;
+
+	ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(UFS_SENSE_SIZE);
 
-out_put:
+	memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
+	memcpy(ucd_req_ptr->sc.cdb, cmd, ARRAY_SIZE(cmd));
+
+	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
+	memset(hba->dev_cmd.buf, 0, UFS_DEV_CMD_BUF_SZ);
+
+	/* Prepare PRD table */
+	if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) {
+		lrbp->utr_descriptor_ptr->prd_table_length =
+				cpu_to_le16(sizeof(struct ufshcd_sg_entry));
+	} else {
+		lrbp->utr_descriptor_ptr->prd_table_length = cpu_to_le16(1);
+	}
+
+	prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr;
+
+	prd_table[0].size       = cpu_to_le32(UFS_SENSE_SIZE - 1);
+	prd_table[0].base_addr  = cpu_to_le32(lower_32_bits(hba->dev_cmd.buf_dma_addr));
+	prd_table[0].upper_addr = cpu_to_le32(upper_32_bits(hba->dev_cmd.buf_dma_addr));
+	prd_table[0].reserved   = 0;
+
+	hba->dev_cmd.complete = &wait;
+
+	ufshcd_add_query_upiu_trace(hba, UFS_SENSE_SEND, lrbp->ucd_req_ptr);
+
+	ufshcd_send_command(hba, tag);
+
+	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout_ms);
+
+	ufshcd_add_query_upiu_trace(hba, err ? UFS_SENSE_ERR : UFS_SENSE_COMP,
+				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
+out:
 	blk_put_request(req);
-out_free:
-	kfree(buffer);
-	return ret;
+out_unlock:
+	/* Invalidate dev_cmd.type for safety */
+	hba->dev_cmd.type = DEV_CMD_TYPE_INVALID;
+	up_read(&hba->clk_scaling_lock);
+	mutex_unlock(&hba->dev_cmd.lock);
+	ufshcd_release(hba);
+	return err;
 }
 
 static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
@@ -8016,7 +8103,7 @@ static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
 	if (ret)
 		goto out_err;
 
-	ret = ufshcd_request_sense_async(hba, sdp);
+	ret = ufshcd_request_sense_direct(hba, wlun);
 	scsi_device_put(sdp);
 out_err:
 	if (ret)
@@ -8094,8 +8181,10 @@ 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;
+	hba->wlun_dev_clr_ua =
+		!!ufshcd_request_sense_direct(hba, UFS_UPIU_UFS_DEVICE_WLUN);
+	hba->wlun_rpmb_clr_ua =
+		!!ufshcd_request_sense_direct(hba, UFS_UPIU_RPMB_WLUN);
 
 	/* Gear up to HS gear if supported */
 	if (hba->max_pwr_info.is_valid) {
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 4723f27a55d1..82c0f689cbdb 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -56,6 +56,8 @@ struct ufs_hba;
 enum dev_cmd_type {
 	DEV_CMD_TYPE_NOP		= 0x0,
 	DEV_CMD_TYPE_QUERY		= 0x1,
+	DEV_CMD_TYPE_SENSE		= 0x2,
+	DEV_CMD_TYPE_INVALID		= 0xff,
 };
 
 enum ufs_event_type {
@@ -236,17 +238,24 @@ struct ufs_query {
 	struct ufs_query_res response;
 };
 
+/* UFS_DEV_CMD_BUF_SZ must be a power of 2 and bigger than UFS_SENSE_SIZE */
+#define UFS_DEV_CMD_BUF_SZ 32
+
 /**
  * struct ufs_dev_cmd - all assosiated fields with device management commands
  * @type: device management command type - Query, NOP OUT
  * @lock: lock to allow one command at a time
  * @complete: internal commands completion
+ * @buf: buffer for data
+ * @buf_dma_addr: DMA address of @buf
  */
 struct ufs_dev_cmd {
 	enum dev_cmd_type type;
 	struct mutex lock;
 	struct completion *complete;
 	struct ufs_query query;
+	void *buf;
+	dma_addr_t buf_dma_addr;
 };
 
 /**
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 599739ee7b20..efa2af4a3a5b 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -46,7 +46,10 @@
 	EM(UFS_QUERY_ERR,	"query_complete_err")		\
 	EM(UFS_TM_SEND,		"tm_send")			\
 	EM(UFS_TM_COMP,		"tm_complete")			\
-	EMe(UFS_TM_ERR,		"tm_complete_err")
+	EM(UFS_TM_ERR,		"tm_complete_err")		\
+	EM(UFS_SENSE_SEND,	"sense_send")			\
+	EM(UFS_SENSE_COMP,	"sense_complete")		\
+	EMe(UFS_SENSE_ERR,	"sense_complete_err")
 
 #define UFS_CMD_TRACE_TSF_TYPES					\
 	EM(UFS_TSF_CDB,		"CDB")		                \
-- 
2.17.1

[PATCH V4 2/3] scsi: ufs: Fix runtime PM dependencies getting broken

[Adrian Hunter]

UFS SCSI devices make use of device links to establish PM dependencies.
However, SCSI PM will force devices' runtime PM state to be active during
system resume. That can break runtime PM dependencies for UFS devices.
Fix by adding a flag 'preserve_rpm' to let UFS SCSI devices opt-out of
the unwanted behaviour.

Fixes: b294ff3e34490f ("scsi: ufs: core: Enable power management for wlun")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/scsi_pm.c     | 16 +++++++++++-----
 drivers/scsi/ufs/ufshcd.c  |  1 +
 include/scsi/scsi_device.h |  1 +
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 3717eea37ecb..0557c1ad304d 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -73,13 +73,22 @@ static int scsi_dev_type_resume(struct device *dev,
 		int (*cb)(struct device *, const struct dev_pm_ops *))
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	struct scsi_device *sdev = NULL;
+	bool preserve_rpm = false;
 	int err = 0;
 
+	if (scsi_is_sdev_device(dev)) {
+		sdev = to_scsi_device(dev);
+		preserve_rpm = sdev->preserve_rpm;
+		if (preserve_rpm && pm_runtime_suspended(dev))
+			return 0;
+	}
+
 	err = cb(dev, pm);
 	scsi_device_resume(to_scsi_device(dev));
 	dev_dbg(dev, "scsi resume: %d\n", err);
 
-	if (err == 0) {
+	if (err == 0 && !preserve_rpm) {
 		pm_runtime_disable(dev);
 		err = pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
@@ -91,11 +100,8 @@ static int scsi_dev_type_resume(struct device *dev,
 		 *
 		 * The resume hook will correct runtime PM status of the disk.
 		 */
-		if (!err && scsi_is_sdev_device(dev)) {
-			struct scsi_device *sdev = to_scsi_device(dev);
-
+		if (!err && sdev)
 			blk_set_runtime_active(sdev->request_queue);
-		}
 	}
 
 	return err;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2802083e8c3f..9c9df2ad2a34 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5053,6 +5053,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 		pm_runtime_get_noresume(&sdev->sdev_gendev);
 	else if (ufshcd_is_rpm_autosuspend_allowed(hba))
 		sdev->rpm_autosuspend = 1;
+	sdev->preserve_rpm = 1;
 
 	ufshcd_crypto_setup_rq_keyslot_manager(hba, q);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 09a17f6e93a7..47eb30a6b7b2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -197,6 +197,7 @@ struct scsi_device {
 	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned try_rc_10_first:1;	/* Try READ_CAPACACITY_10 first */
+	unsigned preserve_rpm:1;	/* Preserve runtime PM */
 	unsigned security_supported:1;	/* Supports Security Protocols */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
-- 
2.17.1

[PATCH V4 3/3] scsi: ufs: Let devices remain runtime suspended during system suspend

[Adrian Hunter]

If the UFS Device WLUN is runtime suspended and is in the same power
mode, link state and b_rpm_dev_flush_capable (BKOP or WB buffer flush etc)
state, then it can remain runtime suspended instead of being runtime
resumed and then system suspended.

The following patches have cleared the way for that to happen:
  scsi: ufs: Fix runtime PM dependencies getting broken
  scsi: ufs: Fix error handler clear ua deadlock

So amend the logic accordingly.

Note, the ufs-hisi driver uses different RPM and SPM, but it is made
explicit by a new parameter to suspend prepare.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufs-hisi.c |  8 +++++-
 drivers/scsi/ufs/ufshcd.c   | 53 ++++++++++++++++++++++++++++---------
 drivers/scsi/ufs/ufshcd.h   | 12 ++++++++-
 3 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c
index 6b706de8354b..4a08fb35642c 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -396,6 +396,12 @@ static int ufs_hisi_pwr_change_notify(struct ufs_hba *hba,
 	return ret;
 }
 
+static int ufs_hisi_suspend_prepare(struct device *dev)
+{
+	/* RPM and SPM are different. Refer ufs_hisi_suspend() */
+	return __ufshcd_suspend_prepare(dev, false);
+}
+
 static int ufs_hisi_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 {
 	struct ufs_hisi_host *host = ufshcd_get_variant(hba);
@@ -574,7 +580,7 @@ static int ufs_hisi_remove(struct platform_device *pdev)
 static const struct dev_pm_ops ufs_hisi_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
 	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
-	.prepare	 = ufshcd_suspend_prepare,
+	.prepare	 = ufs_hisi_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9c9df2ad2a34..56542b09eca6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9806,14 +9806,30 @@ 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);
 
-int ufshcd_suspend_prepare(struct device *dev)
+static bool ufshcd_rpm_ok_for_spm(struct ufs_hba *hba)
+{
+	struct device *dev = &hba->sdev_ufs_device->sdev_gendev;
+	enum ufs_dev_pwr_mode dev_pwr_mode;
+	enum uic_link_state link_state;
+	unsigned long flags;
+	bool res;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+	dev_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl);
+	link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl);
+	res = pm_runtime_suspended(dev) &&
+	      hba->curr_dev_pwr_mode == dev_pwr_mode &&
+	      hba->uic_link_state == link_state &&
+	      !hba->dev_info.b_rpm_dev_flush_capable;
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+
+	return res;
+}
+
+int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 	int ret;
@@ -9825,19 +9841,30 @@ int ufshcd_suspend_prepare(struct device *dev)
 	 * Refer ufshcd_resume_complete()
 	 */
 	if (hba->sdev_ufs_device) {
-		ret = ufshcd_rpm_get_sync(hba);
-		if (ret < 0 && ret != -EACCES) {
-			ufshcd_rpm_put(hba);
-			return ret;
+		/* Prevent runtime suspend */
+		ufshcd_rpm_get_noresume(hba);
+		/*
+		 * Check if already runtime suspended in same state as system
+		 * suspend would be.
+		 */
+		if (!rpm_ok_for_spm || !ufshcd_rpm_ok_for_spm(hba)) {
+			/* RPM state is not ok for SPM, so runtime resume */
+			ret = ufshcd_rpm_resume(hba);
+			if (ret < 0 && ret != -EACCES) {
+				ufshcd_rpm_put(hba);
+				return ret;
+			}
 		}
 		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);
+
+int ufshcd_suspend_prepare(struct device *dev)
+{
+	return __ufshcd_suspend_prepare(dev, true);
+}
 EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 82c0f689cbdb..49e8dd70eb5e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -924,7 +924,6 @@ struct ufs_hba {
 #endif
 	u32 luns_avail;
 	bool complete_put;
-	bool rpmb_complete_put;
 };
 
 /* Returns true if clocks can be gated. Otherwise false */
@@ -1184,6 +1183,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 
 int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
 int ufshcd_suspend_prepare(struct device *dev);
+int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm);
 void ufshcd_resume_complete(struct device *dev);
 
 /* Wrapper functions for safely calling variant operations */
@@ -1392,6 +1392,16 @@ static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba)
 	return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev);
 }
 
+static inline void ufshcd_rpm_get_noresume(struct ufs_hba *hba)
+{
+	pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev);
+}
+
+static inline int ufshcd_rpm_resume(struct ufs_hba *hba)
+{
+	return pm_runtime_resume(&hba->sdev_ufs_device->sdev_gendev);
+}
+
 static inline int ufshcd_rpm_put(struct ufs_hba *hba)
 {
 	return pm_runtime_put(&hba->sdev_ufs_device->sdev_gendev);
-- 
2.17.1

Re: [PATCH V4 1/3] scsi: ufs: Fix error handler clear ua deadlock

[Bart Van Assche]

On 9/16/21 10:02 AM, Adrian Hunter wrote:
> -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error)
> +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun)
>   {
[ ... ]
> +	/* The command queue cannot be frozen */
> +	req = blk_get_request(q, REQ_OP_DRV_OUT, 0);

hba->cmd_queue shares a tag set with the request queues associated with SCSI
LUNs. Since this blk_get_request() call happens from the context of the error
handler before SCSI requests are unblocked, it will hang if all tags are
in use for SCSI requests before the error handler starts.

Bart.

Re: [PATCH V4 1/3] scsi: ufs: Fix error handler clear ua deadlock

[Adrian Hunter]

On 17/09/21 7:09 pm, Bart Van Assche wrote:
> On 9/16/21 10:02 AM, Adrian Hunter wrote:
>> -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error)
>> +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun)
>>   {
> [ ... ]
>> +    /* The command queue cannot be frozen */
>> +    req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
> 
> hba->cmd_queue shares a tag set with the request queues associated with SCSI
> LUNs. Since this blk_get_request() call happens from the context of the error
> handler before SCSI requests are unblocked, it will hang if all tags are
> in use for SCSI requests before the error handler starts.

All the commands sent by ufshcd_probe_hba() take the same approach, so no
difference there.

Re: [PATCH V4 1/3] scsi: ufs: Fix error handler clear ua deadlock

[Bart Van Assche]

On 9/17/21 10:39 AM, Adrian Hunter wrote:
> On 17/09/21 7:09 pm, Bart Van Assche wrote:
>> On 9/16/21 10:02 AM, Adrian Hunter wrote:
>>> -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error)
>>> +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun)
>>>    {
>> [ ... ]
>>> +    /* The command queue cannot be frozen */
>>> +    req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
>>
>> hba->cmd_queue shares a tag set with the request queues associated with SCSI
>> LUNs. Since this blk_get_request() call happens from the context of the error
>> handler before SCSI requests are unblocked, it will hang if all tags are
>> in use for SCSI requests before the error handler starts.
> 
> All the commands sent by ufshcd_probe_hba() take the same approach, so no
> difference there.

The same comment applies to the commands sent by ufshcd_probe_hba() I think.
If you would like to address this issue, how about passing the flag
BLK_MQ_REQ_RESERVED to the blk_get_request() call in ufshcd_exec_dev_cmd()?
The following additional changes are required to make this work:
* Add a 'reserved_tags' member to struct scsi_host_template, e.g. close to
   tag_alloc_policy.
* Modify scsi_mq_setup_tags() such that it copies the reserved_tags value
   from the host template into tag_set->reserved_tags.
* Set 'reserved_tags' in the UFS driver SCSI host template.

Thanks,

Bart.

Re: [PATCH V4 1/3] scsi: ufs: Fix error handler clear ua deadlock

[Adrian Hunter]

On 17/09/21 8:58 pm, Bart Van Assche wrote:
> On 9/17/21 10:39 AM, Adrian Hunter wrote:
>> On 17/09/21 7:09 pm, Bart Van Assche wrote:
>>> On 9/16/21 10:02 AM, Adrian Hunter wrote:
>>>> -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error)
>>>> +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun)
>>>>    {
>>> [ ... ]
>>>> +    /* The command queue cannot be frozen */
>>>> +    req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
>>>
>>> hba->cmd_queue shares a tag set with the request queues associated with SCSI
>>> LUNs. Since this blk_get_request() call happens from the context of the error
>>> handler before SCSI requests are unblocked, it will hang if all tags are
>>> in use for SCSI requests before the error handler starts.
>>
>> All the commands sent by ufshcd_probe_hba() take the same approach, so no
>> difference there.
> 
> The same comment applies to the commands sent by ufshcd_probe_hba() I think.
> If you would like to address this issue, how about passing the flag
> BLK_MQ_REQ_RESERVED to the blk_get_request() call in ufshcd_exec_dev_cmd()?
> The following additional changes are required to make this work:
> * Add a 'reserved_tags' member to struct scsi_host_template, e.g. close to
>   tag_alloc_policy.
> * Modify scsi_mq_setup_tags() such that it copies the reserved_tags value
>   from the host template into tag_set->reserved_tags.
> * Set 'reserved_tags' in the UFS driver SCSI host template.

I think these things only happen on the reset path, so all the slots should
be free, even if all the tags are allocated.  With some care, it might be
possible simply to grab a free slot.

Re: [PATCH V4 1/3] scsi: ufs: Fix error handler clear ua deadlock

[Bart Van Assche]

On 9/20/21 10:30 AM, Adrian Hunter wrote:
> On 17/09/21 8:58 pm, Bart Van Assche wrote:
>> The same comment applies to the commands sent by ufshcd_probe_hba() I think.
>> If you would like to address this issue, how about passing the flag
>> BLK_MQ_REQ_RESERVED to the blk_get_request() call in ufshcd_exec_dev_cmd()?
>> The following additional changes are required to make this work:
>> * Add a 'reserved_tags' member to struct scsi_host_template, e.g. close to
>>    tag_alloc_policy.
>> * Modify scsi_mq_setup_tags() such that it copies the reserved_tags value
>>    from the host template into tag_set->reserved_tags.
>> * Set 'reserved_tags' in the UFS driver SCSI host template.
> 
> I think these things only happen on the reset path, so all the slots should
> be free, even if all the tags are allocated.  With some care, it might be
> possible simply to grab a free slot.

Hmm ... my understanding is that ufshcd_try_to_abort_task() may fail and hence
that it is not guaranteed that there will be free slot. Anyway, I'm fine with
the patch in its current shape and implementing the mechanism explained in my
previous email at a later time.

Bart.