[PATCH v2 0/4] ufs: Do not requeue while ungating the clock

[PATCH v2 0/4] ufs: Do not requeue while ungating the clock

[Bart Van Assche]

Hi Martin,

In the traces we recorded while testing zoned storage we noticed that UFS
commands are requeued while the clock is being ungated. Command requeueing
makes it harder than necessary to preserve the command order. Hence this
patch series that modifies the SCSI core and also the UFS driver such that
clock ungating does not trigger command requeueing.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v1:
- Dropped patch "scsi: ufs: core: Unexport ufshcd_hold() and ufshcd_release()".
- Removed a ufshcd_scsi_block_requests() / ufshcd_scsi_unblock_requests() pair
  from patch "scsi: ufs: Ungate the clock synchronously".

Bart Van Assche (4):
  scsi: core: Rework scsi_host_block()
  scsi: core: Support setting BLK_MQ_F_BLOCKING
  scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag
  scsi: ufs: Ungate the clock synchronously

 drivers/scsi/scsi_lib.c          | 26 ++++++-----
 drivers/ufs/core/ufs-sysfs.c     |  2 +-
 drivers/ufs/core/ufshcd-crypto.c |  2 +-
 drivers/ufs/core/ufshcd-priv.h   |  2 +-
 drivers/ufs/core/ufshcd.c        | 76 ++++++++++----------------------
 include/scsi/scsi_host.h         |  3 ++
 6 files changed, 45 insertions(+), 66 deletions(-)

[PATCH v2 1/4] scsi: core: Rework scsi_host_block()

[Bart Van Assche]

Make scsi_host_block() easier to read by converting it to the widely used
early-return style. See also commit f983622ae605 ("scsi: core: Avoid
calling synchronize_rcu() for each device in scsi_host_block()").

Reviewed-by: Mike Christie <michael.christie@oracle.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Ye Bin <yebin10@huawei.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b7c569a42aa4..758a57616dd3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2939,11 +2939,20 @@ scsi_target_unblock(struct device *dev, enum scsi_device_state new_state)
 }
 EXPORT_SYMBOL_GPL(scsi_target_unblock);
 
+/**
+ * scsi_host_block - Try to transition all logical units to the SDEV_BLOCK state
+ * @shost: device to block
+ *
+ * Pause SCSI command processing for all logical units associated with the SCSI
+ * host and wait until pending scsi_queue_rq() calls have finished.
+ *
+ * Returns zero if successful or a negative error code upon failure.
+ */
 int
 scsi_host_block(struct Scsi_Host *shost)
 {
 	struct scsi_device *sdev;
-	int ret = 0;
+	int ret;
 
 	/*
 	 * Call scsi_internal_device_block_nowait so we can avoid
@@ -2955,7 +2964,7 @@ scsi_host_block(struct Scsi_Host *shost)
 		mutex_unlock(&sdev->state_mutex);
 		if (ret) {
 			scsi_device_put(sdev);
-			break;
+			return ret;
 		}
 	}
 
@@ -2965,10 +2974,9 @@ scsi_host_block(struct Scsi_Host *shost)
 	 */
 	WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);
 
-	if (!ret)
-		synchronize_rcu();
+	synchronize_rcu();
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(scsi_host_block);
 

[PATCH v2 2/4] scsi: core: Support setting BLK_MQ_F_BLOCKING

[Bart Van Assche]

Prepare for adding code in ufshcd_queuecommand() that may sleep. This
patch is similar to a patch posted last year by Mike Christie. See also
https://lore.kernel.org/all/20220308003957.123312-2-michael.christie@oracle.com/

Reviewed-by: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c  | 10 +++-------
 include/scsi/scsi_host.h |  3 +++
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 758a57616dd3..894af68babc2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1982,6 +1982,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 	tag_set->flags = BLK_MQ_F_SHOULD_MERGE;
 	tag_set->flags |=
 		BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy);
+	if (shost->hostt->queuecommand_may_block)
+		tag_set->flags |= BLK_MQ_F_BLOCKING;
 	tag_set->driver_data = shost;
 	if (shost->host_tagset)
 		tag_set->flags |= BLK_MQ_F_TAG_HCTX_SHARED;
@@ -2968,13 +2970,7 @@ scsi_host_block(struct Scsi_Host *shost)
 		}
 	}
 
