[PATCH 00/11] UFS patches for kernel v5.17

[PATCH 00/11] UFS patches for kernel v5.17

[Bart Van Assche]

Hi Martin,

Please consider these UFS driver kernel patches for kernel v5.17.

Thanks,

Bart.

Bart Van Assche (11):
  scsi: ufs: Rename a function argument
  scsi: ufs: Remove is_rpmb_wlun()
  scsi: ufs: Remove the sdev_rpmb member
  scsi: ufs: Remove dead code
  scsi: core: Add support for reserved tags
  scsi: ufs: Rework ufshcd_change_queue_depth()
  scsi: ufs: Fix a deadlock in the error handler
  scsi: ufs: Improve SCSI abort handling further
  scsi: ufs: Fix a kernel crash during shutdown
  scsi: ufs: Optimize the command queueing code
  scsi: ufs: Implement polling support

 drivers/scsi/scsi_lib.c       |   4 +-
 drivers/scsi/ufs/ufs-exynos.c |   4 +-
 drivers/scsi/ufs/ufshcd.c     | 233 +++++++++++++++-------------------
 drivers/scsi/ufs/ufshcd.h     |   5 +-
 include/scsi/scsi_host.h      |   9 +-
 5 files changed, 115 insertions(+), 140 deletions(-)

[PATCH 01/11] scsi: ufs: Rename a function argument

[Bart Van Assche]

The new name makes it clear what the meaning of the function argument is.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufs-exynos.c | 4 ++--
 drivers/scsi/ufs/ufshcd.h     | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c
index cd26bc82462e..474a4a064a68 100644
--- a/drivers/scsi/ufs/ufs-exynos.c
+++ b/drivers/scsi/ufs/ufs-exynos.c
@@ -853,14 +853,14 @@ static int exynos_ufs_post_pwr_mode(struct ufs_hba *hba,
 }
 
 static void exynos_ufs_specify_nexus_t_xfer_req(struct ufs_hba *hba,
-						int tag, bool op)
+						int tag, bool is_scsi_cmd)
 {
 	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
 	u32 type;
 
 	type =  hci_readl(ufs, HCI_UTRL_NEXUS_TYPE);
 
-	if (op)
+	if (is_scsi_cmd)
 		hci_writel(ufs, type | (1 << tag), HCI_UTRL_NEXUS_TYPE);
 	else
 		hci_writel(ufs, type & ~(1 << tag), HCI_UTRL_NEXUS_TYPE);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 4ceb3c7e65b4..a911ad72de7a 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -338,7 +338,8 @@ struct ufs_hba_variant_ops {
 					enum ufs_notify_change_status status,
 					struct ufs_pa_layer_attr *,
 					struct ufs_pa_layer_attr *);
-	void	(*setup_xfer_req)(struct ufs_hba *, int, bool);
+	void	(*setup_xfer_req)(struct ufs_hba *hba, int tag,
+				  bool is_scsi_cmd);
 	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);
 	void    (*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
 					enum ufs_notify_change_status);

[PATCH 02/11] scsi: ufs: Remove is_rpmb_wlun()

[Bart Van Assche]

Commit edc0596cc04b ("scsi: ufs: core: Stop clearing UNIT ATTENTIONS")
removed all callers of is_rpmb_wlun(). Hence also remove the function
itself.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index dac8fbf221f7..d18685d080d7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2650,11 +2650,6 @@ static inline u16 ufshcd_upiu_wlun_to_scsi_wlun(u8 upiu_wlun_id)
 	return (upiu_wlun_id & ~UFS_UPIU_WLUN_ID) | SCSI_W_LUN_BASE;
 }
 
-static inline bool is_rpmb_wlun(struct scsi_device *sdev)
-{
-	return sdev->lun == ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN);
-}
-
 static inline bool is_device_wlun(struct scsi_device *sdev)
 {
 	return sdev->lun ==

[PATCH 03/11] scsi: ufs: Remove the sdev_rpmb member

[Bart Van Assche]

Since the sdev_rpmb member of struct ufs_hba is only used inside
ufshcd_scsi_add_wlus(), convert it into a local variable.

Suggested-by: Jaegeuk Kim <jaegeuk@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 12 ++++++------
 drivers/scsi/ufs/ufshcd.h |  1 -
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d18685d080d7..dff76b1a0d5d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7407,7 +7407,7 @@ static inline void ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
 static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
 {
 	int ret = 0;
-	struct scsi_device *sdev_boot;
+	struct scsi_device *sdev_boot, *sdev_rpmb;
 
 	hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
 		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN), NULL);
@@ -7418,14 +7418,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
 	}
 	scsi_device_put(hba->sdev_ufs_device);
 
-	hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
+	sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
 		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
