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

[PATCH 0/5] 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.

Bart Van Assche (5):
  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: core: Unexport ufshcd_hold() and ufshcd_release()
  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        | 78 ++++++++++----------------------
 include/scsi/scsi_host.h         |  3 ++
 include/ufs/ufshcd.h             |  3 --
 7 files changed, 45 insertions(+), 71 deletions(-)

[PATCH 1/5] 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. This patch reworks code introduced by commit
f983622ae605 ("scsi: core: Avoid calling synchronize_rcu() for each device
in scsi_host_block()").

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);
 

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

[Mike Christie]

On 5/4/23 6:50 PM, Bart Van Assche wrote:
> Make scsi_host_block() easier to read by converting it to the widely used
> early-return style. This patch reworks code introduced by commit
> f983622ae605 ("scsi: core: Avoid calling synchronize_rcu() for each device
> in scsi_host_block()").
> 
> 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);
>  

Reviewed-by: Mike Christie <michael.christie@oracle.com>

[PATCH 2/5] 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/

Cc: 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.
 	 */

Re: [PATCH 2/5] scsi: core: Support setting BLK_MQ_F_BLOCKING

[Mike Christie]

On 5/4/23 6:50 PM, Bart Van Assche wrote:
> 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/
> 
> Cc: 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);

Reviewed-by: Mike Christie <michael.christie@oracle.com>
>  
>  	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 3/5] 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 2b8c2613f7d7..a1bce9c6aee5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8755,6 +8755,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 4/5] scsi: ufs: core: Unexport ufshcd_hold() and ufshcd_release()

[Bart Van Assche]

Unexport these functions since these are only used by the UFS core.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 2 --
 include/ufs/ufshcd.h      | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a1bce9c6aee5..54f91d7d0192 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1847,7 +1847,6 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 out:
 	return rc;
 }
-EXPORT_SYMBOL_GPL(ufshcd_hold);
 
 static void ufshcd_gate_work(struct work_struct *work)
 {
@@ -1950,7 +1949,6 @@ void ufshcd_release(struct ufs_hba *hba)
 	__ufshcd_release(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 }
-EXPORT_SYMBOL_GPL(ufshcd_release);
 
 static ssize_t ufshcd_clkgate_delay_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d6da1efb0212..13824462452d 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1358,9 +1358,6 @@ 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_release(struct ufs_hba *hba);
-
 void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value);
 
 u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);

Re: [PATCH 4/5] scsi: ufs: core: Unexport ufshcd_hold() and ufshcd_release()

[Bao D. Nguyen]

On 5/4/2023 4:50 PM, Bart Van Assche wrote:
> Unexport these functions since these are only used by the UFS core.
Hi Bart,
Qualcomm uses these ufshcd_hold() and ufshcd_release() functions in 
ufs-qcom.c. I am going to post Qualcomm's change soon.

Thanks,
Bao

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 2 --
>   include/ufs/ufshcd.h      | 3 ---
>   2 files changed, 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index a1bce9c6aee5..54f91d7d0192 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1847,7 +1847,6 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
>   out:
>   	return rc;
>   }
> -EXPORT_SYMBOL_GPL(ufshcd_hold);
>   
>   static void ufshcd_gate_work(struct work_struct *work)
>   {
> @@ -1950,7 +1949,6 @@ void ufshcd_release(struct ufs_hba *hba)
>   	__ufshcd_release(hba);
>   	spin_unlock_irqrestore(hba->host->host_lock, flags);
>   }
> -EXPORT_SYMBOL_GPL(ufshcd_release);
>   
>   static ssize_t ufshcd_clkgate_delay_show(struct device *dev,
>   		struct device_attribute *attr, char *buf)
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d6da1efb0212..13824462452d 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1358,9 +1358,6 @@ 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_release(struct ufs_hba *hba);
> -
>   void ufshcd_clkgate_delay_set(struct device *dev, unsigned long value);
>   
>   u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);

Re: [PATCH 4/5] scsi: ufs: core: Unexport ufshcd_hold() and ufshcd_release()