-	/*
-	 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag so
-	 * calling synchronize_rcu() once is enough.
-	 */
-	WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);
-
-	synchronize_rcu();
+	blk_mq_wait_quiesce_done(&shost->tag_set);
 
 	return 0;
 }
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 0f29799efa02..37a8a2608dc2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -458,6 +458,9 @@ struct scsi_host_template {
 	/* True if the host uses host-wide tagspace */
 	unsigned host_tagset:1;
 
+	/* The queuecommand callback may block. See also BLK_MQ_F_BLOCKING. */
+	unsigned queuecommand_may_block:1;
+
 	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */

[PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Bart Van Assche]

Prepare for adding code in ufshcd_queuecommand() that may sleep.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7ee150d67d49..993034ac1696 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8756,6 +8756,7 @@ static const struct scsi_host_template ufshcd_driver_template = {
 	.max_host_blocked	= 1,
 	.track_queue_depth	= 1,
 	.skip_settle_delay	= 1,
+	.queuecommand_may_block = true,
 	.sdev_groups		= ufshcd_driver_groups,
 	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
 };

[PATCH v2 4/4] scsi: ufs: Ungate the clock synchronously

[Bart Van Assche]

Ungating the clock asynchronously causes ufshcd_queuecommand() to
return SCSI_MLQUEUE_HOST_BUSY and hence causes commands to be requeued.
This is suboptimal. Allow ufshcd_queuecommand() to sleep such that
clock ungating does not trigger command requeuing. Remove the
ufshcd_scsi_block_requests() and ufshcd_scsi_unblock_requests() because
these are no longer needed. The flush_work(&hba->clk_gating.ungate_work)
call is sufficient to make the SCSI core wait for clock ungating to
complete.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-sysfs.c     |  2 +-
 drivers/ufs/core/ufshcd-crypto.c |  2 +-
 drivers/ufs/core/ufshcd-priv.h   |  2 +-
 drivers/ufs/core/ufshcd.c        | 84 ++++++++++----------------------
 include/ufs/ufshcd.h             |  2 +-
 5 files changed, 30 insertions(+), 62 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 883f0e44b54e..cdf3d5f2b77b 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -168,7 +168,7 @@ static ssize_t auto_hibern8_show(struct device *dev,
 	}
 
 	pm_runtime_get_sync(hba->dev);
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	ahit = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
 	ufshcd_release(hba);
 	pm_runtime_put_sync(hba->dev);
diff --git a/drivers/ufs/core/ufshcd-crypto.c b/drivers/ufs/core/ufshcd-crypto.c
index 198360fe5e8e..f2c4422cab86 100644
--- a/drivers/ufs/core/ufshcd-crypto.c
+++ b/drivers/ufs/core/ufshcd-crypto.c
@@ -24,7 +24,7 @@ static int ufshcd_program_key(struct ufs_hba *hba,
 	u32 slot_offset = hba->crypto_cfg_register + slot * sizeof(*cfg);
 	int err = 0;
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 
 	if (hba->vops && hba->vops->program_key) {
 		err = hba->vops->program_key(hba, cfg, slot);
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index d53b93c21a0c..22cac71090ae 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -84,7 +84,7 @@ unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
 int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
 			    u8 **buf, bool ascii);
 
-int ufshcd_hold(struct ufs_hba *hba, bool async);
+void ufshcd_hold(struct ufs_hba *hba);
 void ufshcd_release(struct ufs_hba *hba);
 
 int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 993034ac1696..9736b2b4120e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1189,7 +1189,7 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 	bool timeout = false, do_last_check = false;
 	ktime_t start;
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	/*
 	 * Wait for all the outstanding tasks/transfer requests.
@@ -1310,7 +1310,7 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba, u64 timeout_us)
 	}
 
 	/* let's not get into low power until clock scaling is completed */
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 
 out:
 	return ret;
@@ -1640,7 +1640,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 		goto out;
 
 	ufshcd_rpm_get_sync(hba);
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 
 	hba->clk_scaling.is_enabled = value;
 
@@ -1723,7 +1723,7 @@ static void ufshcd_ungate_work(struct work_struct *work)
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	if (hba->clk_gating.state == CLKS_ON) {
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
-		goto unblock_reqs;
+		return;
 	}
 
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
@@ -1746,25 +1746,21 @@ static void ufshcd_ungate_work(struct work_struct *work)
 		}
 		hba->clk_gating.is_suspended = false;
 	}
-unblock_reqs:
-	ufshcd_scsi_unblock_requests(hba);
 }
 
 /**
  * ufshcd_hold - Enable clocks that were gated earlier due to ufshcd_release.
  * Also, exit from hibern8 mode and set the link as active.
  * @hba: per adapter instance
- * @async: This indicates whether caller should ungate clocks asynchronously.
  */
-int ufshcd_hold(struct ufs_hba *hba, bool async)
+void ufshcd_hold(struct ufs_hba *hba)
 {
-	int rc = 0;
 	bool flush_result;
 	unsigned long flags;
 
 	if (!ufshcd_is_clkgating_allowed(hba) ||
 	    !hba->clk_gating.is_initialized)
-		goto out;
+		return;
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->clk_gating.active_reqs++;
 
@@ -1781,15 +1777,10 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		 */
 		if (ufshcd_can_hibern8_during_gating(hba) &&
 		    ufshcd_is_link_hibern8(hba)) {
-			if (async) {
-				rc = -EAGAIN;
-				hba->clk_gating.active_reqs--;
-				break;
-			}
 			spin_unlock_irqrestore(hba->host->host_lock, flags);
 			flush_result = flush_work(&hba->clk_gating.ungate_work);
 			if (hba->clk_gating.is_suspended && !flush_result)
-				goto out;
+				return;
 			spin_lock_irqsave(hba->host->host_lock, flags);
 			goto start;
 		}
@@ -1811,21 +1802,14 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		hba->clk_gating.state = REQ_CLKS_ON;
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
-		if (queue_work(hba->clk_gating.clk_gating_workq,
-			       &hba->clk_gating.ungate_work))
-			ufshcd_scsi_block_requests(hba);
+		queue_work(hba->clk_gating.clk_gating_workq,
+			   &hba->clk_gating.ungate_work);
 		/*
 		 * fall through to check if we should wait for this
 		 * work to be done or not.
 		 */
 		fallthrough;
 	case REQ_CLKS_ON:
-		if (async) {
-			rc = -EAGAIN;
-			hba->clk_gating.active_reqs--;
-			break;
-		}
-
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
 		flush_work(&hba->clk_gating.ungate_work);
 		/* Make sure state is CLKS_ON before returning */
@@ -1837,8 +1821,6 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		break;
 	}
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-out:
-	return rc;
 }
 EXPORT_SYMBOL_GPL(ufshcd_hold);
 
@@ -2070,7 +2052,7 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
 	ufshcd_remove_clk_gating_sysfs(hba);
 
 	/* Ungate the clock if necessary. */
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	hba->clk_gating.is_initialized = false;
 	ufshcd_release(hba);
 
@@ -2466,7 +2448,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD)
 		return 0;
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	mutex_lock(&hba->uic_cmd_mutex);
 	ufshcd_add_delay_before_dme_cmd(hba);
 