-	if (IS_ERR(hba->sdev_rpmb)) {
-		ret = PTR_ERR(hba->sdev_rpmb);
+	if (IS_ERR(sdev_rpmb)) {
+		ret = PTR_ERR(sdev_rpmb);
 		goto remove_sdev_ufs_device;
 	}
-	ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
-	scsi_device_put(hba->sdev_rpmb);
+	ufshcd_blk_pm_runtime_init(sdev_rpmb);
+	scsi_device_put(sdev_rpmb);
 
 	sdev_boot = __scsi_add_device(hba->host, 0, 0,
 		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index a911ad72de7a..65178487adf3 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -809,7 +809,6 @@ struct ufs_hba {
 	 * "UFS device" W-LU.
 	 */
 	struct scsi_device *sdev_ufs_device;
-	struct scsi_device *sdev_rpmb;
 
 #ifdef CONFIG_SCSI_UFS_HWMON
 	struct device *hwmon_device;

[PATCH 04/11] scsi: ufs: Remove dead code

[Bart Van Assche]

Commit 7252a3603015 ("scsi: ufs: Avoid busy-waiting by eliminating tag
conflicts") guarantees that 'tag' is not in use by any SCSI command.
Remove the check that returns early if a conflict occurs.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index dff76b1a0d5d..312e8a5b7733 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6724,11 +6724,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	tag = req->tag;
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
-	if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
-		err = -EBUSY;
-		goto out;
-	}
-
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	lrbp->cmd = NULL;
@@ -6796,7 +6791,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
-out:
 	blk_put_request(req);
 out_unlock:
 	up_read(&hba->clk_scaling_lock);

[PATCH 05/11] scsi: core: Add support for reserved tags

[Bart Van Assche]

Allow SCSI LLDs to allocate reserved tags by passing the BLK_MQ_REQ_RESERVED
flag to blk_mq_alloc_request().

Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c  | 4 +++-
 include/scsi/scsi_host.h | 9 ++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 43151e5d07ab..51d4ae1a5b16 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1916,6 +1916,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 {
 	unsigned int cmd_size, sgl_size;
 	struct blk_mq_tag_set *tag_set = &shost->tag_set;
+	unsigned int reserved_tags = shost->hostt->reserved_tags;
 
 	sgl_size = max_t(unsigned int, sizeof(struct scatterlist),
 				scsi_mq_inline_sgl_size(shost));
@@ -1931,7 +1932,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 		tag_set->ops = &scsi_mq_ops_no_commit;
 	tag_set->nr_hw_queues = shost->nr_hw_queues ? : 1;
 	tag_set->nr_maps = shost->nr_maps ? : 1;
-	tag_set->queue_depth = shost->can_queue;
+	tag_set->queue_depth = shost->can_queue + reserved_tags;
+	tag_set->reserved_tags = reserved_tags;
 	tag_set->cmd_size = cmd_size;
 	tag_set->numa_node = NUMA_NO_NODE;
 	tag_set->flags = BLK_MQ_F_SHOULD_MERGE;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index ebe059badba0..0fa3ee73501a 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -367,10 +367,17 @@ struct scsi_host_template {
 	/*
 	 * This determines if we will use a non-interrupt driven
 	 * or an interrupt driven scheme.  It is set to the maximum number
-	 * of simultaneous commands a single hw queue in HBA will accept.
+	 * of simultaneous commands a single hw queue in HBA will accept. Does
+	 * not include @reserved_tags.
 	 */
 	int can_queue;
 
+	/*
+	 * Number of tags to reserve. A reserved tag can be allocated by passing
+	 * the BLK_MQ_REQ_RESERVED flag to blk_get_request().
+	 */
+	unsigned reserved_tags;
+
 	/*
 	 * In many instances, especially where disconnect / reconnect are
 	 * supported, our host also has an ID on the SCSI bus.  If this is

[PATCH 06/11] scsi: ufs: Rework ufshcd_change_queue_depth()

[Bart Van Assche]

Prepare for making sdev->host->can_queue less than hba->nutrs. This patch
does not change any functionality.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 312e8a5b7733..8400d8e9a6f7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4955,11 +4955,7 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
  */
 static int ufshcd_change_queue_depth(struct scsi_device *sdev, int depth)
 {
-	struct ufs_hba *hba = shost_priv(sdev->host);
-
-	if (depth > hba->nutrs)
-		depth = hba->nutrs;
-	return scsi_change_queue_depth(sdev, depth);
+	return scsi_change_queue_depth(sdev, min(depth, sdev->host->can_queue));
 }
 
 static void ufshcd_hpb_destroy(struct ufs_hba *hba, struct scsi_device *sdev)

[PATCH 07/11] scsi: ufs: Fix a deadlock in the error handler

[Bart Van Assche]

The following deadlock has been observed on a test setup:
* All tags allocated.
* The SCSI error handler calls ufshcd_eh_host_reset_handler()
* ufshcd_eh_host_reset_handler() queues work that calls ufshcd_err_handler()
* ufshcd_err_handler() locks up as follows:

Workqueue: ufs_eh_wq_0 ufshcd_err_handler.cfi_jt
Call trace:
 __switch_to+0x298/0x5d8
 __schedule+0x6cc/0xa94
 schedule+0x12c/0x298
 blk_mq_get_tag+0x210/0x480
 __blk_mq_alloc_request+0x1c8/0x284
 blk_get_request+0x74/0x134
 ufshcd_exec_dev_cmd+0x68/0x640
 ufshcd_verify_dev_init+0x68/0x35c
 ufshcd_probe_hba+0x12c/0x1cb8
 ufshcd_host_reset_and_restore+0x88/0x254
 ufshcd_reset_and_restore+0xd0/0x354
 ufshcd_err_handler+0x408/0xc58
 process_one_work+0x24c/0x66c
 worker_thread+0x3e8/0xa4c
 kthread+0x150/0x1b4
 ret_from_fork+0x10/0x30

Fix this lockup by making ufshcd_exec_dev_cmd() allocate a reserved
request.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8400d8e9a6f7..8f5640647054 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2940,12 +2940,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 
 	down_read(&hba->clk_scaling_lock);
 
-	/*
-	 * Get free slot, sleep if slots are unavailable.
-	 * Even though we use wait_event() which sleeps indefinitely,
-	 * the maximum wait time is bounded by SCSI request timeout.
-	 */
-	req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
+	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, BLK_MQ_REQ_RESERVED);
 	if (IS_ERR(req)) {
 		err = PTR_ERR(req);
 		goto out_unlock;
@@ -8152,7 +8147,8 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
 	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
-	.can_queue		= UFSHCD_CAN_QUEUE,
+	.can_queue		= UFSHCD_CAN_QUEUE - 1,
+	.reserved_tags		= 1,
 	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
 	.max_host_blocked	= 1,
 	.track_queue_depth	= 1,
@@ -9513,8 +9509,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	/* Configure LRB */
 	ufshcd_host_memory_configure(hba);
 
-	host->can_queue = hba->nutrs;
-	host->cmd_per_lun = hba->nutrs;
+	host->can_queue = hba->nutrs - 1;
+	host->cmd_per_lun = hba->nutrs - 1;
 	host->max_id = UFSHCD_MAX_ID;
 	host->max_lun = UFS_MAX_LUNS;
 	host->max_channel = UFSHCD_MAX_CHANNEL;

[PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Bart Van Assche]

Make sure that aborted commands are completed once by clearing the
corresponding tag bit from hba->outstanding_reqs. This patch is a
follow-up for commit cd892096c940 ("scsi: ufs: core: Improve SCSI
abort handling").

Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8f5640647054..1e15ed1f639f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7090,6 +7090,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		goto release;
 	}
 
+	/*
+	 * ufshcd_try_to_abort_task() cleared the 'tag' bit in the doorbell
+	 * register. Clear the corresponding bit from outstanding_reqs to
+	 * prevent early completion.
+	 */
+	spin_lock_irqsave(&hba->outstanding_lock, flags);
+	__clear_bit(tag, &hba->outstanding_reqs);
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+
 	lrbp->cmd = NULL;
 	err = SUCCESS;
 

[PATCH 09/11] scsi: ufs: Fix a kernel crash during shutdown

[Bart Van Assche]

Fix the following kernel crash:

Unable to handle kernel paging request at virtual address ffffffc91e735000
Call trace:
 __queue_work+0x26c/0x624
 queue_work_on+0x6c/0xf0
 ufshcd_hold+0x12c/0x210
 __ufshcd_wl_suspend+0xc0/0x400
 ufshcd_wl_shutdown+0xb8/0xcc
 device_shutdown+0x184/0x224
 kernel_restart+0x4c/0x124
 __arm64_sys_reboot+0x194/0x264
 el0_svc_common+0xc8/0x1d4
 do_el0_svc+0x30/0x8c
 el0_svc+0x20/0x30
 el0_sync_handler+0x84/0xe4
 el0_sync+0x1bc/0x1c0

Fix this crash by ungating the clock before destroying the work queue
on which clock gating work is queued.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1e15ed1f639f..13848e93cda8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1666,7 +1666,8 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 	bool flush_result;
 	unsigned long flags;
 
-	if (!ufshcd_is_clkgating_allowed(hba))
+	if (!ufshcd_is_clkgating_allowed(hba) ||
+	    !hba->clk_gating.is_initialized)
 		goto out;
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->clk_gating.active_reqs++;
@@ -1826,7 +1827,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
 
 	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
 	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
-	    hba->outstanding_tasks ||
+	    hba->outstanding_tasks || !hba->clk_gating.is_initialized ||
 	    hba->active_uic_cmd || hba->uic_async_done ||
 	    hba->clk_gating.state == CLKS_OFF)
 		return;
@@ -1961,11 +1962,15 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
 {
 	if (!hba->clk_gating.is_initialized)
 		return;
+
 	ufshcd_remove_clk_gating_sysfs(hba);
-	cancel_work_sync(&hba->clk_gating.ungate_work);
-	cancel_delayed_work_sync(&hba->clk_gating.gate_work);
-	destroy_workqueue(hba->clk_gating.clk_gating_workq);
+
+	/* Ungate the clock if necessary. */
+	ufshcd_hold(hba, false);
 	hba->clk_gating.is_initialized = false;
+	ufshcd_release(hba);
+
+	destroy_workqueue(hba->clk_gating.clk_gating_workq);
 }
 
 /* Must be called with host lock acquired */

[PATCH 10/11] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

Remove the clock scaling lock from ufshcd_queuecommand() since it is a
performance bottleneck. As requested by Asutosh Das, change the behavior
of ufshcd_clock_scaling_prepare() from waiting until all pending
commands have finished into quiescing request queues. Insert a
rcu_read_lock() / rcu_read_unlock() pair in ufshcd_queuecommand() and also
in __ufshcd_issue_tm_cmd(). Use synchronize_rcu_expedited() to wait for
ongoing command and TMF queueing.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 121 +++++++++++++-------------------------
 drivers/scsi/ufs/ufshcd.h |   1 +
 2 files changed, 42 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 13848e93cda8..36df89e8a575 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1069,65 +1069,6 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
 	return false;
 }
 
-static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
-					u64 wait_timeout_us)
-{
-	unsigned long flags;
-	int ret = 0;
-	u32 tm_doorbell;
-	u32 tr_doorbell;
-	bool timeout = false, do_last_check = false;
-	ktime_t start;
-
-	ufshcd_hold(hba, false);
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	/*
-	 * Wait for all the outstanding tasks/transfer requests.
-	 * Verify by checking the doorbell registers are clear.
-	 */
-	start = ktime_get();
-	do {
-		if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
-			ret = -EBUSY;
-			goto out;
-		}
-
-		tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
-		tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-		if (!tm_doorbell && !tr_doorbell) {
-			timeout = false;
-			break;
-		} else if (do_last_check) {
-			break;
-		}
-
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
-		schedule();
-		if (ktime_to_us(ktime_sub(ktime_get(), start)) >
-		    wait_timeout_us) {
-			timeout = true;
-			/*
-			 * We might have scheduled out for long time so make
-			 * sure to check if doorbells are cleared by this time
-			 * or not.
-			 */
-			do_last_check = true;
-		}
-		spin_lock_irqsave(hba->host->host_lock, flags);
-	} while (tm_doorbell || tr_doorbell);
-
-	if (timeout) {
-		dev_err(hba->dev,
-			"%s: timedout waiting for doorbell to clear (tm=0x%x, tr=0x%x)\n",
-			__func__, tm_doorbell, tr_doorbell);
-		ret = -EBUSY;
-	}
-out:
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	ufshcd_release(hba);
-	return ret;
-}
-
 /**
  * ufshcd_scale_gear - scale up/down UFS gear
  * @hba: per adapter instance
@@ -1175,37 +1116,51 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
 
 static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 {
-	#define DOORBELL_CLR_TOUT_US		(1000 * 1000) /* 1 sec */
-	int ret = 0;
+	struct scsi_device *sdev;
+
 	/*
-	 * make sure that there are no outstanding requests when
-	 * clock scaling is in progress
+	 * Make sure that no commands are being queued while clock scaling
+	 * is in progress.
+	 *
+	 * Since ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd() lock
+	 * the clk_scaling_lock before calling blk_get_request(), lock
+	 * clk_scaling_lock before freezing the request queues to prevent lock
+	 * inversion.
 	 */
-	ufshcd_scsi_block_requests(hba);
 	down_write(&hba->clk_scaling_lock);
-
-	if (!hba->clk_scaling.is_allowed ||
-	    ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
-		ret = -EBUSY;
+	if (!hba->clk_scaling.is_allowed) {
 		up_write(&hba->clk_scaling_lock);
-		ufshcd_scsi_unblock_requests(hba);
-		goto out;
+		return -EBUSY;
 	}
-
+	blk_mq_quiesce_queue_nowait(hba->tmf_queue);
+	blk_mq_quiesce_queue_nowait(hba->cmd_queue);
+	shost_for_each_device(sdev, hba->host)
+		blk_mq_quiesce_queue_nowait(sdev->request_queue);
+	/*
+	 * Calling synchronize_rcu_expedited() reduces the time required to
+	 * quiesce request queues from milliseconds to microseconds.
+	 *
+	 * See also the rcu_read_lock() and rcu_read_unlock() calls in
+	 * ufshcd_queuecommand() and also in __ufshcd_issue_tm_cmd().
+	 */
+	synchronize_rcu_expedited();
 	/* let's not get into low power until clock scaling is completed */
 	ufshcd_hold(hba, false);
-
-out:
-	return ret;
+	return 0;
 }
 
 static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
 {
+	struct scsi_device *sdev;
+
+	shost_for_each_device(sdev, hba->host)
+		blk_mq_unquiesce_queue(sdev->request_queue);
+	blk_mq_unquiesce_queue(hba->cmd_queue);
+	blk_mq_unquiesce_queue(hba->tmf_queue);
 	if (writelock)
 		up_write(&hba->clk_scaling_lock);
 	else
 		up_read(&hba->clk_scaling_lock);
-	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_release(hba);
 }
 
@@ -2698,8 +2653,11 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
-	if (!down_read_trylock(&hba->clk_scaling_lock))
-		return SCSI_MLQUEUE_HOST_BUSY;
+	/*
+	 * Allows ufshcd_clock_scaling_prepare() and also the UFS error handler
+	 * to wait for prior ufshcd_queuecommand() calls.
+	 */
+	rcu_read_lock();
 
 	switch (hba->ufshcd_state) {
 	case UFSHCD_STATE_OPERATIONAL:
@@ -2780,7 +2738,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	ufshcd_send_command(hba, tag);
 out:
-	up_read(&hba->clk_scaling_lock);
+	rcu_read_unlock();
 
 	if (ufs_trigger_eh()) {
 		unsigned long flags;
@@ -5977,8 +5935,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 	}
 	ufshcd_scsi_block_requests(hba);
 	/* Drain ufshcd_queuecommand() */
-	down_write(&hba->clk_scaling_lock);
-	up_write(&hba->clk_scaling_lock);
+	synchronize_rcu();
 	cancel_work_sync(&hba->eeh_work);
 }
 
@@ -6582,6 +6539,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	req->end_io_data = &wait;
 	ufshcd_hold(hba, false);
 
+	rcu_read_lock();
+
 	spin_lock_irqsave(host->host_lock, flags);
 
 	task_tag = req->tag;
@@ -6600,6 +6559,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 
 	spin_unlock_irqrestore(host->host_lock, flags);
 
+	rcu_read_unlock();
+
 	ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_SEND);
 
 	/* wait until the task management command is completed */
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 65178487adf3..7afe818ab1e3 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -778,6 +778,7 @@ struct ufs_hba_monitor {
  * @clk_list_head: UFS host controller clocks list node head
  * @pwr_info: holds current power mode
  * @max_pwr_info: keeps the device max valid pwm
+ * @clk_scaling_lock: used to serialize device commands and clock scaling
  * @desc_size: descriptor sizes reported by device
  * @urgent_bkops_lvl: keeps track of urgent bkops level for device
  * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for

[PATCH 11/11] scsi: ufs: Implement polling support

[Bart Van Assche]

The time spent in io_schedule() is significant when submitting direct
I/O to a UFS device. Hence this patch that implements polling support.
User space software can enable polling by passing the RWF_HIPRI flag to
the preadv2() system call or the IORING_SETUP_IOPOLL flag to the
io_uring interface.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 36df89e8a575..70f128f12445 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5250,6 +5250,31 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 	}
 }
 