[Bart Van Assche]

On 5/5/23 11:40, Bao D. Nguyen wrote:
> On 5/4/2023 4:50 PM, Bart Van Assche wrote:
>> Unexport these functions since these are only used by the UFS core.
>
> Qualcomm uses these ufshcd_hold() and ufshcd_release() functions in ufs-qcom.c. I am going to post Qualcomm's change soon.

Hi Bao,

I will drop this patch.

Does the code that you plan to post pass 'false' or 'true' as second
argument to ufshcd_hold()?

Thanks,

Bart.

Re: [PATCH 4/5] scsi: ufs: core: Unexport ufshcd_hold() and ufshcd_release()

[Bao D. Nguyen]

On 5/5/2023 12:00 PM, Bart Van Assche wrote:
> On 5/5/23 11:40, Bao D. Nguyen wrote:
>> On 5/4/2023 4:50 PM, Bart Van Assche wrote:
>>> Unexport these functions since these are only used by the UFS core.
>>
>> Qualcomm uses these ufshcd_hold() and ufshcd_release() functions in 
>> ufs-qcom.c. I am going to post Qualcomm's change soon.
> 
> Hi Bao,
> 
> I will drop this patch.
> 
> Does the code that you plan to post pass 'false' or 'true' as second
> argument to ufshcd_hold()?
Thanks Bart. We are using ufshcd_hold(hba, false);

Thanks,
Bao

> 
> Thanks,
> 
> Bart.

[PATCH 5/5] 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.

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        | 75 ++++++++++----------------------
 4 files changed, 26 insertions(+), 55 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 54f91d7d0192..e4f490a25def 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;
@@ -1647,7 +1647,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;
 
@@ -1761,17 +1761,15 @@ static void ufshcd_ungate_work(struct work_struct *work)
  * 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++;
 
@@ -1788,15 +1786,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;
 		}
@@ -1827,12 +1820,6 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		 */
 		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 */
@@ -1844,8 +1831,6 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		break;
 	}
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-out:
-	return rc;
 }
 
 static void ufshcd_gate_work(struct work_struct *work)
@@ -2075,7 +2060,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);
 
@@ -2471,7 +2456,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);
 
@@ -2874,12 +2859,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;
@@ -2925,13 +2904,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;
@@ -2959,8 +2932,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;
 
@@ -3254,7 +3225,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);
@@ -3328,7 +3299,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,
@@ -3424,7 +3395,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,
@@ -4242,7 +4213,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);
 
@@ -4349,7 +4320,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);
@@ -4942,7 +4913,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,
@@ -6227,14 +6198,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);
@@ -6242,7 +6213,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);
 }
 
@@ -6887,7 +6858,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);
 
@@ -7123,7 +7094,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,
@@ -7189,7 +7160,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);
 
@@ -7423,7 +7394,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))) {
@@ -9407,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))

RE: [PATCH 5/5] scsi: ufs: Ungate the clock synchronously

[Avri Altman]

> -int ufshcd_hold(struct ufs_hba *hba, bool async)
> +void ufshcd_hold(struct ufs_hba *hba)
>  {
How about switching to the block quiescing API as well - blk_mq_{un}quiesce_tagset,
Instead of scsi_{un}block_requests?

Thanks,
Avri

Re: [PATCH 5/5] scsi: ufs: Ungate the clock synchronously

[Bart Van Assche]

On 5/7/23 00:08, Avri Altman wrote:
>> -int ufshcd_hold(struct ufs_hba *hba, bool async)
>> +void ufshcd_hold(struct ufs_hba *hba)
>>   {
> How about switching to the block quiescing API as well - blk_mq_{un}quiesce_tagset,
> Instead of scsi_{un}block_requests?

Hi Avri,

Did you perhaps want to refer to blk_mq_freeze_queue()? 
ufshcd_scsi_block_requests() may be called from inside 
ufshcd_queuecommand(). Calling blk_mq_freeze_queue() from inside 
ufshcd_queuecommand() would cause a deadlock.

Thanks,

Bart.