@@ -2868,12 +2850,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
 
-	/*
-	 * Allows the UFS error handler to wait for prior ufshcd_queuecommand()
-	 * calls.
-	 */
-	rcu_read_lock();
-
 	switch (hba->ufshcd_state) {
 	case UFSHCD_STATE_OPERATIONAL:
 		break;
@@ -2919,13 +2895,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	hba->req_abort_count = 0;
 
-	err = ufshcd_hold(hba, true);
-	if (err) {
-		err = SCSI_MLQUEUE_HOST_BUSY;
-		goto out;
-	}
-	WARN_ON(ufshcd_is_clkgating_allowed(hba) &&
-		(hba->clk_gating.state != CLKS_ON));
+	ufshcd_hold(hba);
 
 	lrbp = &hba->lrb[tag];
 	lrbp->cmd = cmd;
@@ -2953,8 +2923,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	ufshcd_send_command(hba, tag, hwq);
 
 out:
-	rcu_read_unlock();
-
 	if (ufs_trigger_eh()) {
 		unsigned long flags;
 
@@ -3248,7 +3216,7 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 
 	BUG_ON(!hba);
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	mutex_lock(&hba->dev_cmd.lock);
 	ufshcd_init_query(hba, &request, &response, opcode, idn, index,
 			selector);
@@ -3322,7 +3290,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
 		return -EINVAL;
 	}
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 
 	mutex_lock(&hba->dev_cmd.lock);
 	ufshcd_init_query(hba, &request, &response, opcode, idn, index,
@@ -3418,7 +3386,7 @@ static int __ufshcd_query_descriptor(struct ufs_hba *hba,
 		return -EINVAL;
 	}
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 
 	mutex_lock(&hba->dev_cmd.lock);
 	ufshcd_init_query(hba, &request, &response, opcode, idn, index,
@@ -4236,7 +4204,7 @@ int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 	uic_cmd.command = UIC_CMD_DME_SET;
 	uic_cmd.argument1 = UIC_ARG_MIB(PA_PWRMODE);
 	uic_cmd.argument3 = mode;
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	ufshcd_release(hba);
 
@@ -4343,7 +4311,7 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
 	if (update &&
 	    !pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)) {
 		ufshcd_rpm_get_sync(hba);
-		ufshcd_hold(hba, false);
+		ufshcd_hold(hba);
 		ufshcd_auto_hibern8_enable(hba);
 		ufshcd_release(hba);
 		ufshcd_rpm_put_sync(hba);
@@ -4936,7 +4904,7 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
 	int err = 0;
 	int retries;
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	mutex_lock(&hba->dev_cmd.lock);
 	for (retries = NOP_OUT_RETRIES; retries > 0; retries--) {
 		err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_NOP,
@@ -6221,14 +6189,14 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 		ufshcd_setup_vreg(hba, true);
 		ufshcd_config_vreg_hpm(hba, hba->vreg_info.vccq);
 		ufshcd_config_vreg_hpm(hba, hba->vreg_info.vccq2);
-		ufshcd_hold(hba, false);
+		ufshcd_hold(hba);
 		if (!ufshcd_is_clkgating_allowed(hba))
 			ufshcd_setup_clocks(hba, true);
 		ufshcd_release(hba);
 		pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM;
 		ufshcd_vops_resume(hba, pm_op);
 	} else {
-		ufshcd_hold(hba, false);
+		ufshcd_hold(hba);
 		if (ufshcd_is_clkscaling_supported(hba) &&
 		    hba->clk_scaling.is_enabled)
 			ufshcd_suspend_clkscaling(hba);
@@ -6236,7 +6204,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 	}
 	ufshcd_scsi_block_requests(hba);
 	/* Drain ufshcd_queuecommand() */
-	synchronize_rcu();
+	blk_mq_wait_quiesce_done(&hba->host->tag_set);
 	cancel_work_sync(&hba->eeh_work);
 }
 
@@ -6881,7 +6849,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 		return PTR_ERR(req);
 
 	req->end_io_data = &wait;
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 
 	spin_lock_irqsave(host->host_lock, flags);
 
@@ -7117,7 +7085,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 		cmd_type = DEV_CMD_TYPE_NOP;
 		fallthrough;
 	case UPIU_TRANSACTION_QUERY_REQ:
-		ufshcd_hold(hba, false);
+		ufshcd_hold(hba);
 		mutex_lock(&hba->dev_cmd.lock);
 		err = ufshcd_issue_devman_upiu_cmd(hba, req_upiu, rsp_upiu,
 						   desc_buff, buff_len,
@@ -7183,7 +7151,7 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 	u16 ehs_len;
 
 	/* Protects use of hba->reserved_slot. */
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	mutex_lock(&hba->dev_cmd.lock);
 	down_read(&hba->clk_scaling_lock);
 
@@ -7417,7 +7385,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
 
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	/* If command is already aborted/completed, return FAILED. */
 	if (!(test_bit(tag, &hba->outstanding_reqs))) {
@@ -9410,7 +9378,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	 * If we can't transition into any of the low power modes
 	 * just gate the clocks.
 	 */
-	ufshcd_hold(hba, false);
+	ufshcd_hold(hba);
 	hba->clk_gating.is_suspended = true;
 
 	if (ufshcd_is_clkscaling_supported(hba))
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index db2e669985d5..40c537a3880b 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1357,7 +1357,7 @@ void ufshcd_fixup_dev_quirks(struct ufs_hba *hba,
 int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
 			    u8 **buf, bool ascii);
 
-int ufshcd_hold(struct ufs_hba *hba, bool async);
+void ufshcd_hold(struct ufs_hba *hba);
 void ufshcd_release(struct ufs_hba *hba);
 
 void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value);

Re: [PATCH v2 1/4] scsi: core: Rework scsi_host_block()

[Ming Lei]

On Wed, May 17, 2023 at 03:23:56PM -0700, Bart Van Assche wrote:
> Make scsi_host_block() easier to read by converting it to the widely used
> early-return style. See also commit f983622ae605 ("scsi: core: Avoid
> calling synchronize_rcu() for each device in scsi_host_block()").
> 
> Reviewed-by: Mike Christie <michael.christie@oracle.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Ye Bin <yebin10@huawei.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

Re: [PATCH v2 0/4] ufs: Do not requeue while ungating the clock

[Bart Van Assche]

On 5/17/23 15:23, Bart Van Assche wrote:
> In the traces we recorded while testing zoned storage we noticed that UFS
> commands are requeued while the clock is being ungated. Command requeueing
> makes it harder than necessary to preserve the command order. Hence this
> patch series that modifies the SCSI core and also the UFS driver such that
> clock ungating does not trigger command requeueing.

Can anyone please help with reviewing this patch series? I will be OoO in
June so it would help if review comments would be posted soon.

Thanks,

Bart.

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Adrian Hunter]

On 18/05/23 01:23, Bart Van Assche wrote:
> Prepare for adding code in ufshcd_queuecommand() that may sleep.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 7ee150d67d49..993034ac1696 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8756,6 +8756,7 @@ static const struct scsi_host_template ufshcd_driver_template = {
>  	.max_host_blocked	= 1,
>  	.track_queue_depth	= 1,
>  	.skip_settle_delay	= 1,
> +	.queuecommand_may_block = true,

Shouldn't this only be for controllers that support
clock gating?

>  	.sdev_groups		= ufshcd_driver_groups,
>  	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
>  };
> 

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Bart Van Assche]

On 5/23/23 09:39, Adrian Hunter wrote:
> On 18/05/23 01:23, Bart Van Assche wrote:
>> Prepare for adding code in ufshcd_queuecommand() that may sleep.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/ufs/core/ufshcd.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 7ee150d67d49..993034ac1696 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -8756,6 +8756,7 @@ static const struct scsi_host_template ufshcd_driver_template = {
>>   	.max_host_blocked	= 1,
>>   	.track_queue_depth	= 1,
>>   	.skip_settle_delay	= 1,
>> +	.queuecommand_may_block = true,
> 
> Shouldn't this only be for controllers that support
> clock gating?

Hi Adrian,

The overhead of BLK_MQ_F_BLOCKING is small relative to the time required to
queue a UFS command so I think enabling BLK_MQ_F_BLOCKING for all UFS host
controllers is fine. BLK_MQ_F_BLOCKING causes the block layer to use SRCU
instead of RCU. The cost of the sleepable RCU primitives is dominated by
the memory barrier (smp_mb()) in the srcu_read_lock() and srcu_read_unlock()
calls. From kernel/rcu/srcutree.c:

int __srcu_read_lock(struct srcu_struct *ssp)
{
	int idx;

	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
	smp_mb(); /* B */  /* Avoid leaking the critical section. */
	return idx;
}
EXPORT_SYMBOL_GPL(__srcu_read_lock);

void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
{
	smp_mb(); /* C */  /* Avoid leaking the critical section. */
	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
}
EXPORT_SYMBOL_GPL(__srcu_read_unlock);

The rcu_read_lock() and rcu_read_lock() implementations do not call
smp_mb() as one can see in include/linux/rcupdate.h:

static inline void __rcu_read_lock(void)
{
	preempt_disable();
}

static inline void __rcu_read_unlock(void)
{
	preempt_enable();
	if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD))
		rcu_read_unlock_strict();
}

Thanks,

Bart.

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Adrian Hunter]