+/*
+ * Returns > 0 if one or more commands have been completed or 0 if no
+ * requests have been completed.
+ */
+static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
+{
+	struct ufs_hba *hba = shost_priv(shost);
+	unsigned long completed_reqs, flags;
+	u32 tr_doorbell;
+
+	spin_lock_irqsave(&hba->outstanding_lock, flags);
+	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+	completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
+	WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
+		  "completed: %#lx; outstanding: %#lx\n", completed_reqs,
+		  hba->outstanding_reqs);
+	hba->outstanding_reqs &= ~completed_reqs;
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+
+	if (completed_reqs)
+		__ufshcd_transfer_req_compl(hba, completed_reqs);
+
+	return completed_reqs;
+}
+
 /**
  * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
@@ -5260,9 +5285,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
  */
 static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 {
-	unsigned long completed_reqs, flags;
-	u32 tr_doorbell;
-
 	/* Resetting interrupt aggregation counters first and reading the
 	 * DOOR_BELL afterward allows us to handle all the completed requests.
 	 * In order to prevent other interrupts starvation the DB is read once
@@ -5277,21 +5299,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 	if (ufs_fail_completion())
 		return IRQ_HANDLED;
 
-	spin_lock_irqsave(&hba->outstanding_lock, flags);
-	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-	completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
-	WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
-		  "completed: %#lx; outstanding: %#lx\n", completed_reqs,
-		  hba->outstanding_reqs);
-	hba->outstanding_reqs &= ~completed_reqs;
-	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
-
-	if (completed_reqs) {
-		__ufshcd_transfer_req_compl(hba, completed_reqs);
-		return IRQ_HANDLED;
-	} else {
-		return IRQ_NONE;
-	}
+	return ufshcd_poll(hba->host, 0) ? IRQ_HANDLED : IRQ_NONE;
 }
 
 int __ufshcd_write_ee_control(struct ufs_hba *hba, u32 ee_ctrl_mask)
@@ -8112,6 +8120,7 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.name			= UFSHCD,
 	.proc_name		= UFSHCD,
 	.queuecommand		= ufshcd_queuecommand,
+	.mq_poll		= ufshcd_poll,
 	.slave_alloc		= ufshcd_slave_alloc,
 	.slave_configure	= ufshcd_slave_configure,
 	.slave_destroy		= ufshcd_slave_destroy,

RE: [PATCH 01/11] scsi: ufs: Rename a function argument

[Chanho Park]

Hello,

> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Wednesday, November 10, 2021 9:45 AM
> To: Martin K . Petersen <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org; Jaegeuk Kim <jaegeuk@kernel.org>; Adrian
> Hunter <adrian.hunter@intel.com>; Bart Van Assche <bvanassche@acm.org>;
> James E.J. Bottomley <jejb@linux.ibm.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com>; Alim Akhtar
<alim.akhtar@samsung.com>;
> Bean Huo <beanhuo@micron.com>; Avri Altman <avri.altman@wdc.com>; Kiwoong
> Kim <kwmad.kim@samsung.com>; Yue Hu <huyue2@yulong.com>; Peter Wang
> <peter.wang@mediatek.com>; Chanho Park <chanho61.park@samsung.com>;
> Stanley Chu <stanley.chu@mediatek.com>; Can Guo <cang@codeaurora.org>;
> Asutosh Das <asutoshd@codeaurora.org>; Keoseong Park
> <keosung.park@samsung.com>
> Subject: [PATCH 01/11] scsi: ufs: Rename a function argument
> 
> The new name makes it clear what the meaning of the function argument is.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Great.
Reviewed-by: Chanho Park <chanho61.park@samsung.com>

Best Regards,
Chanho Park

Re: [PATCH 11/11] scsi: ufs: Implement polling support

[Douglas Gilbert]

On 2021-11-09 7:44 p.m., Bart Van Assche wrote:
> The time spent in io_schedule() is significant when submitting direct
> I/O to a UFS device. Hence this patch that implements polling support.
> User space software can enable polling by passing the RWF_HIPRI flag to
> the preadv2() system call or the IORING_SETUP_IOPOLL flag to the
> io_uring interface.

There have been some changes recently (i.e. in linux-stable now),
"HIPRI" seems to be on the out, replaced by "POLLED". [I'm using
poll_lld in my sg rewrite to refer to this type of polling, as "poll"
is an overloaded term in the kernel].

REQ_HIPRI has become REQ_POLLED and blk_poll() is now bio_poll().
That said RWF_HIPRI is still in fs.h and there is no RWF_POLLED (yet).

LL_POLL or LOW_POLL would be more indicative of what is happening,
rather than POLLED, IMO.


Not sure if the new bio_poll() code is working, I'm looking at an
NULL pointer dereference at: RIP: 0010:bio_poll+0x17/0xe0

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/ufs/ufshcd.c | 45 +++++++++++++++++++++++----------------
>   1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 36df89e8a575..70f128f12445 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5250,6 +5250,31 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>   	}
>   }
>   
> +/*
> + * Returns > 0 if one or more commands have been completed or 0 if no
> + * requests have been completed.
> + */
> +static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
> +{
> +	struct ufs_hba *hba = shost_priv(shost);
> +	unsigned long completed_reqs, flags;
> +	u32 tr_doorbell;
> +
> +	spin_lock_irqsave(&hba->outstanding_lock, flags);
> +	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> +	completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
> +	WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
> +		  "completed: %#lx; outstanding: %#lx\n", completed_reqs,
> +		  hba->outstanding_reqs);
> +	hba->outstanding_reqs &= ~completed_reqs;
> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> +
> +	if (completed_reqs)
> +		__ufshcd_transfer_req_compl(hba, completed_reqs);
> +
> +	return completed_reqs;
> +}
> +
>   /**
>    * ufshcd_transfer_req_compl - handle SCSI and query command completion
>    * @hba: per adapter instance
> @@ -5260,9 +5285,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>    */
>   static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
>   {
> -	unsigned long completed_reqs, flags;
> -	u32 tr_doorbell;
> -
>   	/* Resetting interrupt aggregation counters first and reading the
>   	 * DOOR_BELL afterward allows us to handle all the completed requests.
>   	 * In order to prevent other interrupts starvation the DB is read once
> @@ -5277,21 +5299,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
>   	if (ufs_fail_completion())
>   		return IRQ_HANDLED;
>   
> -	spin_lock_irqsave(&hba->outstanding_lock, flags);
> -	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -	completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
> -	WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
> -		  "completed: %#lx; outstanding: %#lx\n", completed_reqs,
> -		  hba->outstanding_reqs);
> -	hba->outstanding_reqs &= ~completed_reqs;
> -	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> -
> -	if (completed_reqs) {
> -		__ufshcd_transfer_req_compl(hba, completed_reqs);
> -		return IRQ_HANDLED;
> -	} else {
> -		return IRQ_NONE;
> -	}
> +	return ufshcd_poll(hba->host, 0) ? IRQ_HANDLED : IRQ_NONE;
>   }
>   
>   int __ufshcd_write_ee_control(struct ufs_hba *hba, u32 ee_ctrl_mask)
> @@ -8112,6 +8120,7 @@ static struct scsi_host_template ufshcd_driver_template = {
>   	.name			= UFSHCD,
>   	.proc_name		= UFSHCD,
>   	.queuecommand		= ufshcd_queuecommand,
> +	.mq_poll		= ufshcd_poll,
>   	.slave_alloc		= ufshcd_slave_alloc,
>   	.slave_configure	= ufshcd_slave_configure,
>   	.slave_destroy		= ufshcd_slave_destroy,
> 

Re: [PATCH 07/11] scsi: ufs: Fix a deadlock in the error handler

[Christoph Hellwig]

Hi Bart,

as pointed out last time: LLDDS have no business directy allocating
tags.  Please work with Hannes and John on the proper APIs, as metnioned
last time as well.

Re: [PATCH 10/11] scsi: ufs: Optimize the command queueing code

[Adrian Hunter]

On 10/11/2021 02:44, Bart Van Assche wrote:
> Remove the clock scaling lock from ufshcd_queuecommand() since it is a
> performance bottleneck. As requested by Asutosh Das, change the behavior
> of ufshcd_clock_scaling_prepare() from waiting until all pending
> commands have finished into quiescing request queues. Insert a
> rcu_read_lock() / rcu_read_unlock() pair in ufshcd_queuecommand() and also
> in __ufshcd_issue_tm_cmd(). Use synchronize_rcu_expedited() to wait for
> ongoing command and TMF queueing.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 121 +++++++++++++-------------------------
>  drivers/scsi/ufs/ufshcd.h |   1 +
>  2 files changed, 42 insertions(+), 80 deletions(-)
> 

<SNIP>

> @@ -2698,8 +2653,11 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  
>  	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
>  
> -	if (!down_read_trylock(&hba->clk_scaling_lock))
> -		return SCSI_MLQUEUE_HOST_BUSY;
> +	/*
> +	 * Allows ufshcd_clock_scaling_prepare() and also the UFS error handler
> +	 * to wait for prior ufshcd_queuecommand() calls.
> +	 */
> +	rcu_read_lock();

The improvement to flush/drain ufshcd_queuecommand() via RCU should
be a separate patch because it is not dependent on the other changes.

<SNIP>

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Adrian Hunter]

On 10/11/2021 02:44, Bart Van Assche wrote:
> Make sure that aborted commands are completed once by clearing the
> corresponding tag bit from hba->outstanding_reqs. This patch is a
> follow-up for commit cd892096c940 ("scsi: ufs: core: Improve SCSI
> abort handling").
> 
> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8f5640647054..1e15ed1f639f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7090,6 +7090,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  		goto release;
>  	}
>  
> +	/*
> +	 * ufshcd_try_to_abort_task() cleared the 'tag' bit in the doorbell
> +	 * register. Clear the corresponding bit from outstanding_reqs to
> +	 * prevent early completion.
> +	 */
> +	spin_lock_irqsave(&hba->outstanding_lock, flags);
> +	__clear_bit(tag, &hba->outstanding_reqs);
> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);