On 23/05/23 20:10, Bart Van Assche wrote:
> On 5/23/23 09:39, Adrian Hunter wrote:
>> On 18/05/23 01:23, Bart Van Assche wrote:
>>> Prepare for adding code in ufshcd_queuecommand() that may sleep.
>>>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   drivers/ufs/core/ufshcd.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>> index 7ee150d67d49..993034ac1696 100644
>>> --- a/drivers/ufs/core/ufshcd.c
>>> +++ b/drivers/ufs/core/ufshcd.c
>>> @@ -8756,6 +8756,7 @@ static const struct scsi_host_template ufshcd_driver_template = {
>>>       .max_host_blocked    = 1,
>>>       .track_queue_depth    = 1,
>>>       .skip_settle_delay    = 1,
>>> +    .queuecommand_may_block = true,
>>
>> Shouldn't this only be for controllers that support
>> clock gating?
> 
> Hi Adrian,
> 
> The overhead of BLK_MQ_F_BLOCKING is small relative to the time required to
> queue a UFS command so I think enabling BLK_MQ_F_BLOCKING for all UFS host
> controllers is fine.

Doesn't it also force the queue to be run asynchronously always?

But in any case, it doesn't seem like something to force on drivers
just because it would take a bit more coding to make it optional.


>                      BLK_MQ_F_BLOCKING causes the block layer to use SRCU
> instead of RCU. The cost of the sleepable RCU primitives is dominated by
> the memory barrier (smp_mb()) in the srcu_read_lock() and srcu_read_unlock()
> calls. From kernel/rcu/srcutree.c:
> 
> int __srcu_read_lock(struct srcu_struct *ssp)
> {
>     int idx;
> 
>     idx = READ_ONCE(ssp->srcu_idx) & 0x1;
>     this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
>     smp_mb(); /* B */  /* Avoid leaking the critical section. */
>     return idx;
> }
> EXPORT_SYMBOL_GPL(__srcu_read_lock);
> 
> void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
> {
>     smp_mb(); /* C */  /* Avoid leaking the critical section. */
>     this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
> }
> EXPORT_SYMBOL_GPL(__srcu_read_unlock);
> 
> The rcu_read_lock() and rcu_read_lock() implementations do not call
> smp_mb() as one can see in include/linux/rcupdate.h:
> 
> static inline void __rcu_read_lock(void)
> {
>     preempt_disable();
> }
> 
> static inline void __rcu_read_unlock(void)
> {
>     preempt_enable();
>     if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD))
>         rcu_read_unlock_strict();
> }
> 
> Thanks,
> 
> Bart.
> 

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Bart Van Assche]

On 5/23/23 12:19, Adrian Hunter wrote:
> On 23/05/23 20:10, Bart Van Assche wrote:
>> The overhead of BLK_MQ_F_BLOCKING is small relative to the time required to
>> queue a UFS command so I think enabling BLK_MQ_F_BLOCKING for all UFS host
>> controllers is fine.
> 
> Doesn't it also force the queue to be run asynchronously always?
> 
> But in any case, it doesn't seem like something to force on drivers
> just because it would take a bit more coding to make it optional.
Making BLK_MQ_F_BLOCKING optional would complicate testing of the UFS 
driver. Although it is possible to make BLK_MQ_F_BLOCKING optional, I'm 
wondering whether it is worth it? I haven't noticed any performance 
difference in my tests with BLK_MQ_F_BLOCKING enabled compared to 
BLK_MQ_F_BLOCKING disabled.

Thanks,

Bart.

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Adrian Hunter]

On 23/05/23 22:57, Bart Van Assche wrote:
> On 5/23/23 12:19, Adrian Hunter wrote:
>> On 23/05/23 20:10, Bart Van Assche wrote:
>>> The overhead of BLK_MQ_F_BLOCKING is small relative to the time required to
>>> queue a UFS command so I think enabling BLK_MQ_F_BLOCKING for all UFS host
>>> controllers is fine.
>>
>> Doesn't it also force the queue to be run asynchronously always?
>>
>> But in any case, it doesn't seem like something to force on drivers
>> just because it would take a bit more coding to make it optional.
> Making BLK_MQ_F_BLOCKING optional would complicate testing of the UFS driver. Although it is possible to make BLK_MQ_F_BLOCKING optional, I'm wondering whether it is worth it? I haven't noticed any performance difference in my tests with BLK_MQ_F_BLOCKING enabled compared to BLK_MQ_F_BLOCKING disabled.

It is hard to know the effects, especially wrt to future hardware.

What about something like this?

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 82321b8b32bc..301c0b2a406d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10049,13 +10049,16 @@ static int ufshcd_set_dma_mask(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
+ * __ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
  * @dev: pointer to device handle
  * @hba_handle: driver private handle
+ * @vops: pointer to variant specific operations
  * Returns 0 on success, non-zero value on failure
  */
-int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
+int __ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle,
+			const struct ufs_hba_variant_ops *vops)
 {
+	struct scsi_host_template sht = ufshcd_driver_template;
 	struct Scsi_Host *host;
 	struct ufs_hba *hba;
 	int err = 0;
@@ -10067,8 +10070,10 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 		goto out_error;
 	}
 
-	host = scsi_host_alloc(&ufshcd_driver_template,
-				sizeof(struct ufs_hba));
+	if (vops && vops->nonblocking)
+		sht.queuecommand_may_block = false;
+
+	host = scsi_host_alloc(&sht, sizeof(struct ufs_hba));
 	if (!host) {
 		dev_err(dev, "scsi_host_alloc failed\n");
 		err = -ENOMEM;
@@ -10078,6 +10083,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 	hba = shost_priv(host);
 	hba->host = host;
 	hba->dev = dev;
+	hba->vops = vops;
 	hba->dev_ref_clk_freq = REF_CLK_FREQ_INVAL;
 	hba->nop_out_timeout = NOP_OUT_TIMEOUT;
 	ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
@@ -10089,7 +10095,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 out_error:
 	return err;
 }