Seems like something ufshcd_clear_cmd() should be doing instead?

> +
>  	lrbp->cmd = NULL;
>  	err = SUCCESS;
>  
> 

RE: [PATCH 01/11] scsi: ufs: Rename a function argument

[Keoseong Park]

Hi Bart,

>The new name makes it clear what the meaning of the function argument is.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Keoseong Park <keosung.park@samsung.com>

Best Regards,
Keoseong Park

>---
> drivers/scsi/ufs/ufs-exynos.c | 4 ++--
> drivers/scsi/ufs/ufshcd.h     | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c
>index cd26bc82462e..474a4a064a68 100644
>--- a/drivers/scsi/ufs/ufs-exynos.c
>+++ b/drivers/scsi/ufs/ufs-exynos.c
>@@ -853,14 +853,14 @@ static int exynos_ufs_post_pwr_mode(struct ufs_hba *hba,
> }
> 
> static void exynos_ufs_specify_nexus_t_xfer_req(struct ufs_hba *hba,
>-						int tag, bool op)
>+						int tag, bool is_scsi_cmd)
> {
> 	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> 	u32 type;
> 
> 	type =  hci_readl(ufs, HCI_UTRL_NEXUS_TYPE);
> 
>-	if (op)
>+	if (is_scsi_cmd)
> 		hci_writel(ufs, type | (1 << tag), HCI_UTRL_NEXUS_TYPE);
> 	else
> 		hci_writel(ufs, type & ~(1 << tag), HCI_UTRL_NEXUS_TYPE);
>diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>index 4ceb3c7e65b4..a911ad72de7a 100644
>--- a/drivers/scsi/ufs/ufshcd.h
>+++ b/drivers/scsi/ufs/ufshcd.h
>@@ -338,7 +338,8 @@ struct ufs_hba_variant_ops {
> 					enum ufs_notify_change_status status,
> 					struct ufs_pa_layer_attr *,
> 					struct ufs_pa_layer_attr *);
>-	void	(*setup_xfer_req)(struct ufs_hba *, int, bool);
>+	void	(*setup_xfer_req)(struct ufs_hba *hba, int tag,
>+				  bool is_scsi_cmd);
> 	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);
> 	void    (*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
> 					enum ufs_notify_change_status);
>

Re: [PATCH 02/11] scsi: ufs: Remove is_rpmb_wlun()

[Asutosh Das (asd)]

On 11/9/2021 4:44 PM, Bart Van Assche wrote:
> Commit edc0596cc04b ("scsi: ufs: core: Stop clearing UNIT ATTENTIONS")
> removed all callers of is_rpmb_wlun(). Hence also remove the function
> itself.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/ufs/ufshcd.c | 5 -----
>   1 file changed, 5 deletions(-)
> 

LGTM.
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index dac8fbf221f7..d18685d080d7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2650,11 +2650,6 @@ static inline u16 ufshcd_upiu_wlun_to_scsi_wlun(u8 upiu_wlun_id)
>   	return (upiu_wlun_id & ~UFS_UPIU_WLUN_ID) | SCSI_W_LUN_BASE;
>   }
>   
> -static inline bool is_rpmb_wlun(struct scsi_device *sdev)
> -{
> -	return sdev->lun == ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN);
> -}
> -
>   static inline bool is_device_wlun(struct scsi_device *sdev)
>   {
>   	return sdev->lun ==
> 


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

Re: [PATCH 03/11] scsi: ufs: Remove the sdev_rpmb member

[Asutosh Das (asd)]

On 11/9/2021 4:44 PM, Bart Van Assche wrote:
> Since the sdev_rpmb member of struct ufs_hba is only used inside
> ufshcd_scsi_add_wlus(), convert it into a local variable.
> 
> Suggested-by: Jaegeuk Kim <jaegeuk@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

>   drivers/scsi/ufs/ufshcd.c | 12 ++++++------
>   drivers/scsi/ufs/ufshcd.h |  1 -
>   2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d18685d080d7..dff76b1a0d5d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7407,7 +7407,7 @@ static inline void ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
>   static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
>   {
>   	int ret = 0;
> -	struct scsi_device *sdev_boot;
> +	struct scsi_device *sdev_boot, *sdev_rpmb;
>   
>   	hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
>   		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN), NULL);
> @@ -7418,14 +7418,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
>   	}
>   	scsi_device_put(hba->sdev_ufs_device);
>   
> -	hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> +	sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
>   		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
> -	if (IS_ERR(hba->sdev_rpmb)) {
> -		ret = PTR_ERR(hba->sdev_rpmb);
> +	if (IS_ERR(sdev_rpmb)) {
> +		ret = PTR_ERR(sdev_rpmb);
>   		goto remove_sdev_ufs_device;
>   	}
> -	ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
> -	scsi_device_put(hba->sdev_rpmb);
> +	ufshcd_blk_pm_runtime_init(sdev_rpmb);
> +	scsi_device_put(sdev_rpmb);
>   
>   	sdev_boot = __scsi_add_device(hba->host, 0, 0,
>   		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index a911ad72de7a..65178487adf3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -809,7 +809,6 @@ struct ufs_hba {
>   	 * "UFS device" W-LU.
>   	 */
>   	struct scsi_device *sdev_ufs_device;
> -	struct scsi_device *sdev_rpmb;
>   
>   #ifdef CONFIG_SCSI_UFS_HWMON
>   	struct device *hwmon_device;
> 


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

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Bart Van Assche]

On 11/10/21 12:57 AM, Adrian Hunter wrote:
> On 10/11/2021 02:44, Bart Van Assche wrote:
>> Make sure that aborted commands are completed once by clearing the
>> corresponding tag bit from hba->outstanding_reqs. This patch is a
>> follow-up for commit cd892096c940 ("scsi: ufs: core: Improve SCSI
>> abort handling").
>>
>> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/ufs/ufshcd.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 8f5640647054..1e15ed1f639f 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7090,6 +7090,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>>   		goto release;
>>   	}
>>   
>> +	/*
>> +	 * ufshcd_try_to_abort_task() cleared the 'tag' bit in the doorbell
>> +	 * register. Clear the corresponding bit from outstanding_reqs to
>> +	 * prevent early completion.
>> +	 */
>> +	spin_lock_irqsave(&hba->outstanding_lock, flags);
>> +	__clear_bit(tag, &hba->outstanding_reqs);
>> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> 
> Seems like something ufshcd_clear_cmd() should be doing instead?

Hi Adrian,

I'm concerned that would break ufshcd_eh_device_reset_handler() since 
that reset handler retries SCSI commands by calling 
__ufshcd_transfer_req_compl() after having called ufshcd_clear_cmd().

Thanks,

Bart.

Re: [PATCH 10/11] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

On 11/10/21 12:04 AM, Adrian Hunter wrote:
> On 10/11/2021 02:44, Bart Van Assche wrote:
>> @@ -2698,8 +2653,11 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>>   
>>   	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
>>   
>> -	if (!down_read_trylock(&hba->clk_scaling_lock))
>> -		return SCSI_MLQUEUE_HOST_BUSY;
>> +	/*
>> +	 * Allows ufshcd_clock_scaling_prepare() and also the UFS error handler
>> +	 * to wait for prior ufshcd_queuecommand() calls.
>> +	 */
>> +	rcu_read_lock();
> 
> The improvement to flush/drain ufshcd_queuecommand() via RCU should
> be a separate patch because it is not dependent on the other changes.

I will split this patch into two patches.

Thanks,

Bart.

RE: [PATCH 04/11] scsi: ufs: Remove dead code

[Avri Altman]

> 
> Commit 7252a3603015 ("scsi: ufs: Avoid busy-waiting by eliminating tag
> conflicts") guarantees that 'tag' is not in use by any SCSI command.
> Remove the check that returns early if a conflict occurs.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Avri Altman <avri.altman@wdc.com>

RE: [PATCH 06/11] scsi: ufs: Rework ufshcd_change_queue_depth()

[Avri Altman]

> 
> Prepare for making sdev->host->can_queue less than hba->nutrs. This patch
> does not change any functionality.
ufshcd_set_queue_depth() may also needs similar adjustments?

Thanks,
Avri

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 312e8a5b7733..8400d8e9a6f7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4955,11 +4955,7 @@ static int ufshcd_slave_alloc(struct scsi_device
> *sdev)
>   */
>  static int ufshcd_change_queue_depth(struct scsi_device *sdev, int depth)  {
> -       struct ufs_hba *hba = shost_priv(sdev->host);
> -
> -       if (depth > hba->nutrs)
> -               depth = hba->nutrs;
> -       return scsi_change_queue_depth(sdev, depth);
> +       return scsi_change_queue_depth(sdev, min(depth,
> + sdev->host->can_queue));
>  }
> 
>  static void ufshcd_hpb_destroy(struct ufs_hba *hba, struct scsi_device *sdev)

RE: [PATCH 07/11] scsi: ufs: Fix a deadlock in the error handler

[Avri Altman]

> The following deadlock has been observed on a test setup:
> * All tags allocated.
> * The SCSI error handler calls ufshcd_eh_host_reset_handler()
> * ufshcd_eh_host_reset_handler() queues work that calls ufshcd_err_handler()
> * ufshcd_err_handler() locks up as follows:
> 
> Workqueue: ufs_eh_wq_0 ufshcd_err_handler.cfi_jt Call trace:
>  __switch_to+0x298/0x5d8
>  __schedule+0x6cc/0xa94
>  schedule+0x12c/0x298
>  blk_mq_get_tag+0x210/0x480
>  __blk_mq_alloc_request+0x1c8/0x284
>  blk_get_request+0x74/0x134
>  ufshcd_exec_dev_cmd+0x68/0x640
>  ufshcd_verify_dev_init+0x68/0x35c
>  ufshcd_probe_hba+0x12c/0x1cb8
>  ufshcd_host_reset_and_restore+0x88/0x254
>  ufshcd_reset_and_restore+0xd0/0x354
>  ufshcd_err_handler+0x408/0xc58
>  process_one_work+0x24c/0x66c
>  worker_thread+0x3e8/0xa4c
>  kthread+0x150/0x1b4
>  ret_from_fork+0x10/0x30
> 
> Fix this lockup by making ufshcd_exec_dev_cmd() allocate a reserved request.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 8400d8e9a6f7..8f5640647054 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2940,12 +2940,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
> 
>         down_read(&hba->clk_scaling_lock);
> 
> -       /*
> -        * Get free slot, sleep if slots are unavailable.
> -        * Even though we use wait_event() which sleeps indefinitely,
> -        * the maximum wait time is bounded by SCSI request timeout.
> -        */
> -       req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
> +       req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT,
> + BLK_MQ_REQ_RESERVED);
>         if (IS_ERR(req)) {
>                 err = PTR_ERR(req);
>                 goto out_unlock;
> @@ -8152,7 +8147,8 @@ static struct scsi_host_template
> ufshcd_driver_template = {
>         .this_id                = -1,
>         .sg_tablesize           = SG_ALL,
>         .cmd_per_lun            = UFSHCD_CMD_PER_LUN,
UFSHCD_CMD_PER_LUN needs to be < 32 as well?

Thanks,
Avri
> -       .can_queue              = UFSHCD_CAN_QUEUE,
> +       .can_queue              = UFSHCD_CAN_QUEUE - 1,
> +       .reserved_tags          = 1,
>         .max_segment_size       = PRDT_DATA_BYTE_COUNT_MAX,
>         .max_host_blocked       = 1,
>         .track_queue_depth      = 1,
> @@ -9513,8 +9509,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
>         /* Configure LRB */
>         ufshcd_host_memory_configure(hba);
> 
> -       host->can_queue = hba->nutrs;
> -       host->cmd_per_lun = hba->nutrs;
> +       host->can_queue = hba->nutrs - 1;
> +       host->cmd_per_lun = hba->nutrs - 1;
>         host->max_id = UFSHCD_MAX_ID;
>         host->max_lun = UFS_MAX_LUNS;
>         host->max_channel = UFSHCD_MAX_CHANNEL;

RE: [PATCH 09/11] scsi: ufs: Fix a kernel crash during shutdown

[Avri Altman]

> Fix the following kernel crash:
> 
> Unable to handle kernel paging request at virtual address ffffffc91e735000 Call
> trace:
>  __queue_work+0x26c/0x624
>  queue_work_on+0x6c/0xf0
>  ufshcd_hold+0x12c/0x210
>  __ufshcd_wl_suspend+0xc0/0x400
>  ufshcd_wl_shutdown+0xb8/0xcc
>  device_shutdown+0x184/0x224
>  kernel_restart+0x4c/0x124
>  __arm64_sys_reboot+0x194/0x264
>  el0_svc_common+0xc8/0x1d4
>  do_el0_svc+0x30/0x8c
>  el0_svc+0x20/0x30
>  el0_sync_handler+0x84/0xe4
>  el0_sync+0x1bc/0x1c0
> 
> Fix this crash by ungating the clock before destroying the work queue on which
> clock gating work is queued.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 1e15ed1f639f..13848e93cda8 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1666,7 +1666,8 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
>         bool flush_result;
>         unsigned long flags;
> 
> -       if (!ufshcd_is_clkgating_allowed(hba))
> +       if (!ufshcd_is_clkgating_allowed(hba) ||
> +           !hba->clk_gating.is_initialized)
>                 goto out;
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         hba->clk_gating.active_reqs++;
> @@ -1826,7 +1827,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
> 
>         if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
>             hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> -           hba->outstanding_tasks ||
> +           hba->outstanding_tasks || !hba->clk_gating.is_initialized ||
>             hba->active_uic_cmd || hba->uic_async_done ||
>             hba->clk_gating.state == CLKS_OFF)
>                 return;
> @@ -1961,11 +1962,15 @@ static void ufshcd_exit_clk_gating(struct ufs_hba
> *hba)  {
>         if (!hba->clk_gating.is_initialized)
>                 return;
> +
>         ufshcd_remove_clk_gating_sysfs(hba);
> -       cancel_work_sync(&hba->clk_gating.ungate_work);
> -       cancel_delayed_work_sync(&hba->clk_gating.gate_work);
> -       destroy_workqueue(hba->clk_gating.clk_gating_workq);
> +
> +       /* Ungate the clock if necessary. */
> +       ufshcd_hold(hba, false);
>         hba->clk_gating.is_initialized = false;
> +       ufshcd_release(hba);
Not sure that the symmetry in calling ufshcd_release() is meaningful here?
You don't really want to queue a new gate work, this is why you added !.is_initialized to release(),
Or did I get it wrong?

Thanks,
Avri
> +
> +       destroy_workqueue(hba->clk_gating.clk_gating_workq);
>  }
> 
>  /* Must be called with host lock acquired */

RE: [PATCH 10/11] scsi: ufs: Optimize the command queueing code

[Avri Altman]

> On 10/11/2021 02:44, Bart Van Assche wrote:
> > Remove the clock scaling lock from ufshcd_queuecommand() since it is a
> > performance bottleneck. As requested by Asutosh Das, change the
> > behavior of ufshcd_clock_scaling_prepare() from waiting until all
> > pending commands have finished into quiescing request queues. Insert a
> > rcu_read_lock() / rcu_read_unlock() pair in ufshcd_queuecommand() and
> > also in __ufshcd_issue_tm_cmd(). Use synchronize_rcu_expedited() to
> > wait for ongoing command and TMF queueing.
> >
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 121 +++++++++++++-------------------------
> >  drivers/scsi/ufs/ufshcd.h |   1 +
> >  2 files changed, 42 insertions(+), 80 deletions(-)
> >
> 
> <SNIP>
> 
> > @@ -2698,8 +2653,11 @@ static int ufshcd_queuecommand(struct Scsi_Host
> > *host, struct scsi_cmnd *cmd)
> >
> >       WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> >
> > -     if (!down_read_trylock(&hba->clk_scaling_lock))
> > -             return SCSI_MLQUEUE_HOST_BUSY;
> > +     /*
> > +      * Allows ufshcd_clock_scaling_prepare() and also the UFS error handler
> > +      * to wait for prior ufshcd_queuecommand() calls.
> > +      */
> > +     rcu_read_lock();
> 
> The improvement to flush/drain ufshcd_queuecommand() via RCU should be a
> separate patch because it is not dependent on the other changes.
Ack on that.

Thanks,
Avri

> 
> <SNIP>

RE: [PATCH 11/11] scsi: ufs: Implement polling support

[Avri Altman]

> 
> The time spent in io_schedule() is significant when submitting direct I/O to a
> UFS device. Hence this patch that implements polling support.
> User space software can enable polling by passing the RWF_HIPRI flag to the
> preadv2() system call or the IORING_SETUP_IOPOLL flag to the io_uring
> interface.
Do you expect that this will fix, or at least to some extent, the queue drainage that is so noticeable in direct benchmarks?
Can you share some early estimations?

Also I think this should be a separate patch as well.

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 45 +++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 36df89e8a575..70f128f12445 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5250,6 +5250,31 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>         }
>  }
> 
> +/*
> + * Returns > 0 if one or more commands have been completed or 0 if no
> + * requests have been completed.
> + */
> +static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
> +{
> +       struct ufs_hba *hba = shost_priv(shost);
> +       unsigned long completed_reqs, flags;
> +       u32 tr_doorbell;
> +
> +       spin_lock_irqsave(&hba->outstanding_lock, flags);
> +       tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> +       completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
> +       WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
> +                 "completed: %#lx; outstanding: %#lx\n", completed_reqs,
> +                 hba->outstanding_reqs);
> +       hba->outstanding_reqs &= ~completed_reqs;
> +       spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> +
> +       if (completed_reqs)
> +               __ufshcd_transfer_req_compl(hba, completed_reqs);
> +
> +       return completed_reqs;
> +}
> +
>  /**
>   * ufshcd_transfer_req_compl - handle SCSI and query command completion
>   * @hba: per adapter instance
> @@ -5260,9 +5285,6 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>   */
>  static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)  {
> -       unsigned long completed_reqs, flags;
> -       u32 tr_doorbell;
> -
>         /* Resetting interrupt aggregation counters first and reading the
>          * DOOR_BELL afterward allows us to handle all the completed requests.
>          * In order to prevent other interrupts starvation the DB is read once @@ -
> 5277,21 +5299,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
>         if (ufs_fail_completion())
>                 return IRQ_HANDLED;
> 
> -       spin_lock_irqsave(&hba->outstanding_lock, flags);
> -       tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -       completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
> -       WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
> -                 "completed: %#lx; outstanding: %#lx\n", completed_reqs,
> -                 hba->outstanding_reqs);
> -       hba->outstanding_reqs &= ~completed_reqs;
> -       spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> -
> -       if (completed_reqs) {
> -               __ufshcd_transfer_req_compl(hba, completed_reqs);
> -               return IRQ_HANDLED;
> -       } else {
> -               return IRQ_NONE;
> -       }
> +       return ufshcd_poll(hba->host, 0) ? IRQ_HANDLED : IRQ_NONE;
>  }
> 
>  int __ufshcd_write_ee_control(struct ufs_hba *hba, u32 ee_ctrl_mask) @@ -
> 8112,6 +8120,7 @@ static struct scsi_host_template ufshcd_driver_template =
> {
>         .name                   = UFSHCD,
>         .proc_name              = UFSHCD,
>         .queuecommand           = ufshcd_queuecommand,
> +       .mq_poll                = ufshcd_poll,
Did you consider to use some form blk_mq_tagset_busy_iter,
And return nutrs - busy?

Thanks,
Avri

>         .slave_alloc            = ufshcd_slave_alloc,
>         .slave_configure        = ufshcd_slave_configure,
>         .slave_destroy          = ufshcd_slave_destroy,

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Peter Wang]

Hi Bart,

On 11/10/21 8:44 AM, Bart Van Assche wrote:
> Make sure that aborted commands are completed once by clearing the
> corresponding tag bit from hba->outstanding_reqs. This patch is a
> follow-up for commit cd892096c940 ("scsi: ufs: core: Improve SCSI
> abort handling").
>
> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/ufs/ufshcd.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8f5640647054..1e15ed1f639f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7090,6 +7090,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>   		goto release;
>   	}
>   
> +	/*
> +	 * ufshcd_try_to_abort_task() cleared the 'tag' bit in the doorbell
> +	 * register. Clear the corresponding bit from outstanding_reqs to
> +	 * prevent early completion.
> +	 */
> +	spin_lock_irqsave(&hba->outstanding_lock, flags);
> +	__clear_bit(tag, &hba->outstanding_reqs);
> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> +

should we also call unmap?

     scsi_dma_unmap(cmd);


>   	lrbp->cmd = NULL;
>   	err = SUCCESS;
>   

Re: [PATCH 03/11] scsi: ufs: Remove the sdev_rpmb member

[Alim Akhtar]

Hi Bart

On Wed, Nov 10, 2021 at 6:21 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Since the sdev_rpmb member of struct ufs_hba is only used inside
> ufshcd_scsi_add_wlus(), convert it into a local variable.
>
> Suggested-by: Jaegeuk Kim <jaegeuk@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
Thanks
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/scsi/ufs/ufshcd.c | 12 ++++++------
>  drivers/scsi/ufs/ufshcd.h |  1 -
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d18685d080d7..dff76b1a0d5d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7407,7 +7407,7 @@ static inline void ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
>  static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
>  {
>         int ret = 0;
> -       struct scsi_device *sdev_boot;
> +       struct scsi_device *sdev_boot, *sdev_rpmb;
>
>         hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
>                 ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN), NULL);
> @@ -7418,14 +7418,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
>         }
>         scsi_device_put(hba->sdev_ufs_device);
>
> -       hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
> +       sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
>                 ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
> -       if (IS_ERR(hba->sdev_rpmb)) {
> -               ret = PTR_ERR(hba->sdev_rpmb);
> +       if (IS_ERR(sdev_rpmb)) {
> +               ret = PTR_ERR(sdev_rpmb);
>                 goto remove_sdev_ufs_device;
>         }
> -       ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
> -       scsi_device_put(hba->sdev_rpmb);
> +       ufshcd_blk_pm_runtime_init(sdev_rpmb);
> +       scsi_device_put(sdev_rpmb);
>
>         sdev_boot = __scsi_add_device(hba->host, 0, 0,
>                 ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index a911ad72de7a..65178487adf3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -809,7 +809,6 @@ struct ufs_hba {
>          * "UFS device" W-LU.
>          */
>         struct scsi_device *sdev_ufs_device;
> -       struct scsi_device *sdev_rpmb;
>
>  #ifdef CONFIG_SCSI_UFS_HWMON
>         struct device *hwmon_device;



-- 
Regards,
Alim

Re: [PATCH 02/11] scsi: ufs: Remove is_rpmb_wlun()

[Alim Akhtar]

Hi Bart

On Wed, Nov 10, 2021 at 6:21 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Commit edc0596cc04b ("scsi: ufs: core: Stop clearing UNIT ATTENTIONS")
> removed all callers of is_rpmb_wlun(). Hence also remove the function
> itself.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/scsi/ufs/ufshcd.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index dac8fbf221f7..d18685d080d7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2650,11 +2650,6 @@ static inline u16 ufshcd_upiu_wlun_to_scsi_wlun(u8 upiu_wlun_id)
>         return (upiu_wlun_id & ~UFS_UPIU_WLUN_ID) | SCSI_W_LUN_BASE;
>  }
>
> -static inline bool is_rpmb_wlun(struct scsi_device *sdev)
> -{
> -       return sdev->lun == ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN);
> -}
> -
>  static inline bool is_device_wlun(struct scsi_device *sdev)
>  {
>         return sdev->lun ==



-- 
Regards,
Alim

Re: [PATCH 01/11] scsi: ufs: Rename a function argument

[Alim Akhtar]

Hi Bart

On Wed, Nov 10, 2021 at 6:22 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> The new name makes it clear what the meaning of the function argument is.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
Thanks
Acked-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/scsi/ufs/ufs-exynos.c | 4 ++--
>  drivers/scsi/ufs/ufshcd.h     | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c
> index cd26bc82462e..474a4a064a68 100644
> --- a/drivers/scsi/ufs/ufs-exynos.c
> +++ b/drivers/scsi/ufs/ufs-exynos.c
> @@ -853,14 +853,14 @@ static int exynos_ufs_post_pwr_mode(struct ufs_hba *hba,
>  }
>
>  static void exynos_ufs_specify_nexus_t_xfer_req(struct ufs_hba *hba,
> -                                               int tag, bool op)
> +                                               int tag, bool is_scsi_cmd)
>  {
>         struct exynos_ufs *ufs = ufshcd_get_variant(hba);
>         u32 type;
>
>         type =  hci_readl(ufs, HCI_UTRL_NEXUS_TYPE);
>
> -       if (op)
> +       if (is_scsi_cmd)
>                 hci_writel(ufs, type | (1 << tag), HCI_UTRL_NEXUS_TYPE);
>         else
>                 hci_writel(ufs, type & ~(1 << tag), HCI_UTRL_NEXUS_TYPE);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 4ceb3c7e65b4..a911ad72de7a 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -338,7 +338,8 @@ struct ufs_hba_variant_ops {
>                                         enum ufs_notify_change_status status,
>                                         struct ufs_pa_layer_attr *,
>                                         struct ufs_pa_layer_attr *);
> -       void    (*setup_xfer_req)(struct ufs_hba *, int, bool);
> +       void    (*setup_xfer_req)(struct ufs_hba *hba, int tag,
> +                                 bool is_scsi_cmd);
>         void    (*setup_task_mgmt)(struct ufs_hba *, int, u8);
>         void    (*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
>                                         enum ufs_notify_change_status);



-- 
Regards,
Alim

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Adrian Hunter]

On 10/11/2021 20:56, Bart Van Assche wrote:
> On 11/10/21 12:57 AM, Adrian Hunter wrote:
>> On 10/11/2021 02:44, Bart Van Assche wrote:
>>> Make sure that aborted commands are completed once by clearing the
>>> corresponding tag bit from hba->outstanding_reqs. This patch is a
>>> follow-up for commit cd892096c940 ("scsi: ufs: core: Improve SCSI
>>> abort handling").
>>>
>>> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   drivers/scsi/ufs/ufshcd.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 8f5640647054..1e15ed1f639f 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -7090,6 +7090,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>>>           goto release;
>>>       }
>>>   +    /*
>>> +     * ufshcd_try_to_abort_task() cleared the 'tag' bit in the doorbell
>>> +     * register. Clear the corresponding bit from outstanding_reqs to
>>> +     * prevent early completion.
>>> +     */
>>> +    spin_lock_irqsave(&hba->outstanding_lock, flags);
>>> +    __clear_bit(tag, &hba->outstanding_reqs);
>>> +    spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>>
>> Seems like something ufshcd_clear_cmd() should be doing instead?
> 
> Hi Adrian,
> 
> I'm concerned that would break ufshcd_eh_device_reset_handler() since that reset handler retries SCSI commands by calling __ufshcd_transfer_req_compl() after having called ufshcd_clear_cmd().

Whenever an outstanding_reqs bit transitions 1 -> 0, then
__ufshcd_transfer_req_compl() must be called.

In all cases, the correct logic must have the effect of:

	spin_lock_irqsave(&hba->outstanding_lock, flags);
	bit = __test_and_clear_bit(tag, &hba->outstanding_reqs);
	spin_unlock_irqrestore(&hba->outstanding_lock, flags);

	if (bit)
		__ufshcd_transfer_req_compl(hba, 1UL << tag);

To put it another way, how else does the driver know whether or
not __ufshcd_transfer_req_compl() has been called already.


As a separate issue, in ufshcd_abort() there is:

	/* If command is already aborted/completed, return FAILED. */
	if (!(test_bit(tag, &hba->outstanding_reqs))) {
		dev_err(hba->dev,
			"%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
			__func__, tag, hba->outstanding_reqs, reg);
		goto release;
	}

which seems wrong. FAILED should only be returned to escalate the
error handling, so if the slot has already successfully been
cleared, that is SUCCESS.  scsi_times_out() has already blocked
the scsi_done() path (by setting SCMD_STATE_COMPLETE), so any use
after free must be being caused by SCSI not the ufs driver.

path

Re: [PATCH 10/11] scsi: ufs: Optimize the command queueing code

[Asutosh Das (asd)]

Hi Bart,

On 11/9/2021 4:44 PM, Bart Van Assche wrote:
> Remove the clock scaling lock from ufshcd_queuecommand() since it is a
> performance bottleneck. As requested by Asutosh Das, change the behavior
> of ufshcd_clock_scaling_prepare() from waiting until all pending > commands have finished into quiescing request queues. Insert a

Umm, I was suggesting the following in prepare():
* The requests in the queue should not be issued - as is done now by 
ufshcd_scsi_block_requests()
* The ongoing requests in DBR should be completed i.e. DBR = 0.
* Proceed with scaling

The below code is not waiting for the ongoing requests to complete i.e. 
There's no call to wait for DBR to be 0.
I think waiting for DBR to be 0 is still needed.

> rcu_read_lock() / rcu_read_unlock() pair in ufshcd_queuecommand() and also
> in __ufshcd_issue_tm_cmd(). Use synchronize_rcu_expedited() to wait for
> ongoing command and TMF queueing.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/ufs/ufshcd.c | 121 +++++++++++++-------------------------
>   drivers/scsi/ufs/ufshcd.h |   1 +
>   2 files changed, 42 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 13848e93cda8..36df89e8a575 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1069,65 +1069,6 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
>   	return false;
>   }
>   
> -static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
> -					u64 wait_timeout_us)
> -{
> -	unsigned long flags;
> -	int ret = 0;
> -	u32 tm_doorbell;
> -	u32 tr_doorbell;
> -	bool timeout = false, do_last_check = false;
> -	ktime_t start;
> -
> -	ufshcd_hold(hba, false);
> -	spin_lock_irqsave(hba->host->host_lock, flags);
> -	/*
> -	 * Wait for all the outstanding tasks/transfer requests.
> -	 * Verify by checking the doorbell registers are clear.
> -	 */
> -	start = ktime_get();
> -	do {
> -		if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
> -			ret = -EBUSY;
> -			goto out;
> -		}
> -
> -		tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> -		tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -		if (!tm_doorbell && !tr_doorbell) {
> -			timeout = false;
> -			break;
> -		} else if (do_last_check) {
> -			break;
> -		}
> -
> -		spin_unlock_irqrestore(hba->host->host_lock, flags);
> -		schedule();
> -		if (ktime_to_us(ktime_sub(ktime_get(), start)) >
> -		    wait_timeout_us) {
> -			timeout = true;
> -			/*
> -			 * We might have scheduled out for long time so make
> -			 * sure to check if doorbells are cleared by this time
> -			 * or not.
> -			 */
> -			do_last_check = true;
> -		}
> -		spin_lock_irqsave(hba->host->host_lock, flags);
> -	} while (tm_doorbell || tr_doorbell);
> -
> -	if (timeout) {
> -		dev_err(hba->dev,
> -			"%s: timedout waiting for doorbell to clear (tm=0x%x, tr=0x%x)\n",
> -			__func__, tm_doorbell, tr_doorbell);
> -		ret = -EBUSY;
> -	}
> -out:
> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
> -	ufshcd_release(hba);
> -	return ret;
> -}
> -
>   /**
>    * ufshcd_scale_gear - scale up/down UFS gear
>    * @hba: per adapter instance
> @@ -1175,37 +1116,51 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>   
>   static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
>   {
> -	#define DOORBELL_CLR_TOUT_US		(1000 * 1000) /* 1 sec */
> -	int ret = 0;
> +	struct scsi_device *sdev;
> +
>   	/*
> -	 * make sure that there are no outstanding requests when
> -	 * clock scaling is in progress
> +	 * Make sure that no commands are being queued while clock scaling
> +	 * is in progress.
> +	 *
> +	 * Since ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd() lock
> +	 * the clk_scaling_lock before calling blk_get_request(), lock
> +	 * clk_scaling_lock before freezing the request queues to prevent lock
> +	 * inversion.
>   	 */
> -	ufshcd_scsi_block_requests(hba);
>   	down_write(&hba->clk_scaling_lock);
> -
> -	if (!hba->clk_scaling.is_allowed ||
> -	    ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
> -		ret = -EBUSY;
> +	if (!hba->clk_scaling.is_allowed) {
>   		up_write(&hba->clk_scaling_lock);
> -		ufshcd_scsi_unblock_requests(hba);
> -		goto out;
> +		return -EBUSY;
>   	}
> -
> +	blk_mq_quiesce_queue_nowait(hba->tmf_queue);
> +	blk_mq_quiesce_queue_nowait(hba->cmd_queue);
> +	shost_for_each_device(sdev, hba->host)
> +		blk_mq_quiesce_queue_nowait(sdev->request_queue);
> +	/*
> +	 * Calling synchronize_rcu_expedited() reduces the time required to
> +	 * quiesce request queues from milliseconds to microseconds.
> +	 *
> +	 * See also the rcu_read_lock() and rcu_read_unlock() calls in
> +	 * ufshcd_queuecommand() and also in __ufshcd_issue_tm_cmd().
> +	 */
> +	synchronize_rcu_expedited();
>   	/* let's not get into low power until clock scaling is completed */
>   	ufshcd_hold(hba, false);
> -
> -out:
> -	return ret;
> +	return 0;
>   }
>   
>   static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
>   {
> +	struct scsi_device *sdev;
> +
> +	shost_for_each_device(sdev, hba->host)
> +		blk_mq_unquiesce_queue(sdev->request_queue);
> +	blk_mq_unquiesce_queue(hba->cmd_queue);
> +	blk_mq_unquiesce_queue(hba->tmf_queue);
>   	if (writelock)
>   		up_write(&hba->clk_scaling_lock);
>   	else
>   		up_read(&hba->clk_scaling_lock);
> -	ufshcd_scsi_unblock_requests(hba);
>   	ufshcd_release(hba);
>   }
>   
> @@ -2698,8 +2653,11 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>   
>   	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
>   
> -	if (!down_read_trylock(&hba->clk_scaling_lock))
> -		return SCSI_MLQUEUE_HOST_BUSY;
> +	/*
> +	 * Allows ufshcd_clock_scaling_prepare() and also the UFS error handler
> +	 * to wait for prior ufshcd_queuecommand() calls.
> +	 */
> +	rcu_read_lock();
>   
>   	switch (hba->ufshcd_state) {
>   	case UFSHCD_STATE_OPERATIONAL:
> @@ -2780,7 +2738,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>   
>   	ufshcd_send_command(hba, tag);
>   out:
> -	up_read(&hba->clk_scaling_lock);
> +	rcu_read_unlock();
>   
>   	if (ufs_trigger_eh()) {
>   		unsigned long flags;
> @@ -5977,8 +5935,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>   	}
>   	ufshcd_scsi_block_requests(hba);
>   	/* Drain ufshcd_queuecommand() */
> -	down_write(&hba->clk_scaling_lock);
> -	up_write(&hba->clk_scaling_lock);
> +	synchronize_rcu();
>   	cancel_work_sync(&hba->eeh_work);
>   }
>   
> @@ -6582,6 +6539,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>   	req->end_io_data = &wait;
>   	ufshcd_hold(hba, false);
>   
> +	rcu_read_lock();
> +
>   	spin_lock_irqsave(host->host_lock, flags);
>   
>   	task_tag = req->tag;
> @@ -6600,6 +6559,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>   
>   	spin_unlock_irqrestore(host->host_lock, flags);
>   
> +	rcu_read_unlock();
> +
>   	ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_SEND);
>   
>   	/* wait until the task management command is completed */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 65178487adf3..7afe818ab1e3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -778,6 +778,7 @@ struct ufs_hba_monitor {
>    * @clk_list_head: UFS host controller clocks list node head
>    * @pwr_info: holds current power mode
>    * @max_pwr_info: keeps the device max valid pwm
> + * @clk_scaling_lock: used to serialize device commands and clock scaling
>    * @desc_size: descriptor sizes reported by device
>    * @urgent_bkops_lvl: keeps track of urgent bkops level for device
>    * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
> 


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

Re: [PATCH 04/11] scsi: ufs: Remove dead code

[Bean Huo]

On Tue, 2021-11-09 at 16:44 -0800, Bart Van Assche wrote:
> Commit 7252a3603015 ("scsi: ufs: Avoid busy-waiting by eliminating
> tag
> conflicts") guarantees that 'tag' is not in use by any SCSI command.
> Remove the check that returns early if a conflict occurs.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index dff76b1a0d5d..312e8a5b7733 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6724,11 +6724,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>  	tag = req->tag;
>  	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
>  
> -	if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
> -		err = -EBUSY;
> -		goto out;
> -	}
> -
>  	lrbp = &hba->lrb[tag];
>  	WARN_ON(lrbp->cmd);
>  	lrbp->cmd = NULL;
> @@ -6796,7 +6791,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>  	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
> UFS_QUERY_COMP,
>  				    (struct utp_upiu_req *)lrbp-
> >ucd_rsp_ptr);
>  
> -out:
>  	blk_put_request(req);