-EXPORT_SYMBOL(ufshcd_alloc_host);
+EXPORT_SYMBOL(__ufshcd_alloc_host);
 
 /* This function exists because blk_mq_alloc_tag_set() requires this. */
 static blk_status_t ufshcd_queue_tmf(struct blk_mq_hw_ctx *hctx,
diff --git a/drivers/ufs/host/ufshcd-pci.c b/drivers/ufs/host/ufshcd-pci.c
index 9c911787f84c..ce42b822fcb6 100644
--- a/drivers/ufs/host/ufshcd-pci.c
+++ b/drivers/ufs/host/ufshcd-pci.c
@@ -447,6 +447,7 @@ static int ufs_intel_mtl_init(struct ufs_hba *hba)
 
 static struct ufs_hba_variant_ops ufs_intel_cnl_hba_vops = {
 	.name                   = "intel-pci",
+	.nonblocking		= true,
 	.init			= ufs_intel_common_init,
 	.exit			= ufs_intel_common_exit,
 	.link_startup_notify	= ufs_intel_link_startup_notify,
@@ -455,6 +456,7 @@ static struct ufs_hba_variant_ops ufs_intel_cnl_hba_vops = {
 
 static struct ufs_hba_variant_ops ufs_intel_ehl_hba_vops = {
 	.name                   = "intel-pci",
+	.nonblocking		= true,
 	.init			= ufs_intel_ehl_init,
 	.exit			= ufs_intel_common_exit,
 	.link_startup_notify	= ufs_intel_link_startup_notify,
@@ -463,6 +465,7 @@ static struct ufs_hba_variant_ops ufs_intel_ehl_hba_vops = {
 
 static struct ufs_hba_variant_ops ufs_intel_lkf_hba_vops = {
 	.name                   = "intel-pci",
+	.nonblocking		= true,
 	.init			= ufs_intel_lkf_init,
 	.exit			= ufs_intel_common_exit,
 	.hce_enable_notify	= ufs_intel_hce_enable_notify,
@@ -475,6 +478,7 @@ static struct ufs_hba_variant_ops ufs_intel_lkf_hba_vops = {
 
 static struct ufs_hba_variant_ops ufs_intel_adl_hba_vops = {
 	.name			= "intel-pci",
+	.nonblocking		= true,
 	.init			= ufs_intel_adl_init,
 	.exit			= ufs_intel_common_exit,
 	.link_startup_notify	= ufs_intel_link_startup_notify,
@@ -484,6 +488,7 @@ static struct ufs_hba_variant_ops ufs_intel_adl_hba_vops = {
 
 static struct ufs_hba_variant_ops ufs_intel_mtl_hba_vops = {
 	.name                   = "intel-pci",
+	.nonblocking		= true,
 	.init			= ufs_intel_mtl_init,
 	.exit			= ufs_intel_common_exit,
 	.hce_enable_notify	= ufs_intel_hce_enable_notify,
@@ -538,6 +543,7 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
 static int
 ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	const struct ufs_hba_variant_ops *vops;
 	struct ufs_host *ufs_host;
 	struct ufs_hba *hba;
 	void __iomem *mmio_base;
@@ -559,14 +565,14 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	mmio_base = pcim_iomap_table(pdev)[0];
 
-	err = ufshcd_alloc_host(&pdev->dev, &hba);
+	vops = (const struct ufs_hba_variant_ops *)id->driver_data;
+
+	err = __ufshcd_alloc_host(&pdev->dev, &hba, vops);
 	if (err) {
 		dev_err(&pdev->dev, "Allocation failed\n");
 		return err;
 	}
 
-	hba->vops = (struct ufs_hba_variant_ops *)id->driver_data;
-
 	err = ufshcd_init(hba, mmio_base, pdev->irq);
 	if (err) {
 		dev_err(&pdev->dev, "Initialization failed\n");
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 8039c2b72502..dbf2312ad756 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -274,6 +274,8 @@ struct ufs_pwr_mode_info {
 /**
  * struct ufs_hba_variant_ops - variant specific callbacks
  * @name: variant name
+ * @nonblocking: queuecommand will not block (incompatible with
+ *               UFSHCD_CAP_CLK_GATING)
  * @init: called when the driver is initialized
  * @exit: called to cleanup everything done in init
  * @get_ufs_hci_version: called to get UFS HCI version
@@ -310,6 +312,7 @@ struct ufs_pwr_mode_info {
  */
 struct ufs_hba_variant_ops {
 	const char *name;
+	bool nonblocking;
 	int	(*init)(struct ufs_hba *);
 	void    (*exit)(struct ufs_hba *);
 	u32	(*get_ufs_hci_version)(struct ufs_hba *);
@@ -1225,7 +1228,20 @@ static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
 	ufshcd_writel(hba, tmp, reg);
 }
 
-int ufshcd_alloc_host(struct device *, struct ufs_hba **);
+int __ufshcd_alloc_host(struct device *, struct ufs_hba **,
+			const struct ufs_hba_variant_ops *);
+
+/**
+ * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
+ * @dev: pointer to device handle
+ * @hba_handle: driver private handle
+ * Returns 0 on success, non-zero value on failure
+ */
+static inline int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
+{
+	return __ufshcd_alloc_host(dev, hba_handle, NULL);
+}
+
 void ufshcd_dealloc_host(struct ufs_hba *);
 int ufshcd_hba_enable(struct ufs_hba *hba);
 int ufshcd_init(struct ufs_hba *, void __iomem *, unsigned int);

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Bart Van Assche]

On 5/23/23 22:55, Adrian Hunter wrote:
> On 23/05/23 22:57, Bart Van Assche wrote:
>> On 5/23/23 12:19, Adrian Hunter wrote:
>>> On 23/05/23 20:10, Bart Van Assche wrote:
>>>> The overhead of BLK_MQ_F_BLOCKING is small relative to the
>>>> time required to queue a UFS command so I think enabling 
>>>> BLK_MQ_F_BLOCKING for all UFS host controllers is fine.
>>> 
>>> Doesn't it also force the queue to be run asynchronously always?
>>> 
>>> But in any case, it doesn't seem like something to force on 
>>> drivers just because it would take a bit more coding to make it 
>>> optional.
>> 
>> Making BLK_MQ_F_BLOCKING optional would complicate testing of the 
>> UFS driver. Although it is possible to make BLK_MQ_F_BLOCKING 
>> optional, I'm wondering whether it is worth it? I haven't noticed 
>> any performance difference in my tests with BLK_MQ_F_BLOCKING 
>> enabled compared to BLK_MQ_F_BLOCKING disabled.
> 
> It is hard to know the effects, especially wrt to future hardware.

Are you perhaps referring to performance effects? I think the block
layer can be modified to run the queue synchronously in most cases even
if BLK_MQ_F_BLOCKING is set. The patch below works fine but is probably
not acceptable for upstream because it uses in_atomic():


diff --git a/block/blk-mq.c b/block/blk-mq.c
index 551e7760f45e..00fbe0aa56b5 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1471,9 +1471,23 @@ static void blk_mq_requeue_work(struct work_struct *work)
  	blk_mq_run_hw_queues(q, false);
  }

+static bool may_sleep(void)
+{
+#ifdef CONFIG_PREEMPT_COUNT
+	return !in_atomic() && !irqs_disabled();
+#else
+	return false;
+#endif
+}
+
  void blk_mq_kick_requeue_list(struct request_queue *q)
  {
+	if (may_sleep()) {
+		blk_mq_requeue_work(&q->requeue_work.work);
+		return;
+	}
  	kblockd_mod_delayed_work_on(WORK_CPU_UNBOUND, &q->requeue_work, 0);
+
  }
  EXPORT_SYMBOL(blk_mq_kick_requeue_list);

@@ -2228,7 +2242,7 @@ void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
  	if (!need_run)
  		return;

-	if (async || (hctx->flags & BLK_MQ_F_BLOCKING) ||
+	if (async || (hctx->flags & BLK_MQ_F_BLOCKING && !may_sleep()) ||
  	    !cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask)) {
  		blk_mq_delay_run_hw_queue(hctx, 0);
  		return;


> What about something like this? [ ... ]

This introduces a redundancy and a potential for a conflict between the
"nonblocking" flag and the UFSHCD_CAP_CLK_GATING flag. It is unfortunate
that hba->caps is set so late otherwise we could use the result of
(hba->caps & UFSHCD_CAP_CLK_GATING) to check whether or not
BLK_MQ_F_BLOCKING is needed.

Additionally, this patch introduces a use-after-free issue since it
causes scsi_host_alloc() to store a pointer to a stack variable (sht)
into a SCSI host structure.

Bart.

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Adrian Hunter]

On 26/05/23 00:16, Bart Van Assche wrote:
> On 5/23/23 22:55, Adrian Hunter wrote:
>> On 23/05/23 22:57, Bart Van Assche wrote:
>>> On 5/23/23 12:19, Adrian Hunter wrote:
>>>> On 23/05/23 20:10, Bart Van Assche wrote:
>>>>> The overhead of BLK_MQ_F_BLOCKING is small relative to the
>>>>> time required to queue a UFS command so I think enabling BLK_MQ_F_BLOCKING for all UFS host controllers is fine.
>>>>
>>>> Doesn't it also force the queue to be run asynchronously always?
>>>>
>>>> But in any case, it doesn't seem like something to force on drivers just because it would take a bit more coding to make it optional.
>>>
>>> Making BLK_MQ_F_BLOCKING optional would complicate testing of the UFS driver. Although it is possible to make BLK_MQ_F_BLOCKING optional, I'm wondering whether it is worth it? I haven't noticed any performance difference in my tests with BLK_MQ_F_BLOCKING enabled compared to BLK_MQ_F_BLOCKING disabled.
>>
>> It is hard to know the effects, especially wrt to future hardware.
> 
> Are you perhaps referring to performance effects? I think the block
> layer can be modified to run the queue synchronously in most cases even
> if BLK_MQ_F_BLOCKING is set. The patch below works fine but is probably
> not acceptable for upstream because it uses in_atomic(): [ ... ]

Why would we want to when we don't have to?

>> What about something like this? [ ... ]
> 
> This introduces a redundancy and a potential for a conflict between the
> "nonblocking" flag and the UFSHCD_CAP_CLK_GATING flag. It is unfortunate
> that hba->caps is set so late otherwise we could use the result of
> (hba->caps & UFSHCD_CAP_CLK_GATING) to check whether or not
> BLK_MQ_F_BLOCKING is needed.
> 
> Additionally, this patch introduces a use-after-free issue since it
> causes scsi_host_alloc() to store a pointer to a stack variable (sht)
> into a SCSI host structure.

So with those fixed and the vops->may_block instead of vops->nonblocking:


diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 82321b8b32bc..6a4389f02b4e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2027,6 +2027,11 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 	if (!ufshcd_is_clkgating_allowed(hba))
 		return;
 
+	if (!hba->host->hostt->queuecommand_may_block) {
+		dev_warn(hba->dev, "Nonblocking queue, so clock gating is not enabled!\n");
+		return;
+	}
+
 	hba->clk_gating.state = CLKS_ON;
 
 	hba->clk_gating.delay_ms = 150;
@@ -8708,33 +8713,41 @@ static struct ufs_hba_variant_params ufs_hba_vps = {
 	.ondemand_data.downdifferential	= 5,
 };
 
-static const struct scsi_host_template ufshcd_driver_template = {
-	.module			= THIS_MODULE,
-	.name			= UFSHCD,
-	.proc_name		= UFSHCD,
-	.map_queues		= ufshcd_map_queues,
-	.queuecommand		= ufshcd_queuecommand,
-	.mq_poll		= ufshcd_poll,
-	.slave_alloc		= ufshcd_slave_alloc,
-	.slave_configure	= ufshcd_slave_configure,
-	.slave_destroy		= ufshcd_slave_destroy,
-	.change_queue_depth	= ufshcd_change_queue_depth,
-	.eh_abort_handler	= ufshcd_abort,
-	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
-	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
-	.eh_timed_out		= ufshcd_eh_timed_out,
-	.this_id		= -1,
-	.sg_tablesize		= SG_ALL,
-	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
-	.can_queue		= UFSHCD_CAN_QUEUE,
-	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
-	.max_sectors		= (1 << 20) / SECTOR_SIZE, /* 1 MiB */
-	.max_host_blocked	= 1,
-	.track_queue_depth	= 1,
-	.skip_settle_delay	= 1,
+#define UFSHCD_SHT_COMMON \
+	.module			= THIS_MODULE,				\
+	.name			= UFSHCD,				\
+	.proc_name		= UFSHCD,				\
+	.map_queues		= ufshcd_map_queues,			\
+	.queuecommand		= ufshcd_queuecommand,			\
+	.mq_poll		= ufshcd_poll,				\
+	.slave_alloc		= ufshcd_slave_alloc,			\
+	.slave_configure	= ufshcd_slave_configure,		\
+	.slave_destroy		= ufshcd_slave_destroy,			\
+	.change_queue_depth	= ufshcd_change_queue_depth,		\
+	.eh_abort_handler	= ufshcd_abort,				\
+	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,	\
+	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,	\
+	.eh_timed_out		= ufshcd_eh_timed_out,			\
+	.this_id		= -1,					\
+	.sg_tablesize		= SG_ALL,				\
+	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,			\
+	.can_queue		= UFSHCD_CAN_QUEUE,			\
+	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,		\
+	.max_sectors		= (1 << 20) / SECTOR_SIZE, /* 1 MiB */	\
+	.max_host_blocked	= 1,					\
+	.track_queue_depth	= 1,					\
+	.skip_settle_delay	= 1,					\
+	.sdev_groups		= ufshcd_driver_groups,			\
+	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS
+
+static const struct scsi_host_template ufshcd_sht_blocking = {
+	UFSHCD_SHT_COMMON,
 	.queuecommand_may_block = true,
-	.sdev_groups		= ufshcd_driver_groups,
-	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
+};
+
+static const struct scsi_host_template ufshcd_sht_nonblocking = {
+	UFSHCD_SHT_COMMON,
+	.queuecommand_may_block = false,
 };
 
 static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
@@ -10049,13 +10062,16 @@ static int ufshcd_set_dma_mask(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
+ * __ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
  * @dev: pointer to device handle
  * @hba_handle: driver private handle
+ * @vops: pointer to variant specific operations
  * Returns 0 on success, non-zero value on failure
  */
-int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
+int __ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle,
+			const struct ufs_hba_variant_ops *vops)
 {
+	const struct scsi_host_template *sht;
 	struct Scsi_Host *host;
 	struct ufs_hba *hba;
 	int err = 0;
@@ -10067,8 +10083,12 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 		goto out_error;
 	}
 
-	host = scsi_host_alloc(&ufshcd_driver_template,
-				sizeof(struct ufs_hba));
+	if (vops && vops->may_block)
+		sht = &ufshcd_sht_blocking;
+	else
+		sht = &ufshcd_sht_nonblocking;
+
+	host = scsi_host_alloc(sht, sizeof(struct ufs_hba));
 	if (!host) {
 		dev_err(dev, "scsi_host_alloc failed\n");
 		err = -ENOMEM;
@@ -10078,6 +10098,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 	hba = shost_priv(host);
 	hba->host = host;
 	hba->dev = dev;
+	hba->vops = vops;
 	hba->dev_ref_clk_freq = REF_CLK_FREQ_INVAL;
 	hba->nop_out_timeout = NOP_OUT_TIMEOUT;
 	ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
@@ -10089,7 +10110,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 out_error:
 	return err;
 }
-EXPORT_SYMBOL(ufshcd_alloc_host);
+EXPORT_SYMBOL(__ufshcd_alloc_host);
 
 /* This function exists because blk_mq_alloc_tag_set() requires this. */
 static blk_status_t ufshcd_queue_tmf(struct blk_mq_hw_ctx *hctx,
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 8039c2b72502..dbeb879b47d1 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -274,6 +274,7 @@ struct ufs_pwr_mode_info {
 /**
  * struct ufs_hba_variant_ops - variant specific callbacks
  * @name: variant name
+ * @may_block: queuecommand may block (required with UFSHCD_CAP_CLK_GATING)
  * @init: called when the driver is initialized
  * @exit: called to cleanup everything done in init
  * @get_ufs_hci_version: called to get UFS HCI version
@@ -310,6 +311,7 @@ struct ufs_pwr_mode_info {
  */
 struct ufs_hba_variant_ops {
 	const char *name;
+	bool	may_block;
 	int	(*init)(struct ufs_hba *);
 	void    (*exit)(struct ufs_hba *);
 	u32	(*get_ufs_hci_version)(struct ufs_hba *);
@@ -1225,7 +1227,20 @@ static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
 	ufshcd_writel(hba, tmp, reg);
 }
 
-int ufshcd_alloc_host(struct device *, struct ufs_hba **);
+int __ufshcd_alloc_host(struct device *, struct ufs_hba **,
+			const struct ufs_hba_variant_ops *);
+
+/**
+ * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
+ * @dev: pointer to device handle
+ * @hba_handle: driver private handle
+ * Returns 0 on success, non-zero value on failure
+ */
+static inline int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
+{
+	return __ufshcd_alloc_host(dev, hba_handle, NULL);
+}
+
 void ufshcd_dealloc_host(struct ufs_hba *);
 int ufshcd_hba_enable(struct ufs_hba *hba);
 int ufshcd_init(struct ufs_hba *, void __iomem *, unsigned int);

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Bart Van Assche]

On 5/26/23 00:11, Adrian Hunter wrote:
> Why would we want to when we don't have to?

Anyone is allowed to formulate objections to a patch but when objecting 
to a patch or an approach please explain *why*. I think I already 
explained that unconditionally enabling BLK_MQ_F_BLOCKING makes testing 
the UFS host controller driver easier.

> So with those fixed and the vops->may_block instead of vops->nonblocking:

[ ... ]

I think a much simpler solution exists. I plan to post a new version of 
this patch series soon.

Bart.

Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Adrian Hunter]

On 26/05/23 20:27, Bart Van Assche wrote:
> On 5/26/23 00:11, Adrian Hunter wrote:
>> Why would we want to when we don't have to?
> 
> Anyone is allowed to formulate objections to a patch but when objecting to a patch or an approach please explain *why*. I think I already explained that unconditionally enabling BLK_MQ_F_BLOCKING makes testing the UFS host controller driver easier.

I don't think you are claiming blocking queues are better for block drivers than non-blocking queues, so it is quite reasonable to stay with non-blocking queues.

I am not sure testing is really relevant to this discussion, but it seems like different combinations need to be tested anyway e.g. testing with / without clock gating is the same testing with / without blocking.

> 
>> So with those fixed and the vops->may_block instead of vops->nonblocking:
> 
> [ ... ]
> 
> I think a much simpler solution exists. I plan to post a new version of this patch series soon.

Thank you!