Hi Bart,

Doesn't it need to be paired with blk_get_request()?
Kind regards,
Bean

>  out_unlock:
>  	up_read(&hba->clk_scaling_lock);

Re: [PATCH 04/11] scsi: ufs: Remove dead code

[Bean Huo]

On Tue, 2021-11-09 at 16:44 -0800, Bart Van Assche wrote:
> Commit 7252a3603015 ("scsi: ufs: Avoid busy-waiting by eliminating
> tag
> 
> conflicts") guarantees that 'tag' is not in use by any SCSI command.
> 
> Remove the check that returns early if a conflict occurs.
> 
> 
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Ignore my previous email.

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

Re: [PATCH 06/11] scsi: ufs: Rework ufshcd_change_queue_depth()

[Bart Van Assche]

On 11/10/21 11:22 PM, Avri Altman wrote:
>>
>> Prepare for making sdev->host->can_queue less than hba->nutrs. This patch
>> does not change any functionality.
 >
> ufshcd_set_queue_depth() may also needs similar adjustments?

That's a great question. I will modify scsi_change_queue_depth() such 
that it limits the queue depth to host->can_queue.

Thanks,

Bart.

Re: [PATCH 07/11] scsi: ufs: Fix a deadlock in the error handler

[Bart Van Assche]

On 11/9/21 10:42 PM, Christoph Hellwig wrote:
> as pointed out last time: LLDDS have no business directy allocating
> tags.  Please work with Hannes and John on the proper APIs, as metnioned
> last time as well.

Hi Christoph,

I will do this.

Thanks,

Bart.

Re: [PATCH 07/11] scsi: ufs: Fix a deadlock in the error handler

[Bart Van Assche]

On 11/10/21 11:33 PM, Avri Altman wrote:
>> @@ -8152,7 +8147,8 @@ static struct scsi_host_template
>> ufshcd_driver_template = {
>>          .this_id                = -1,
>>          .sg_tablesize           = SG_ALL,
>>          .cmd_per_lun            = UFSHCD_CMD_PER_LUN,
 >
> UFSHCD_CMD_PER_LUN needs to be < 32 as well?

I will make that change although that change should not affect the 
behavior of the UFS driver since the SCSI core limits cmd_per_lun to 
host->can_queue.

Thanks,

Bart.

Re: [PATCH 09/11] scsi: ufs: Fix a kernel crash during shutdown

[Bart Van Assche]

On 11/10/21 11:48 PM, Avri Altman wrote:
>> @@ -1961,11 +1962,15 @@ static void ufshcd_exit_clk_gating(struct ufs_hba
>> *hba)  {
>>          if (!hba->clk_gating.is_initialized)
>>                  return;
>> +
>>          ufshcd_remove_clk_gating_sysfs(hba);
>> -       cancel_work_sync(&hba->clk_gating.ungate_work);
>> -       cancel_delayed_work_sync(&hba->clk_gating.gate_work);
>> -       destroy_workqueue(hba->clk_gating.clk_gating_workq);
>> +
>> +       /* Ungate the clock if necessary. */
>> +       ufshcd_hold(hba, false);
>>          hba->clk_gating.is_initialized = false;
>> +       ufshcd_release(hba);
 >
> Not sure that the symmetry in calling ufshcd_release() is meaningful here?
> You don't really want to queue a new gate work, this is why you added !.is_initialized to release(),
> Or did I get it wrong?

Hi Avri,

The purpose of the above changes is as follows:
* Call ufshcd_hold() to ungate the clock and also to wait for any 
pending work.
* Change hba->clk_gating.is_initialized from true to false to make sure 
that the clock remains ungated and also that no new work is queued.
* Call ufshcd_release() to decrement hba->clk_gating.active_reqs.

Thanks,

Bart.

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Bart Van Assche]

On 11/12/21 2:56 AM, Adrian Hunter wrote:
> On 10/11/2021 20:56, Bart Van Assche wrote:
>> On 11/10/21 12:57 AM, Adrian Hunter wrote:
>>> Seems like something ufshcd_clear_cmd() should be doing instead?
>>
>> I'm concerned that would break ufshcd_eh_device_reset_handler()
 >> since that reset handler retries SCSI commands by calling
 >> __ufshcd_transfer_req_compl() after having called ufshcd_clear_cmd().
> Whenever an outstanding_reqs bit transitions 1 -> 0, then
> __ufshcd_transfer_req_compl() must be called.

I will look further into this.

> As a separate issue, in ufshcd_abort() there is:
> 
> 	/* If command is already aborted/completed, return FAILED. */
> 	if (!(test_bit(tag, &hba->outstanding_reqs))) {
> 		dev_err(hba->dev,
> 			"%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
> 			__func__, tag, hba->outstanding_reqs, reg);
> 		goto release;
> 	}
> 
> which seems wrong. FAILED should only be returned to escalate the
> error handling, so if the slot has already successfully been
> cleared, that is SUCCESS.  scsi_times_out() has already blocked
> the scsi_done() path (by setting SCMD_STATE_COMPLETE), so any use
> after free must be being caused by SCSI not the ufs driver.

scmd_eh_abort_handler() would trigger a use-after-free if ufshcd_abort() 
would return SUCCESS for completed commands. Hence the choice for the 
return value FAILED for completed commands.

BTW, can this code path ever be reached since scsi_done() sets the 
SCMD_STATE_COMPLETE bit before it calls blk_mq_complete_request() and 
since scsi_times_out() tests that bit before it schedules a call of 
ufshcd_abort()?

Thanks,

Bart.

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Adrian Hunter]

On 16/11/2021 01:09, Bart Van Assche wrote:
> On 11/12/21 2:56 AM, Adrian Hunter wrote:
>> On 10/11/2021 20:56, Bart Van Assche wrote:
>>> On 11/10/21 12:57 AM, Adrian Hunter wrote:
>>>> Seems like something ufshcd_clear_cmd() should be doing instead?
>>>
>>> I'm concerned that would break ufshcd_eh_device_reset_handler()
>>> since that reset handler retries SCSI commands by calling
>>> __ufshcd_transfer_req_compl() after having called ufshcd_clear_cmd().
>> Whenever an outstanding_reqs bit transitions 1 -> 0, then
>> __ufshcd_transfer_req_compl() must be called.
> 
> I will look further into this.
> 
>> As a separate issue, in ufshcd_abort() there is:
>>
>>     /* If command is already aborted/completed, return FAILED. */
>>     if (!(test_bit(tag, &hba->outstanding_reqs))) {
>>         dev_err(hba->dev,
>>             "%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
>>             __func__, tag, hba->outstanding_reqs, reg);
>>         goto release;
>>     }
>>
>> which seems wrong. FAILED should only be returned to escalate the
>> error handling, so if the slot has already successfully been
>> cleared, that is SUCCESS.  scsi_times_out() has already blocked
>> the scsi_done() path (by setting SCMD_STATE_COMPLETE), so any use
>> after free must be being caused by SCSI not the ufs driver.
> 
> scmd_eh_abort_handler() would trigger a use-after-free if ufshcd_abort() would return SUCCESS for completed commands. Hence the choice for the return value FAILED for completed commands.
> 
> BTW, can this code path ever be reached since scsi_done() sets the SCMD_STATE_COMPLETE bit before it calls blk_mq_complete_request() and since scsi_times_out() tests that bit before it schedules a call of ufshcd_abort()?

Racing with timing out, the UFS interrupt handler can clear hba->outstanding_reqs bit and call __ufshcd_transfer_req_compl(), but the request will not be freed if scsi_times_out() wins the race.  So there should be no use-after-free unless SCSI error handling itself has a bug.

One perhaps unrelated issue in scsi_times_out():

		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
			return BLK_EH_RESET_TIMER;

Shouldn't that return BLK_EH_DONE not BLK_EH_RESET_TIMER, since the request has been through blk_mq_complete_request() ?

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Peter Wang]

On 11/11/21 5:17 PM, Peter Wang wrote:
> Hi Bart,
>
> On 11/10/21 8:44 AM, Bart Van Assche wrote:
>> Make sure that aborted commands are completed once by clearing the
>> corresponding tag bit from hba->outstanding_reqs. This patch is a
>> follow-up for commit cd892096c940 ("scsi: ufs: core: Improve SCSI
>> abort handling").
>>
>> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/ufs/ufshcd.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 8f5640647054..1e15ed1f639f 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7090,6 +7090,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>>           goto release;
>>       }
>>   +    /*
>> +     * ufshcd_try_to_abort_task() cleared the 'tag' bit in the doorbell
>> +     * register. Clear the corresponding bit from outstanding_reqs to
>> +     * prevent early completion.
>> +     */
>> +    spin_lock_irqsave(&hba->outstanding_lock, flags);
>> +    __clear_bit(tag, &hba->outstanding_reqs);
>> +    spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>> +
>
> should we also call unmap?
>
>     scsi_dma_unmap(cmd);
>
Hi Bart,

Should we add unmap?


Thanks.

Peter


>
>>       lrbp->cmd = NULL;
>>       err = SUCCESS;

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Bart Van Assche]

On 11/16/21 1:03 AM, Adrian Hunter wrote:
> One perhaps unrelated issue in scsi_times_out():
> 
> 		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
> 			return BLK_EH_RESET_TIMER;
> 
> Shouldn't that return BLK_EH_DONE not BLK_EH_RESET_TIMER, since the request has been through blk_mq_complete_request() ?

I think so. I will submit a patch.

Bart.

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Bart Van Assche]

On 11/16/21 1:07 AM, Peter Wang wrote:
> Should we add unmap?

Hi Peter,

I will add DMA unmapping code in the abort handler.

Thanks,

Bart.

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Adrian Hunter]

On 16/11/2021 18:08, Bart Van Assche wrote:
> On 11/16/21 1:07 AM, Peter Wang wrote:
>> Should we add unmap?
> 
> Hi Peter,
> 
> I will add DMA unmapping code in the abort handler.

I would note that __ufshcd_transfer_req_compl() does that, as well as providing
a matching ufshcd_release() for the ufshcd_hold() in ufshcd_queuecommand(), so
do consider __ufshcd_transfer_req_compl().

Using __ufshcd_transfer_req_compl() seems consistent with the error handler which
uses __ufshcd_transfer_req_compl() via ufshcd_complete_requests(), which will pick
up all the requests that the error handler has just aborted via
ufshcd_try_to_abort_task().  Also ufshcd_host_reset_and_restore() uses
__ufshcd_transfer_req_compl() via ufshcd_complete_requests(), which will pick
up anything still in outstanding_reqs because the doorbell has become zero at
that point.

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Bart Van Assche]

On 11/16/21 12:16, Adrian Hunter wrote:
> On 16/11/2021 18:08, Bart Van Assche wrote:
>> On 11/16/21 1:07 AM, Peter Wang wrote:
>>> Should we add unmap?
>>
>> Hi Peter,
>>
>> I will add DMA unmapping code in the abort handler.
> 
> I would note that __ufshcd_transfer_req_compl() does that, as well as providing
> a matching ufshcd_release() for the ufshcd_hold() in ufshcd_queuecommand(), so
> do consider __ufshcd_transfer_req_compl().
> 
> Using __ufshcd_transfer_req_compl() seems consistent with the error handler which
> uses __ufshcd_transfer_req_compl() via ufshcd_complete_requests(), which will pick
> up all the requests that the error handler has just aborted via
> ufshcd_try_to_abort_task().  Also ufshcd_host_reset_and_restore() uses
> __ufshcd_transfer_req_compl() via ufshcd_complete_requests(), which will pick
> up anything still in outstanding_reqs because the doorbell has become zero at
> that point.

Hi Adrian,

Although I agree with minimizing code duplication, I'm not sure that 
__ufshcd_transfer_req_compl() should be called from inside 
ufshcd_abort(). __ufshcd_transfer_req_compl() also calls 
ufshcd_update_monitor() and ufshcd_add_command_trace(hba, index, 
UFS_CMD_COMP). Neither function should be called when aborting a command.

Thanks,

Bart.

Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Adrian Hunter]

On 16/11/2021 23:53, Bart Van Assche wrote:
> On 11/16/21 12:16, Adrian Hunter wrote:
>> On 16/11/2021 18:08, Bart Van Assche wrote:
>>> On 11/16/21 1:07 AM, Peter Wang wrote:
>>>> Should we add unmap?
>>>
>>> Hi Peter,
>>>
>>> I will add DMA unmapping code in the abort handler.
>>
>> I would note that __ufshcd_transfer_req_compl() does that, as well as providing
>> a matching ufshcd_release() for the ufshcd_hold() in ufshcd_queuecommand(), so
>> do consider __ufshcd_transfer_req_compl().
>>
>> Using __ufshcd_transfer_req_compl() seems consistent with the error handler which
>> uses __ufshcd_transfer_req_compl() via ufshcd_complete_requests(), which will pick
>> up all the requests that the error handler has just aborted via
>> ufshcd_try_to_abort_task().  Also ufshcd_host_reset_and_restore() uses
>> __ufshcd_transfer_req_compl() via ufshcd_complete_requests(), which will pick
>> up anything still in outstanding_reqs because the doorbell has become zero at
>> that point.
> 
> Hi Adrian,
> 
> Although I agree with minimizing code duplication, I'm not sure that __ufshcd_transfer_req_compl() should be called from inside ufshcd_abort(). __ufshcd_transfer_req_compl() also calls ufshcd_update_monitor() and ufshcd_add_command_trace(hba, index, UFS_CMD_COMP). Neither function should be called when aborting a command.

But we should trace the abort I would have thought - seems like a separate issue though.

Re: [PATCH 11/11] scsi: ufs: Implement polling support

[Bart Van Assche]

On 11/11/21 00:11, Avri Altman wrote:
> Also I think this should be a separate patch as well.

I see an 8x IOPS improvement on my test setup (2736 IOPS with interrupt based
completions; 22000 IOPS when using polling).

>> +       .mq_poll                = ufshcd_poll,
> Did you consider to use some form blk_mq_tagset_busy_iter,
> And return nutrs - busy?

Hmm ... wouldn't it be racy to use blk_mq_tagset_busy_iter() inside ufshcd_poll()
since multiple threads may be polling at the same time?

Thanks,

Bart.

Re: [PATCH 11/11] scsi: ufs: Implement polling support

[Bart Van Assche]

On 11/9/21 17:36, Douglas Gilbert wrote:
> On 2021-11-09 7:44 p.m., Bart Van Assche wrote:
>> The time spent in io_schedule() is significant when submitting direct
>> I/O to a UFS device. Hence this patch that implements polling support.
>> User space software can enable polling by passing the RWF_HIPRI flag to
>> the preadv2() system call or the IORING_SETUP_IOPOLL flag to the
>> io_uring interface.
> 
> There have been some changes recently (i.e. in linux-stable now),
> "HIPRI" seems to be on the out, replaced by "POLLED". [I'm using
> poll_lld in my sg rewrite to refer to this type of polling, as "poll"
> is an overloaded term in the kernel].
> 
> REQ_HIPRI has become REQ_POLLED and blk_poll() is now bio_poll().
> That said RWF_HIPRI is still in fs.h and there is no RWF_POLLED (yet).

Hi Doug,

My reference to RWF_HIPRI in the patch description refers to the flag
defined in the uapi headers. As far as I know that flag is still there:

$ PAGER= git grep define.RWF_HIPRI include/uapi
include/uapi/linux/fs.h:#define RWF_HIPRI       ((__force __kernel_rwf_t)0x00000001)

Thanks,

Bart.