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

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

[Bart Van Assche]

Hi Martin,

This patch series includes the following changes:
- Fix a deadlock in the UFS error handler.
- Add polling support in the UFS driver.
- Several smaller fixes for the UFS driver.

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

Thanks,

Bart.

Changes compared to v2:
- Dropped SCSI core patches that add support for internal commands.
- Reworked patch "Fix a deadlock in the error handler" such that it uses a
  reserved tag as proposed by Adrian.
- Split patch "ufs: Introduce ufshcd_release_scsi_cmd()" into two patches.

Changes compared to v1:
- Add internal command support to the SCSI core.
- Reworked patch "ufs: Optimize the command queueing code".

Bart Van Assche (17):
  scsi: core: Fix scsi_device_max_queue_depth()
  scsi: core: Fix a race between scsi_done() and scsi_times_out()
  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: ufs: Fix race conditions related to driver data
  scsi: ufs: Remove ufshcd_any_tag_in_use()
  scsi: ufs: Rework ufshcd_change_queue_depth()
  scsi: ufs: Fix a deadlock in the error handler
  scsi: ufs: Remove the 'update_scaling' local variable
  scsi: ufs: Introduce ufshcd_release_scsi_cmd()
  scsi: ufs: Improve SCSI abort handling further
  scsi: ufs: Fix a kernel crash during shutdown
  scsi: ufs: Stop using the clock scaling lock in the error handler
  scsi: ufs: Optimize the command queueing code
  scsi: ufs: Implement polling support

 drivers/scsi/scsi.c                |   4 +-
 drivers/scsi/scsi_error.c          |  22 +--
 drivers/scsi/ufs/tc-dwc-g210-pci.c |   1 -
 drivers/scsi/ufs/ufs-exynos.c      |   4 +-
 drivers/scsi/ufs/ufshcd-pci.c      |   2 -
 drivers/scsi/ufs/ufshcd-pltfrm.c   |   2 -
 drivers/scsi/ufs/ufshcd.c          | 268 ++++++++++++++++-------------
 drivers/scsi/ufs/ufshcd.h          |   7 +-
 8 files changed, 165 insertions(+), 145 deletions(-)

[PATCH v3 01/17] scsi: core: Fix scsi_device_max_queue_depth()

[Bart Van Assche]

The comment above scsi_device_max_queue_depth() and also the description
of commit ca4453213951 ("scsi: core: Make sure sdev->queue_depth is <=
max(shost->can_queue, 1024)") contradict the implementation of the function
scsi_device_max_queue_depth(). Additionally, the maximum queue depth of a
SCSI LUN never exceeds host->can_queue. Fix scsi_device_max_queue_depth()
by changing max_t() into min_t().

Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sumanesh Samanta <sumanesh.samanta@broadcom.com>
Fixes: ca4453213951 ("scsi: core: Make sure sdev->queue_depth is <= max(shost->can_queue, 1024)")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index dee4d9c6046d..211aace69c22 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -200,11 +200,11 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 
 
 /*
- * 1024 is big enough for saturating the fast scsi LUN now
+ * 1024 is big enough for saturating fast SCSI LUNs.
  */
 int scsi_device_max_queue_depth(struct scsi_device *sdev)
 {
-	return max_t(int, sdev->host->can_queue, 1024);
+	return min_t(int, sdev->host->can_queue, 1024);
 }
 
 /**

Re: [PATCH v3 01/17] scsi: core: Fix scsi_device_max_queue_depth()

[Ming Lei]

On Tue, Nov 30, 2021 at 03:33:08PM -0800, Bart Van Assche wrote:
> The comment above scsi_device_max_queue_depth() and also the description
> of commit ca4453213951 ("scsi: core: Make sure sdev->queue_depth is <=
> max(shost->can_queue, 1024)") contradict the implementation of the function
> scsi_device_max_queue_depth(). Additionally, the maximum queue depth of a
> SCSI LUN never exceeds host->can_queue. Fix scsi_device_max_queue_depth()
> by changing max_t() into min_t().
> 
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sumanesh Samanta <sumanesh.samanta@broadcom.com>
> Fixes: ca4453213951 ("scsi: core: Make sure sdev->queue_depth is <= max(shost->can_queue, 1024)")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index dee4d9c6046d..211aace69c22 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -200,11 +200,11 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  
>  
>  /*
> - * 1024 is big enough for saturating the fast scsi LUN now
> + * 1024 is big enough for saturating fast SCSI LUNs.
>   */
>  int scsi_device_max_queue_depth(struct scsi_device *sdev)
>  {
> -	return max_t(int, sdev->host->can_queue, 1024);
> +	return min_t(int, sdev->host->can_queue, 1024);
>  }

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

-- 
Ming

[PATCH v3 02/17] scsi: core: Fix a race between scsi_done() and scsi_times_out()

[Bart Van Assche]

This patch restores the behavior of the following algorithm from the legacy
block layer:
- Before completing a request, test-and-set REQ_ATOM_COMPLETE atomically.
  Only call the block driver completion function if that flag was not yet
  set.
- Before calling the block driver timeout function, test-and-set
  REQ_ATOM_COMPLETE atomically. Only call the timeout handler if that flag
  was not yet set. If that flag was already set, do not restart the timer.

Cc: Keith Busch <kbusch@kernel.org>
Reported-by: Adrian Hunter <adrian.hunter@intel.com>
Fixes: 065990bd198e ("scsi: set timed out out mq requests to complete")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_error.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 9cb0f9df621a..cd05f2db3339 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -331,6 +331,14 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 	enum blk_eh_timer_return rtn = BLK_EH_DONE;
 	struct Scsi_Host *host = scmd->device->host;
 
+	/*
+	 * scsi_done() may be called concurrently with scsi_times_out(). Only
+	 * one of these two functions should proceed. Hence return early if
+	 * scsi_done() won the race.
+	 */
+	if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
+		return BLK_EH_DONE;
+
 	trace_scsi_dispatch_cmd_timeout(scmd);
 	scsi_log_completion(scmd, TIMEOUT_ERROR);
 
@@ -341,20 +349,6 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_DONE) {
-		/*
-		 * Set the command to complete first in order to prevent a real
-		 * completion from releasing the command while error handling
-		 * is using it. If the command was already completed, then the
-		 * lower level driver beat the timeout handler, and it is safe
-		 * to return without escalating error recovery.
-		 *
-		 * If timeout handling lost the race to a real completion, the
-		 * block layer may ignore that due to a fake timeout injection,
-		 * so return RESET_TIMER to allow error handling another shot
-		 * at this command.
-		 */
-		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
-			return BLK_EH_RESET_TIMER;
 		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
 			scsi_eh_scmd_add(scmd);

Re: [PATCH v3 02/17] scsi: core: Fix a race between scsi_done() and scsi_times_out()

[Keith Busch]

On Tue, Nov 30, 2021 at 03:33:09PM -0800, Bart Van Assche wrote:
> This patch restores the behavior of the following algorithm from the legacy
> block layer:
> - Before completing a request, test-and-set REQ_ATOM_COMPLETE atomically.
>   Only call the block driver completion function if that flag was not yet
>   set.
> - Before calling the block driver timeout function, test-and-set
>   REQ_ATOM_COMPLETE atomically. Only call the timeout handler if that flag
>   was not yet set. If that flag was already set, do not restart the timer.
> 
> Cc: Keith Busch <kbusch@kernel.org>
> Reported-by: Adrian Hunter <adrian.hunter@intel.com>
> Fixes: 065990bd198e ("scsi: set timed out out mq requests to complete")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_error.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 9cb0f9df621a..cd05f2db3339 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -331,6 +331,14 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
>  	enum blk_eh_timer_return rtn = BLK_EH_DONE;
>  	struct Scsi_Host *host = scmd->device->host;
>  
> +	/*
> +	 * scsi_done() may be called concurrently with scsi_times_out(). Only
> +	 * one of these two functions should proceed. Hence return early if
> +	 * scsi_done() won the race.
> +	 */
> +	if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
> +		return BLK_EH_DONE;
> +

If the the timeout handler successfully sets the state to complete, and
the lld returns BLK_EH_RESET_TIMER, who gets to complete this command?

>  	trace_scsi_dispatch_cmd_timeout(scmd);
>  	scsi_log_completion(scmd, TIMEOUT_ERROR);
>  
> @@ -341,20 +349,6 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
>  		rtn = host->hostt->eh_timed_out(scmd);
>  
>  	if (rtn == BLK_EH_DONE) {
> -		/*
> -		 * Set the command to complete first in order to prevent a real
> -		 * completion from releasing the command while error handling
> -		 * is using it. If the command was already completed, then the
> -		 * lower level driver beat the timeout handler, and it is safe
> -		 * to return without escalating error recovery.
> -		 *
> -		 * If timeout handling lost the race to a real completion, the
> -		 * block layer may ignore that due to a fake timeout injection,
> -		 * so return RESET_TIMER to allow error handling another shot
> -		 * at this command.
> -		 */
> -		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
> -			return BLK_EH_RESET_TIMER;
>  		if (scsi_abort_command(scmd) != SUCCESS) {
>  			set_host_byte(scmd, DID_TIME_OUT);
>  			scsi_eh_scmd_add(scmd);

Re: [PATCH v3 02/17] scsi: core: Fix a race between scsi_done() and scsi_times_out()

[Bart Van Assche]

On 12/1/21 1:43 PM, Keith Busch wrote:
> If the the timeout handler successfully sets the state to complete, and
> the lld returns BLK_EH_RESET_TIMER, who gets to complete this command?

That's a longstanding problem, isn't it? Anyway, how about replacing this
patch with the two untested patches below?


Subject: [PATCH 1/2] scsi: core: Rename scsi_cmnd.state

Prepare for removal of SCMD_STATE_COMPLETE. This patch does not change
any functionality.

Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Keith Busch <kbusch@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/hosts.c      |  2 +-
  drivers/scsi/scsi_error.c |  2 +-
  drivers/scsi/scsi_lib.c   | 12 ++++++------
  include/scsi/scsi_cmnd.h  |  4 ++--
  4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index f69b77cbf538..f5869bd13bcf 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -572,7 +572,7 @@ static bool scsi_host_check_in_flight(struct request *rq, void *data,
  	int *count = data;
  	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);

-	if (test_bit(SCMD_STATE_INFLIGHT, &cmd->state))
+	if (test_bit(SCMD_STATE_INFLIGHT, &cmd->in_flight))
  		(*count)++;

  	return true;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 9cb0f9df621a..7b603b259ae2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -353,7 +353,7 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
  		 * so return RESET_TIMER to allow error handling another shot
  		 * at this command.
  		 */
-		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
+		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->in_flight))
  			return BLK_EH_RESET_TIMER;
  		if (scsi_abort_command(scmd) != SUCCESS) {
  			set_host_byte(scmd, DID_TIME_OUT);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 621d841d819a..6bb0e2726d51 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -280,7 +280,7 @@ static void scsi_dec_host_busy(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
  	unsigned long flags;

  	rcu_read_lock();
-	__clear_bit(SCMD_STATE_INFLIGHT, &cmd->state);
+	__clear_bit(SCMD_STATE_INFLIGHT, &cmd->in_flight);
  	if (unlikely(scsi_host_in_recovery(shost))) {
  		spin_lock_irqsave(shost->host_lock, flags);
  		if (shost->host_failed || shost->host_eh_scheduled)
@@ -1138,7 +1138,7 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)

  	jiffies_at_alloc = cmd->jiffies_at_alloc;
  	retries = cmd->retries;
-	in_flight = test_bit(SCMD_STATE_INFLIGHT, &cmd->state);
+	in_flight = test_bit(SCMD_STATE_INFLIGHT, &cmd->in_flight);
  	/*
  	 * Zero out the cmd, except for the embedded scsi_request. Only clear
  	 * the driver-private command data if the LLD does not supply a
@@ -1158,7 +1158,7 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
  	cmd->jiffies_at_alloc = jiffies_at_alloc;
  	cmd->retries = retries;
  	if (in_flight)
-		__set_bit(SCMD_STATE_INFLIGHT, &cmd->state);
+		__set_bit(SCMD_STATE_INFLIGHT, &cmd->in_flight);
  	cmd->budget_token = budget_token;

  }
@@ -1367,7 +1367,7 @@ static inline int scsi_host_queue_ready(struct request_queue *q,
  		spin_unlock_irq(shost->host_lock);
  	}

-	__set_bit(SCMD_STATE_INFLIGHT, &cmd->state);
+	__set_bit(SCMD_STATE_INFLIGHT, &cmd->in_flight);

  	return 1;

@@ -1597,7 +1597,7 @@ void scsi_done(struct scsi_cmnd *cmd)

  	if (unlikely(blk_should_fake_timeout(scsi_cmd_to_rq(cmd)->q)))
  		return;
-	if (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state)))
+	if (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->in_flight)))
  		return;
  	trace_scsi_dispatch_cmd_done(cmd);
  	blk_mq_complete_request(scsi_cmd_to_rq(cmd));
@@ -1691,7 +1691,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
  			goto out_dec_host_busy;
  		req->rq_flags |= RQF_DONTPREP;
  	} else {
-		clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
+		clear_bit(SCMD_STATE_COMPLETE, &cmd->in_flight);
  	}

  	cmd->flags &= SCMD_PRESERVED_FLAGS;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 477a800a9543..6cbfbfbbb803 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -60,7 +60,7 @@ struct scsi_pointer {
  /* flags preserved across unprep / reprep */
  #define SCMD_PRESERVED_FLAGS	(SCMD_INITIALIZED)

-/* for scmd->state */
+/* for scmd->in_flight */
  #define SCMD_STATE_COMPLETE	0
  #define SCMD_STATE_INFLIGHT	1

@@ -139,7 +139,7 @@ struct scsi_cmnd {

  	int result;		/* Status code from lower level driver */
  	int flags;		/* Command flags */
-	unsigned long state;	/* Command completion state */
+	unsigned long in_flight;/* Command completion state */

  	unsigned int extra_len;	/* length of alignment and padding */
  };




Subject: [PATCH 2/2] scsi: core: Fix a race between scsi_done() and scsi_times_out()

If scsi_done() and scsi_times_out() are called concurrently, make sure
that only one of these two functions proceeds. If scsi_done() is called
while scsi_times_out() is in progress and if scsi_times_out() returns
BLK_EH_RESET_TIMER, complete the command. If scsi_times_out() returns
BLK_EH_RESET_TIMER, allow scsi_done() to complete the command.

Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Keith Busch <kbusch@kernel.org>
Fixes: 065990bd198e ("scsi: set timed out out mq requests to complete")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/scsi_error.c  | 35 +++++++++++++++++++++--------------
  drivers/scsi/scsi_lib.c    | 16 +++++++++++++---
  drivers/scsi/virtio_scsi.c |  4 ++--
  include/scsi/scsi_cmnd.h   | 12 ++++++++++--
  4 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 7b603b259ae2..42dd967167e6 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -330,6 +330,15 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
  	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
  	enum blk_eh_timer_return rtn = BLK_EH_DONE;
  	struct Scsi_Host *host = scmd->device->host;
+	int old = SCMD_STATE_SUBMITTED;
+
+	/*
+	 * scsi_done() may be called concurrently with scsi_times_out(). Only
+	 * one of these two functions should proceed. Hence return early if
+	 * scsi_done() won the race.
+	 */
+	if (!atomic_try_cmpxchg(&scmd->state, &old, SCMD_STATE_TIMED_OUT))
+		return BLK_EH_DONE;

  	trace_scsi_dispatch_cmd_timeout(scmd);
  	scsi_log_completion(scmd, TIMEOUT_ERROR);
@@ -341,26 +350,24 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
  		rtn = host->hostt->eh_timed_out(scmd);

  	if (rtn == BLK_EH_DONE) {
-		/*
-		 * Set the command to complete first in order to prevent a real
-		 * completion from releasing the command while error handling
-		 * is using it. If the command was already completed, then the
-		 * lower level driver beat the timeout handler, and it is safe
-		 * to return without escalating error recovery.
-		 *
-		 * If timeout handling lost the race to a real completion, the
-		 * block layer may ignore that due to a fake timeout injection,
-		 * so return RESET_TIMER to allow error handling another shot
-		 * at this command.
-		 */
-		if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->in_flight))
-			return BLK_EH_RESET_TIMER;
  		if (scsi_abort_command(scmd) != SUCCESS) {
  			set_host_byte(scmd, DID_TIME_OUT);
  			scsi_eh_scmd_add(scmd);
  		}
+		return rtn;
  	}

+	/* The order of the atomic_try_cmpxchg() calls below is important. */
+	old = SCMD_STATE_TIMED_OUT;
+	if (atomic_try_cmpxchg(&scmd->state, &old, SCMD_STATE_SUBMITTED))
+		return rtn;
+	old = SCMD_STATE_COMPLETE_AFTER_TIMEOUT;
+	WARN_ON_ONCE(!atomic_try_cmpxchg(&scmd->state, &old,
+					 SCMD_STATE_COMPLETED));
+	WARN_ON_ONCE(scmd->submitter != SUBMITTED_BY_BLOCK_LAYER);
+	trace_scsi_dispatch_cmd_done(scmd);
+	blk_mq_complete_request(scsi_cmd_to_rq(scmd));
+
  	return rtn;
  }

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6bb0e2726d51..797ad188e7a2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1586,6 +1586,8 @@ static blk_status_t scsi_prepare_cmd(struct request *req)

  void scsi_done(struct scsi_cmnd *cmd)
  {
+	int old;
+
  	switch (cmd->submitter) {
  	case SUBMITTED_BY_BLOCK_LAYER:
  		break;
@@ -1597,8 +1599,15 @@ void scsi_done(struct scsi_cmnd *cmd)

  	if (unlikely(blk_should_fake_timeout(scsi_cmd_to_rq(cmd)->q)))
  		return;
-	if (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->in_flight)))
-		return;
+	for (;;) {
+		old = SCMD_STATE_SUBMITTED;
+		if (atomic_try_cmpxchg(&cmd->state, &old, SCMD_STATE_COMPLETED))
+			break;
+		old = SCMD_STATE_TIMED_OUT;
+		if (atomic_try_cmpxchg(&cmd->state, &old,
+				       SCMD_STATE_COMPLETE_AFTER_TIMEOUT))
+			return;
+	}
  	trace_scsi_dispatch_cmd_done(cmd);
  	blk_mq_complete_request(scsi_cmd_to_rq(cmd));
  }
@@ -1691,7 +1700,8 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
  			goto out_dec_host_busy;
  		req->rq_flags |= RQF_DONTPREP;
  	} else {
-		clear_bit(SCMD_STATE_COMPLETE, &cmd->in_flight);
+		BUILD_BUG_ON(SCMD_STATE_SUBMITTED != 0);
+		atomic_set(&cmd->state, SCMD_STATE_SUBMITTED);
  	}

  	cmd->flags &= SCMD_PRESERVED_FLAGS;
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 19f7d7b90625..9ea3ec308ecd 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -620,8 +620,8 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
  	 * we're using independent interrupts (e.g. MSI).  Poll the
  	 * virtqueues once.
  	 *
-	 * In the abort case, scsi_done() will do nothing, because the
-	 * command timed out and hence SCMD_STATE_COMPLETE has been set.
+	 * In the abort case, scsi_done() will do nothing. See also the
+	 * scsi_done() implementation.
  	 */
  	virtscsi_poll_requests(vscsi);

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 6cbfbfbbb803..350b47792a4d 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -53,6 +53,14 @@ struct scsi_pointer {
  	volatile int phase;
  };

+enum scsi_cmd_state {
+	SCMD_STATE_SUBMITTED = 0,	/* Owned by device or not submitted. */
+	SCMD_STATE_COMPLETED = 1,	/* scsi_done() is in progress. */
+	SCMD_STATE_TIMED_OUT = 2,	/* Inside timeout handler. */
+	SCMD_STATE_COMPLETE_AFTER_TIMEOUT = 3, /* Complete after timeout
+						* handler finished. */
+} __packed;
+
  /* for scmd->flags */
  #define SCMD_TAGGED		(1 << 0)
  #define SCMD_INITIALIZED	(1 << 1)
@@ -61,8 +69,7 @@ struct scsi_pointer {
  #define SCMD_PRESERVED_FLAGS	(SCMD_INITIALIZED)

  /* for scmd->in_flight */
-#define SCMD_STATE_COMPLETE	0
-#define SCMD_STATE_INFLIGHT	1
+#define SCMD_STATE_INFLIGHT	0

  enum scsi_cmnd_submitter {
  	SUBMITTED_BY_BLOCK_LAYER = 0,
@@ -92,6 +99,7 @@ struct scsi_cmnd {
  	int retries;
  	int allowed;

+	atomic_t state; /* See also enum scsi_cmd_state */
  	unsigned char prot_op;
  	unsigned char prot_type;
  	unsigned char prot_flags;

[PATCH v3 03/17] scsi: ufs: Rename a function argument

[Bart Van Assche]

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

Reviewed-by: Chanho Park <chanho61.park@samsung.com>
Acked-by: Alim Akhtar <alim.akhtar@samsung.com>
Reviewed-by: Keoseong Park <keosung.park@samsung.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
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 6103e98e9a08..28c1bbe9fa7d 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 v3 04/17] 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.

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
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 024f6d958341..4821ad9912bb 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 v3 05/17] 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.

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
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 4821ad9912bb..973b7b083dbe 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7412,7 +7412,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);
@@ -7423,14 +7423,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 28c1bbe9fa7d..ecc6c545a19d 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 v3 06/17] 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.

Acked-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 973b7b083dbe..d4996ada55b6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6730,11 +6730,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;
@@ -6802,8 +6797,8 @@ 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_mq_free_request(req);
+
 out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;

[PATCH v3 07/17] scsi: ufs: Fix race conditions related to driver data

[Bart Van Assche]

The driver data pointer must be set before any callbacks are registered
that use that pointer. Hence move the initialization of that pointer
from after the ufshcd_init() call to inside ufshcd_init().

Fixes: 3b1d05807a9a ("[SCSI] ufs: Segregate PCI Specific Code")
Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/tc-dwc-g210-pci.c | 1 -
 drivers/scsi/ufs/ufshcd-pci.c      | 2 --
 drivers/scsi/ufs/ufshcd-pltfrm.c   | 2 --
 drivers/scsi/ufs/ufshcd.c          | 7 +++++++
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/tc-dwc-g210-pci.c b/drivers/scsi/ufs/tc-dwc-g210-pci.c
index 679289e1a78e..7b08e2e07cc5 100644
--- a/drivers/scsi/ufs/tc-dwc-g210-pci.c
+++ b/drivers/scsi/ufs/tc-dwc-g210-pci.c
@@ -110,7 +110,6 @@ tc_dwc_g210_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return err;
 	}
 
-	pci_set_drvdata(pdev, hba);
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_allow(&pdev->dev);
 
diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 51424557810d..a673eedb2f05 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -522,8 +522,6 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return err;
 	}
 
-	pci_set_drvdata(pdev, hba);
-
 	hba->vops = (struct ufs_hba_variant_ops *)id->driver_data;
 
 	err = ufshcd_init(hba, mmio_base, pdev->irq);
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index eaeae83b999f..8b16bbbcb806 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -361,8 +361,6 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 		goto dealloc_host;
 	}
 
-	platform_set_drvdata(pdev, hba);
-
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d4996ada55b6..04a19b826837 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9481,6 +9481,13 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	struct device *dev = hba->dev;
 	char eh_wq_name[sizeof("ufs_eh_wq_00")];
 
+	/*
+	 * dev_set_drvdata() must be called before any callbacks are registered
+	 * that use dev_get_drvdata() (frequency scaling, clock scaling, hwmon,
+	 * sysfs).
+	 */
+	dev_set_drvdata(dev, hba);
+
 	if (!mmio_base) {
 		dev_err(hba->dev,
 		"Invalid memory reference for mmio_base is NULL\n");

[PATCH v3 08/17] scsi: ufs: Remove ufshcd_any_tag_in_use()

[Bart Van Assche]

Use hba->outstanding_reqs instead of ufshcd_any_tag_in_use(). This patch
prepares for removal of the blk_mq_start_request() call from
ufshcd_wait_for_dev_cmd(). blk_mq_tagset_busy_iter() only iterates over
started requests.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 04a19b826837..974bf47e733c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1352,25 +1352,6 @@ static int ufshcd_devfreq_target(struct device *dev,
 	return ret;
 }
 
-static bool ufshcd_is_busy(struct request *req, void *priv, bool reserved)
-{
-	int *busy = priv;
-
-	WARN_ON_ONCE(reserved);
-	(*busy)++;
-	return false;
-}
-
-/* Whether or not any tag is in use by a request that is in progress. */
-static bool ufshcd_any_tag_in_use(struct ufs_hba *hba)
-{
-	struct request_queue *q = hba->cmd_queue;
-	int busy = 0;
-
-	blk_mq_tagset_busy_iter(q->tag_set, ufshcd_is_busy, &busy);
-	return busy;
-}
-
 static int ufshcd_devfreq_get_dev_status(struct device *dev,
 		struct devfreq_dev_status *stat)
 {
@@ -1769,7 +1750,7 @@ static void ufshcd_gate_work(struct work_struct *work)
 
 	if (hba->clk_gating.active_reqs
 		|| hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
-		|| ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks
+		|| hba->outstanding_reqs || hba->outstanding_tasks
 		|| hba->active_uic_cmd || hba->uic_async_done)
 		goto rel_lock;
 

[PATCH v3 09/17] 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 974bf47e733c..2d0f59424b00 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4936,11 +4936,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 v3 10/17] 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 | 53 +++++++++++----------------------------
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2d0f59424b00..da4714aaa850 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -128,8 +128,9 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
 enum {
 	UFSHCD_MAX_CHANNEL	= 0,
 	UFSHCD_MAX_ID		= 1,
-	UFSHCD_CMD_PER_LUN	= 32,
-	UFSHCD_CAN_QUEUE	= 32,
+	UFSHCD_NUM_RESERVED	= 1,
+	UFSHCD_CMD_PER_LUN	= 32 - UFSHCD_NUM_RESERVED,
+	UFSHCD_CAN_QUEUE	= 32 - UFSHCD_NUM_RESERVED,
 };
 
 static const char *const ufshcd_state_name[] = {
@@ -2170,6 +2171,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
 	hba->nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
 	hba->nutmrs =
 	((hba->capabilities & MASK_TASK_MANAGEMENT_REQUEST_SLOTS) >> 16) + 1;
+	hba->reserved_slot = hba->nutrs - 1;
 
 	/* Read crypto capabilities */
 	err = ufshcd_hba_init_crypto_capabilities(hba);
@@ -2912,30 +2914,15 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 		enum dev_cmd_type cmd_type, int timeout)
 {
-	struct request_queue *q = hba->cmd_queue;
 	DECLARE_COMPLETION_ONSTACK(wait);
-	struct request *req;
+	const u32 tag = hba->reserved_slot;
 	struct ufshcd_lrb *lrbp;
 	int err;
-	int tag;
 
-	down_read(&hba->clk_scaling_lock);
+	/* Protects use of hba->reserved_slot. */
+	lockdep_assert_held(&hba->dev_cmd.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_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
-	if (IS_ERR(req)) {
-		err = PTR_ERR(req);
-		goto out_unlock;
-	}
-	tag = req->tag;
-	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
-	/* Set the timeout such that the SCSI error handler is not activated. */
-	req->timeout = msecs_to_jiffies(2 * timeout);
-	blk_mq_start_request(req);
+	down_read(&hba->clk_scaling_lock);
 
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
@@ -2953,8 +2940,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
 out:
-	blk_mq_free_request(req);
-out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;
 }
@@ -6689,23 +6674,16 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 					enum dev_cmd_type cmd_type,
 					enum query_opcode desc_op)
 {
-	struct request_queue *q = hba->cmd_queue;
 	DECLARE_COMPLETION_ONSTACK(wait);
-	struct request *req;
+	const u32 tag = hba->reserved_slot;
 	struct ufshcd_lrb *lrbp;
 	int err = 0;
-	int tag;
 	u8 upiu_flags;
 
-	down_read(&hba->clk_scaling_lock);
+	/* Protects use of hba->reserved_slot. */
+	lockdep_assert_held(&hba->dev_cmd.lock);
 
-	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
-	if (IS_ERR(req)) {
-		err = PTR_ERR(req);
-		goto out_unlock;
-	}
-	tag = req->tag;
-	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
+	down_read(&hba->clk_scaling_lock);
 
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
@@ -6774,9 +6752,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);
 
-	blk_mq_free_request(req);
-
-out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;
 }
@@ -9507,8 +9482,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 - UFSHCD_NUM_RESERVED;
+	host->cmd_per_lun = hba->nutrs - UFSHCD_NUM_RESERVED;
 	host->max_id = UFSHCD_MAX_ID;
 	host->max_lun = UFS_MAX_LUNS;
 	host->max_channel = UFSHCD_MAX_CHANNEL;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index ecc6c545a19d..c3c2792f309f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -745,6 +745,7 @@ struct ufs_hba_monitor {
  * @capabilities: UFS Controller Capabilities
  * @nutrs: Transfer Request Queue depth supported by controller
  * @nutmrs: Task Management Queue depth supported by controller
+ * @reserved_slot: Used to submit device commands. Protected by @dev_cmd.lock.
  * @ufs_version: UFS Version to which controller complies
  * @vops: pointer to variant specific operations
  * @priv: pointer to variant specific private data
@@ -836,6 +837,7 @@ struct ufs_hba {
 	u32 capabilities;
 	int nutrs;
 	int nutmrs;
+	u32 reserved_slot;
 	u32 ufs_version;
 	const struct ufs_hba_variant_ops *vops;
 	struct ufs_hba_variant_params *vps;

Re: [PATCH v3 10/17] scsi: ufs: Fix a deadlock in the error handler

[Adrian Hunter]

On 01/12/2021 01:33, Bart Van Assche wrote:
> 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>

One minor comment below, nevertheless:

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/scsi/ufs/ufshcd.c | 53 +++++++++++----------------------------
>  drivers/scsi/ufs/ufshcd.h |  2 ++
>  2 files changed, 16 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2d0f59424b00..da4714aaa850 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -128,8 +128,9 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
>  enum {
>  	UFSHCD_MAX_CHANNEL	= 0,
>  	UFSHCD_MAX_ID		= 1,
> -	UFSHCD_CMD_PER_LUN	= 32,
> -	UFSHCD_CAN_QUEUE	= 32,
> +	UFSHCD_NUM_RESERVED	= 1,
> +	UFSHCD_CMD_PER_LUN	= 32 - UFSHCD_NUM_RESERVED,
> +	UFSHCD_CAN_QUEUE	= 32 - UFSHCD_NUM_RESERVED,
>  };
>  
>  static const char *const ufshcd_state_name[] = {
> @@ -2170,6 +2171,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
>  	hba->nutrs = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
>  	hba->nutmrs =
>  	((hba->capabilities & MASK_TASK_MANAGEMENT_REQUEST_SLOTS) >> 16) + 1;
> +	hba->reserved_slot = hba->nutrs - 1;
>  
>  	/* Read crypto capabilities */
>  	err = ufshcd_hba_init_crypto_capabilities(hba);
> @@ -2912,30 +2914,15 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
>  static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>  		enum dev_cmd_type cmd_type, int timeout)
>  {
> -	struct request_queue *q = hba->cmd_queue;

I think cmd_queue is not used anymore after this.

>  	DECLARE_COMPLETION_ONSTACK(wait);
> -	struct request *req;
> +	const u32 tag = hba->reserved_slot;
>  	struct ufshcd_lrb *lrbp;
>  	int err;
> -	int tag;
>  
> -	down_read(&hba->clk_scaling_lock);
> +	/* Protects use of hba->reserved_slot. */
> +	lockdep_assert_held(&hba->dev_cmd.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_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
> -	if (IS_ERR(req)) {
> -		err = PTR_ERR(req);
> -		goto out_unlock;
> -	}
> -	tag = req->tag;
> -	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> -	/* Set the timeout such that the SCSI error handler is not activated. */
> -	req->timeout = msecs_to_jiffies(2 * timeout);
> -	blk_mq_start_request(req);
> +	down_read(&hba->clk_scaling_lock);
>  
>  	lrbp = &hba->lrb[tag];
>  	WARN_ON(lrbp->cmd);
> @@ -2953,8 +2940,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>  				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
>  
>  out:
> -	blk_mq_free_request(req);
> -out_unlock:
>  	up_read(&hba->clk_scaling_lock);
>  	return err;
>  }
> @@ -6689,23 +6674,16 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>  					enum dev_cmd_type cmd_type,
>  					enum query_opcode desc_op)
>  {
> -	struct request_queue *q = hba->cmd_queue;
>  	DECLARE_COMPLETION_ONSTACK(wait);
> -	struct request *req;
> +	const u32 tag = hba->reserved_slot;
>  	struct ufshcd_lrb *lrbp;
>  	int err = 0;
> -	int tag;
>  	u8 upiu_flags;
>  
> -	down_read(&hba->clk_scaling_lock);
> +	/* Protects use of hba->reserved_slot. */
> +	lockdep_assert_held(&hba->dev_cmd.lock);
>  
> -	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
> -	if (IS_ERR(req)) {
> -		err = PTR_ERR(req);
> -		goto out_unlock;
> -	}
> -	tag = req->tag;
> -	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> +	down_read(&hba->clk_scaling_lock);
>  
>  	lrbp = &hba->lrb[tag];
>  	WARN_ON(lrbp->cmd);
> @@ -6774,9 +6752,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);
>  
> -	blk_mq_free_request(req);
> -
> -out_unlock:
>  	up_read(&hba->clk_scaling_lock);
>  	return err;
>  }
> @@ -9507,8 +9482,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 - UFSHCD_NUM_RESERVED;
> +	host->cmd_per_lun = hba->nutrs - UFSHCD_NUM_RESERVED;
>  	host->max_id = UFSHCD_MAX_ID;
>  	host->max_lun = UFS_MAX_LUNS;
>  	host->max_channel = UFSHCD_MAX_CHANNEL;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index ecc6c545a19d..c3c2792f309f 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -745,6 +745,7 @@ struct ufs_hba_monitor {
>   * @capabilities: UFS Controller Capabilities
>   * @nutrs: Transfer Request Queue depth supported by controller
>   * @nutmrs: Task Management Queue depth supported by controller
> + * @reserved_slot: Used to submit device commands. Protected by @dev_cmd.lock.
>   * @ufs_version: UFS Version to which controller complies
>   * @vops: pointer to variant specific operations
>   * @priv: pointer to variant specific private data
> @@ -836,6 +837,7 @@ struct ufs_hba {
>  	u32 capabilities;
>  	int nutrs;
>  	int nutmrs;
> +	u32 reserved_slot;
>  	u32 ufs_version;
>  	const struct ufs_hba_variant_ops *vops;
>  	struct ufs_hba_variant_params *vps;
> 

Re: [PATCH v3 10/17] scsi: ufs: Fix a deadlock in the error handler

[Bart Van Assche]

On 12/1/21 5:48 AM, Adrian Hunter wrote:
> I think cmd_queue is not used anymore after this.

Let's remove cmd_queue via a separate patch. I have started testing this patch:

Subject: [PATCH] scsi: ufs: Remove hba->cmd_queue

Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/ufs/ufshcd.c | 11 +----------
  drivers/scsi/ufs/ufshcd.h |  2 --
  2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5b3efc880246..d379c2b0c058 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9409,7 +9409,6 @@ void ufshcd_remove(struct ufs_hba *hba)
  	ufs_sysfs_remove_nodes(hba->dev);
  	blk_cleanup_queue(hba->tmf_queue);
  	blk_mq_free_tag_set(&hba->tmf_tag_set);
-	blk_cleanup_queue(hba->cmd_queue);
  	scsi_remove_host(hba->host);
  	/* disable interrupts */
  	ufshcd_disable_intr(hba, hba->intr_mask);
@@ -9630,12 +9629,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
  		goto out_disable;
  	}

-	hba->cmd_queue = blk_mq_init_queue(&hba->host->tag_set);
-	if (IS_ERR(hba->cmd_queue)) {
-		err = PTR_ERR(hba->cmd_queue);
-		goto out_remove_scsi_host;
-	}
-
  	hba->tmf_tag_set = (struct blk_mq_tag_set) {
  		.nr_hw_queues	= 1,
  		.queue_depth	= hba->nutmrs,
@@ -9644,7 +9637,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
  	};
  	err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
  	if (err < 0)
-		goto free_cmd_queue;
+		goto out_remove_scsi_host;
  	hba->tmf_queue = blk_mq_init_queue(&hba->tmf_tag_set);
  	if (IS_ERR(hba->tmf_queue)) {
  		err = PTR_ERR(hba->tmf_queue);
@@ -9713,8 +9706,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
  	blk_cleanup_queue(hba->tmf_queue);
  free_tmf_tag_set:
  	blk_mq_free_tag_set(&hba->tmf_tag_set);
-free_cmd_queue:
-	blk_cleanup_queue(hba->cmd_queue);
  out_remove_scsi_host:
  	scsi_remove_host(hba->host);
  out_disable:
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 411c6015bbfe..88c20f3608c2 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -738,7 +738,6 @@ struct ufs_hba_monitor {
   * @host: Scsi_Host instance of the driver
   * @dev: device handle
   * @lrb: local reference block
- * @cmd_queue: Used to allocate command tags from hba->host->tag_set.
   * @outstanding_tasks: Bits representing outstanding task requests
   * @outstanding_lock: Protects @outstanding_reqs.
   * @outstanding_reqs: Bits representing outstanding transfer requests
@@ -805,7 +804,6 @@ struct ufs_hba {

  	struct Scsi_Host *host;
  	struct device *dev;
-	struct request_queue *cmd_queue;
  	/*
  	 * This field is to keep a reference to "scsi_device" corresponding to
  	 * "UFS device" W-LU.

Re: [PATCH v3 10/17] scsi: ufs: Fix a deadlock in the error handler

[Adrian Hunter]

On 01/12/2021 23:26, Bart Van Assche wrote:
> On 12/1/21 5:48 AM, Adrian Hunter wrote:
>> I think cmd_queue is not used anymore after this.
> 
> Let's remove cmd_queue via a separate patch. I have started testing this patch:
> 
> Subject: [PATCH] scsi: ufs: Remove hba->cmd_queue
> 
> Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/scsi/ufs/ufshcd.c | 11 +----------
>  drivers/scsi/ufs/ufshcd.h |  2 --
>  2 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5b3efc880246..d379c2b0c058 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -9409,7 +9409,6 @@ void ufshcd_remove(struct ufs_hba *hba)
>      ufs_sysfs_remove_nodes(hba->dev);
>      blk_cleanup_queue(hba->tmf_queue);
>      blk_mq_free_tag_set(&hba->tmf_tag_set);
> -    blk_cleanup_queue(hba->cmd_queue);
>      scsi_remove_host(hba->host);
>      /* disable interrupts */
>      ufshcd_disable_intr(hba, hba->intr_mask);
> @@ -9630,12 +9629,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>          goto out_disable;
>      }
> 
> -    hba->cmd_queue = blk_mq_init_queue(&hba->host->tag_set);
> -    if (IS_ERR(hba->cmd_queue)) {
> -        err = PTR_ERR(hba->cmd_queue);
> -        goto out_remove_scsi_host;
> -    }
> -
>      hba->tmf_tag_set = (struct blk_mq_tag_set) {
>          .nr_hw_queues    = 1,
>          .queue_depth    = hba->nutmrs,
> @@ -9644,7 +9637,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>      };
>      err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
>      if (err < 0)
> -        goto free_cmd_queue;
> +        goto out_remove_scsi_host;
>      hba->tmf_queue = blk_mq_init_queue(&hba->tmf_tag_set);
>      if (IS_ERR(hba->tmf_queue)) {
>          err = PTR_ERR(hba->tmf_queue);
> @@ -9713,8 +9706,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>      blk_cleanup_queue(hba->tmf_queue);
>  free_tmf_tag_set:
>      blk_mq_free_tag_set(&hba->tmf_tag_set);
> -free_cmd_queue:
> -    blk_cleanup_queue(hba->cmd_queue);
>  out_remove_scsi_host:
>      scsi_remove_host(hba->host);
>  out_disable:
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 411c6015bbfe..88c20f3608c2 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -738,7 +738,6 @@ struct ufs_hba_monitor {
>   * @host: Scsi_Host instance of the driver
>   * @dev: device handle
>   * @lrb: local reference block
> - * @cmd_queue: Used to allocate command tags from hba->host->tag_set.
>   * @outstanding_tasks: Bits representing outstanding task requests
>   * @outstanding_lock: Protects @outstanding_reqs.
>   * @outstanding_reqs: Bits representing outstanding transfer requests
> @@ -805,7 +804,6 @@ struct ufs_hba {
> 
>      struct Scsi_Host *host;
>      struct device *dev;
> -    struct request_queue *cmd_queue;
>      /*
>       * This field is to keep a reference to "scsi_device" corresponding to
>       * "UFS device" W-LU.

[PATCH v3 11/17] scsi: ufs: Remove the 'update_scaling' local variable

[Bart Van Assche]

This patch does not change any functionality but makes the next patch in
this series easier to read.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index da4714aaa850..4e9755c060af 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5225,7 +5225,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 	struct scsi_cmnd *cmd;
 	int result;
 	int index;
-	bool update_scaling = false;
 
 	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
 		lrbp = &hba->lrb[index];
@@ -5243,18 +5242,16 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 			/* Do not touch lrbp after scsi done */
 			scsi_done(cmd);
 			ufshcd_release(hba);
-			update_scaling = true;
+			ufshcd_clk_scaling_update_busy(hba);
 		} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
 			lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
 			if (hba->dev_cmd.complete) {
 				ufshcd_add_command_trace(hba, index,
 							 UFS_DEV_COMP);
 				complete(hba->dev_cmd.complete);
-				update_scaling = true;
+				ufshcd_clk_scaling_update_busy(hba);
 			}
 		}
-		if (update_scaling)
-			ufshcd_clk_scaling_update_busy(hba);
 	}
 }
 

[PATCH v3 12/17] scsi: ufs: Introduce ufshcd_release_scsi_cmd()

[Bart Van Assche]

The only functional change in this patch is that scsi_done() is now called
after ufshcd_release() and ufshcd_clk_scaling_update_busy() instead of
before.

The next patch in this series will introduce a call to
ufshcd_release_scsi_cmd() in the abort handler.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 4e9755c060af..8703e4a70256 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5213,6 +5213,18 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 	return retval;
 }
 
+/* Release the resources allocated for processing a SCSI command. */
+static void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
+				    struct ufshcd_lrb *lrbp)
+{
+	struct scsi_cmnd *cmd = lrbp->cmd;
+
+	scsi_dma_unmap(cmd);
+	lrbp->cmd = NULL;	/* Mark the command as completed. */
+	ufshcd_release(hba);
+	ufshcd_clk_scaling_update_busy(hba);
+}
+
 /**
  * __ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
@@ -5223,7 +5235,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 {
 	struct ufshcd_lrb *lrbp;
 	struct scsi_cmnd *cmd;
-	int result;
 	int index;
 
 	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
@@ -5234,15 +5245,10 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 			if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
 				ufshcd_update_monitor(hba, lrbp);
 			ufshcd_add_command_trace(hba, index, UFS_CMD_COMP);
-			result = ufshcd_transfer_rsp_status(hba, lrbp);
-			scsi_dma_unmap(cmd);
-			cmd->result = result;
-			/* Mark completed command as NULL in LRB */
-			lrbp->cmd = NULL;
+			cmd->result = ufshcd_transfer_rsp_status(hba, lrbp);
+			ufshcd_release_scsi_cmd(hba, lrbp);
 			/* Do not touch lrbp after scsi done */
 			scsi_done(cmd);
-			ufshcd_release(hba);
-			ufshcd_clk_scaling_update_busy(hba);
 		} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
 			lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
 			if (hba->dev_cmd.complete) {

Re: [PATCH v3 12/17] scsi: ufs: Introduce ufshcd_release_scsi_cmd()

[Adrian Hunter]

On 01/12/2021 01:33, Bart Van Assche wrote:
> The only functional change in this patch is that scsi_done() is now called
> after ufshcd_release() and ufshcd_clk_scaling_update_busy() instead of
> before.
> 
> The next patch in this series will introduce a call to
> ufshcd_release_scsi_cmd() in the abort handler.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/scsi/ufs/ufshcd.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 4e9755c060af..8703e4a70256 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5213,6 +5213,18 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>  	return retval;
>  }
>  
> +/* Release the resources allocated for processing a SCSI command. */
> +static void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
> +				    struct ufshcd_lrb *lrbp)
> +{
> +	struct scsi_cmnd *cmd = lrbp->cmd;
> +
> +	scsi_dma_unmap(cmd);
> +	lrbp->cmd = NULL;	/* Mark the command as completed. */
> +	ufshcd_release(hba);
> +	ufshcd_clk_scaling_update_busy(hba);
> +}
> +
>  /**
>   * __ufshcd_transfer_req_compl - handle SCSI and query command completion
>   * @hba: per adapter instance
> @@ -5223,7 +5235,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>  {
>  	struct ufshcd_lrb *lrbp;
>  	struct scsi_cmnd *cmd;
> -	int result;
>  	int index;
>  
>  	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
> @@ -5234,15 +5245,10 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>  			if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
>  				ufshcd_update_monitor(hba, lrbp);
>  			ufshcd_add_command_trace(hba, index, UFS_CMD_COMP);
> -			result = ufshcd_transfer_rsp_status(hba, lrbp);
> -			scsi_dma_unmap(cmd);
> -			cmd->result = result;
> -			/* Mark completed command as NULL in LRB */
> -			lrbp->cmd = NULL;
> +			cmd->result = ufshcd_transfer_rsp_status(hba, lrbp);
> +			ufshcd_release_scsi_cmd(hba, lrbp);
>  			/* Do not touch lrbp after scsi done */
>  			scsi_done(cmd);
> -			ufshcd_release(hba);
> -			ufshcd_clk_scaling_update_busy(hba);
>  		} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
>  			lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
>  			if (hba->dev_cmd.complete) {
> 

[PATCH v3 13/17] scsi: ufs: Improve SCSI abort handling further

[Bart Van Assche]

Release resources when aborting a command. Make sure that aborted commands
are completed once by clearing the corresponding tag bit from
hba->outstanding_reqs. This patch is an improved version of commit
3ff1f6b6ba6f ("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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8703e4a70256..1a4f2ebf955e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6984,6 +6984,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	unsigned long flags;
 	int err = FAILED;
+	bool outstanding;
 	u32 reg;
 
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
@@ -7061,6 +7062,17 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		goto release;
 	}
 
+	/*
+	 * Clear the corresponding bit from outstanding_reqs since the command
+	 * has been aborted successfully.
+	 */
+	spin_lock_irqsave(&hba->outstanding_lock, flags);
+	outstanding = __test_and_clear_bit(tag, &hba->outstanding_reqs);
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+
+	if (outstanding)
+		ufshcd_release_scsi_cmd(hba, lrbp);
+
 	err = SUCCESS;
 
 release:

Re: [PATCH v3 13/17] scsi: ufs: Improve SCSI abort handling further

[Adrian Hunter]

On 01/12/2021 01:33, Bart Van Assche wrote:
> Release resources when aborting a command. Make sure that aborted commands
> are completed once by clearing the corresponding tag bit from
> hba->outstanding_reqs. This patch is an improved version of commit
> 3ff1f6b6ba6f ("scsi: ufs: core: Improve SCSI abort handling").
> 
> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/scsi/ufs/ufshcd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8703e4a70256..1a4f2ebf955e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6984,6 +6984,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
>  	unsigned long flags;
>  	int err = FAILED;
> +	bool outstanding;
>  	u32 reg;
>  
>  	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> @@ -7061,6 +7062,17 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  		goto release;
>  	}
>  
> +	/*
> +	 * Clear the corresponding bit from outstanding_reqs since the command
> +	 * has been aborted successfully.
> +	 */
> +	spin_lock_irqsave(&hba->outstanding_lock, flags);
> +	outstanding = __test_and_clear_bit(tag, &hba->outstanding_reqs);
> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> +
> +	if (outstanding)
> +		ufshcd_release_scsi_cmd(hba, lrbp);
> +
>  	err = SUCCESS;
>  
>  release:
> 

[PATCH v3 14/17] 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 1a4f2ebf955e..074ea9ae54e0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1648,7 +1648,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++;
@@ -1808,7 +1809,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;
@@ -1943,11 +1944,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 v3 15/17] scsi: ufs: Stop using the clock scaling lock in the error handler

[Bart Van Assche]

Instead of locking and unlocking the clock scaling lock, surround the
command queueing code with an RCU reader lock and call synchronize_rcu().
This patch prepares for removal of the clock scaling lock.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 074ea9ae54e0..c4cf5c4b4893 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2684,6 +2684,12 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	if (!down_read_trylock(&hba->clk_scaling_lock))
 		return SCSI_MLQUEUE_HOST_BUSY;
 
+	/*
+	 * Allows the UFS error handler to wait for prior ufshcd_queuecommand()
+	 * calls.
+	 */
+	rcu_read_lock();
+
 	switch (hba->ufshcd_state) {
 	case UFSHCD_STATE_OPERATIONAL:
 		break;
@@ -2762,7 +2768,10 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	}
 
 	ufshcd_send_command(hba, tag);
+
 out:
+	rcu_read_unlock();
+
 	up_read(&hba->clk_scaling_lock);
 
 	if (ufs_trigger_eh()) {
@@ -5951,8 +5960,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);
 }
 

Re: [PATCH v3 15/17] scsi: ufs: Stop using the clock scaling lock in the error handler

[Adrian Hunter]

On 01/12/2021 01:33, Bart Van Assche wrote:
> Instead of locking and unlocking the clock scaling lock, surround the
> command queueing code with an RCU reader lock and call synchronize_rcu().
> This patch prepares for removal of the clock scaling lock.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/scsi/ufs/ufshcd.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 074ea9ae54e0..c4cf5c4b4893 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2684,6 +2684,12 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  	if (!down_read_trylock(&hba->clk_scaling_lock))
>  		return SCSI_MLQUEUE_HOST_BUSY;
>  
> +	/*
> +	 * Allows the UFS error handler to wait for prior ufshcd_queuecommand()
> +	 * calls.
> +	 */
> +	rcu_read_lock();
> +
>  	switch (hba->ufshcd_state) {
>  	case UFSHCD_STATE_OPERATIONAL:
>  		break;
> @@ -2762,7 +2768,10 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  	}
>  
>  	ufshcd_send_command(hba, tag);
> +
>  out:
> +	rcu_read_unlock();
> +
>  	up_read(&hba->clk_scaling_lock);
>  
>  	if (ufs_trigger_eh()) {
> @@ -5951,8 +5960,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);
>  }
>  
> 

[PATCH v3 16/17] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

Remove the clock scaling lock from ufshcd_queuecommand() since it is a
performance bottleneck. Instead, use synchronize_rcu_expedited() to wait
for ongoing ufshcd_queuecommand() calls.

Cc: Asutosh Das (asd) <asutoshd@codeaurora.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, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c4cf5c4b4893..3e4c62c6f9d2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1196,6 +1196,13 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 	/* let's not get into low power until clock scaling is completed */
 	ufshcd_hold(hba, false);
 
+	/*
+	 * Wait for ongoing ufshcd_queuecommand() calls. Calling
+	 * synchronize_rcu_expedited() instead of synchronize_rcu() reduces the
+	 * waiting time from milliseconds to microseconds.
+	 */
+	synchronize_rcu_expedited();
+
 out:
 	return ret;
 }
@@ -2681,9 +2688,6 @@ 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 the UFS error handler to wait for prior ufshcd_queuecommand()
 	 * calls.
@@ -2772,8 +2776,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 out:
 	rcu_read_unlock();
 
-	up_read(&hba->clk_scaling_lock);
-
 	if (ufs_trigger_eh()) {
 		unsigned long flags;
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c3c2792f309f..411c6015bbfe 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -779,6 +779,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

Re: [PATCH v3 16/17] scsi: ufs: Optimize the command queueing code

[Asutosh Das (asd)]

On 11/30/2021 3:33 PM, Bart Van Assche wrote:
> Remove the clock scaling lock from ufshcd_queuecommand() since it is a
> performance bottleneck. Instead, use synchronize_rcu_expedited() to wait
> for ongoing ufshcd_queuecommand() calls.
> 
> Cc: Asutosh Das (asd) <asutoshd@codeaurora.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, 8 insertions(+), 5 deletions(-)
> 
Hi Bart,
Say an IO (req1) has crossed the scsi_host_queue_ready() check but 
hasn't yet reached ufshcd_queuecommand() and DBR is 0.
ufshcd_clock_scaling_prepare() is invoked and completes and scaling 
proceeds to change the clocks and gear.
I wonder if the IO (req1) would be issued while scaling is in progress.
If so, do you think a check should be added in ufshcd_queuecommand() to 
see if scaling is in progress or if host is blocked?

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c4cf5c4b4893..3e4c62c6f9d2 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1196,6 +1196,13 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
>   	/* let's not get into low power until clock scaling is completed */
>   	ufshcd_hold(hba, false);
>   
> +	/*
> +	 * Wait for ongoing ufshcd_queuecommand() calls. Calling
> +	 * synchronize_rcu_expedited() instead of synchronize_rcu() reduces the
> +	 * waiting time from milliseconds to microseconds.
> +	 */
> +	synchronize_rcu_expedited();
> +
>   out:
>   	return ret;
>   }
> @@ -2681,9 +2688,6 @@ 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 the UFS error handler to wait for prior ufshcd_queuecommand()
>   	 * calls.
> @@ -2772,8 +2776,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>   out:
>   	rcu_read_unlock();
>   
> -	up_read(&hba->clk_scaling_lock);
> -
>   	if (ufs_trigger_eh()) {
>   		unsigned long flags;
>   
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c3c2792f309f..411c6015bbfe 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -779,6 +779,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 v3 16/17] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

On 12/1/21 3:33 PM, Asutosh Das (asd) wrote:
> Hi Bart,
> Say an IO (req1) has crossed the scsi_host_queue_ready() check but hasn't yet reached ufshcd_queuecommand() and DBR is 0.
> ufshcd_clock_scaling_prepare() is invoked and completes and scaling proceeds to change the clocks and gear.
> I wonder if the IO (req1) would be issued while scaling is in progress.
> If so, do you think a check should be added in ufshcd_queuecommand() to see if scaling is in progress or if host is blocked?

How about using the patch below instead of patch 16/17 from this patch
series? The patch below should not trigger the race condition mentioned
above.

Thanks,

Bart.


Subject: [PATCH] scsi: ufs: Optimize the command queueing code

Remove the clock scaling lock from ufshcd_queuecommand() since it is a
performance bottleneck. Instead, wait until all budget_maps have cleared
to wait for ongoing ufshcd_queuecommand() calls.

Cc: Asutosh Das (asd) <asutoshd@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/ufs/ufshcd.c | 33 +++++++++++++++++++++++----------
  drivers/scsi/ufs/ufshcd.h |  1 +
  2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c4cf5c4b4893..be679f2a97da 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1070,13 +1070,31 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
  	return false;
  }

+/*
+ * Determine the number of pending commands by counting the bits in the SCSI
+ * device budget maps. This approach has been selected because a bit is set in
+ * the budget map before scsi_host_queue_ready() checks the host_self_blocked
+ * flag. The host_self_blocked flag can be modified by calling
+ * scsi_block_requests() or scsi_unblock_requests().
+ */
+static u32 ufshcd_pending_cmds(struct ufs_hba *hba)
+{
+	struct scsi_device *sdev;
+	u32 pending;
+
+	shost_for_each_device(sdev, hba->host)
+		pending += sbitmap_weight(&sdev->budget_map);
+
+	return pending;
+}
+
  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;
+	u32 tr_pending;
  	bool timeout = false, do_last_check = false;
  	ktime_t start;

@@ -1094,8 +1112,8 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
  		}

  		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) {
+		tr_pending = ufshcd_pending_cmds(hba);
+		if (!tm_doorbell && !tr_pending) {
  			timeout = false;
  			break;
  		} else if (do_last_check) {
@@ -1115,12 +1133,12 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
  			do_last_check = true;
  		}
  		spin_lock_irqsave(hba->host->host_lock, flags);
-	} while (tm_doorbell || tr_doorbell);
+	} while (tm_doorbell || tr_pending);

  	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);
+			__func__, tm_doorbell, tr_pending);
  		ret = -EBUSY;
  	}
  out:
@@ -2681,9 +2699,6 @@ 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 the UFS error handler to wait for prior ufshcd_queuecommand()
  	 * calls.
@@ -2772,8 +2787,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
  out:
  	rcu_read_unlock();

-	up_read(&hba->clk_scaling_lock);
-
  	if (ufs_trigger_eh()) {
  		unsigned long flags;

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c3c2792f309f..411c6015bbfe 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -779,6 +779,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

Re: [PATCH v3 16/17] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

On 12/2/21 10:13 AM, Bart Van Assche wrote:
> +static u32 ufshcd_pending_cmds(struct ufs_hba *hba)
> +{
> +    struct scsi_device *sdev;
> +    u32 pending;
        ^^^^^^^^^^^

(replying to my own email)

The above should be changed into "u32 pending = 0;"

Thanks,

Bart.

Re: [PATCH v3 16/17] scsi: ufs: Optimize the command queueing code

[Asutosh Das (asd)]

On 12/2/2021 10:13 AM, Bart Van Assche wrote:
> On 12/1/21 3:33 PM, Asutosh Das (asd) wrote:
>> Hi Bart,
>> Say an IO (req1) has crossed the scsi_host_queue_ready() check but 
>> hasn't yet reached ufshcd_queuecommand() and DBR is 0.
>> ufshcd_clock_scaling_prepare() is invoked and completes and scaling 
>> proceeds to change the clocks and gear.
>> I wonder if the IO (req1) would be issued while scaling is in progress.
>> If so, do you think a check should be added in ufshcd_queuecommand() 
>> to see if scaling is in progress or if host is blocked?
> 
> How about using the patch below instead of patch 16/17 from this patch
> series? The patch below should not trigger the race condition mentioned
> above.
> 
> Thanks,
> 
> Bart.
> 
Hi Bart,
This looks like it'd work.

-asd
> 
> Subject: [PATCH] scsi: ufs: Optimize the command queueing code
> 
> Remove the clock scaling lock from ufshcd_queuecommand() since it is a
> performance bottleneck. Instead, wait until all budget_maps have cleared
> to wait for ongoing ufshcd_queuecommand() calls.
> 
> Cc: Asutosh Das (asd) <asutoshd@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/ufs/ufshcd.c | 33 +++++++++++++++++++++++----------
>   drivers/scsi/ufs/ufshcd.h |  1 +
>   2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c4cf5c4b4893..be679f2a97da 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1070,13 +1070,31 @@ static bool 
> ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
>       return false;
>   }
> 
> +/*
> + * Determine the number of pending commands by counting the bits in the 
> SCSI
> + * device budget maps. This approach has been selected because a bit is 
> set in
> + * the budget map before scsi_host_queue_ready() checks the 
> host_self_blocked
> + * flag. The host_self_blocked flag can be modified by calling
> + * scsi_block_requests() or scsi_unblock_requests().
> + */
> +static u32 ufshcd_pending_cmds(struct ufs_hba *hba)
> +{
> +    struct scsi_device *sdev;
> +    u32 pending;
> +
> +    shost_for_each_device(sdev, hba->host)
> +        pending += sbitmap_weight(&sdev->budget_map);
> +
> +    return pending;
> +}
> +
>   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;
> +    u32 tr_pending;
>       bool timeout = false, do_last_check = false;
>       ktime_t start;
> 
> @@ -1094,8 +1112,8 @@ static int ufshcd_wait_for_doorbell_clr(struct 
> ufs_hba *hba,
>           }
> 
>           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) {
> +        tr_pending = ufshcd_pending_cmds(hba);
> +        if (!tm_doorbell && !tr_pending) {
>               timeout = false;
>               break;
>           } else if (do_last_check) {
> @@ -1115,12 +1133,12 @@ static int ufshcd_wait_for_doorbell_clr(struct 
> ufs_hba *hba,
>               do_last_check = true;
>           }
>           spin_lock_irqsave(hba->host->host_lock, flags);
> -    } while (tm_doorbell || tr_doorbell);
> +    } while (tm_doorbell || tr_pending);
> 
>       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);
> +            __func__, tm_doorbell, tr_pending);
>           ret = -EBUSY;
>       }
>   out:
> @@ -2681,9 +2699,6 @@ 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 the UFS error handler to wait for prior 
> ufshcd_queuecommand()
>        * calls.
> @@ -2772,8 +2787,6 @@ static int ufshcd_queuecommand(struct Scsi_Host 
> *host, struct scsi_cmnd *cmd)
>   out:
>       rcu_read_unlock();
> 
> -    up_read(&hba->clk_scaling_lock);
> -
>       if (ufs_trigger_eh()) {
>           unsigned long flags;
> 
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c3c2792f309f..411c6015bbfe 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -779,6 +779,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

[PATCH v3 17/17] scsi: ufs: Implement polling support

[Bart Van Assche]

The time spent in io_schedule() and also the interrupt latency are
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.

Although the block layer supports to partition the tag space for
interrupt-based completions (HCTX_TYPE_DEFAULT) purposes and polling
(HCTX_TYPE_POLL), the choice has been made to use the same hardware
queue for both hctx types because partitioning the tag space would
negatively affect performance.

On my test setup this patch increases IOPS from 2736 to 22000 (8x) for the
following test:

for hipri in 0 1; do
    fio --ioengine=io_uring --iodepth=1 --rw=randread \
    --runtime=60 --time_based=1 --direct=1 --name=qd1 \
    --filename=/dev/block/sda --ioscheduler=none --gtod_reduce=1 \
    --norandommap --hipri=$hipri
done

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3e4c62c6f9d2..5b3efc880246 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2651,6 +2651,36 @@ static inline bool is_device_wlun(struct scsi_device *sdev)
 		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN);
 }
 
+/*
+ * Associate the UFS controller queue with the default and poll HCTX types.
+ * Initialize the mq_map[] arrays.
+ */
+static int ufshcd_map_queues(struct Scsi_Host *shost)
+{
+	int i, ret;
+
+	for (i = 0; i < shost->nr_maps; i++) {
+		struct blk_mq_queue_map *map = &shost->tag_set.map[i];
+
+		switch (i) {
+		case HCTX_TYPE_DEFAULT:
+		case HCTX_TYPE_POLL:
+			map->nr_queues = 1;
+			break;
+		case HCTX_TYPE_READ:
+			map->nr_queues = 0;
+			break;
+		default:
+			WARN_ON_ONCE(true);
+		}
+		map->queue_offset = 0;
+		ret = blk_mq_map_queues(map);
+		WARN_ON_ONCE(ret);
+	}
+
+	return 0;
+}
+
 static void ufshcd_init_lrb(struct ufs_hba *hba, struct ufshcd_lrb *lrb, int i)
 {
 	struct utp_transfer_cmd_desc *cmd_descp = hba->ucdl_base_addr;
@@ -2686,7 +2716,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	struct ufshcd_lrb *lrbp;
 	int err = 0;
 
-	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
+	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
 
 	/*
 	 * Allows the UFS error handler to wait for prior ufshcd_queuecommand()
@@ -5277,6 +5307,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
@@ -5287,9 +5342,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
@@ -5304,21 +5356,13 @@ 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);
+	/*
+	 * Ignore the ufshcd_poll() return value and return IRQ_HANDLED since we
+	 * do not want polling to trigger spurious interrupt complaints.
+	 */
+	ufshcd_poll(hba->host, 0);
 
-	if (completed_reqs) {
-		__ufshcd_transfer_req_compl(hba, completed_reqs);
-		return IRQ_HANDLED;
-	} else {
-		return IRQ_NONE;
-	}
+	return IRQ_HANDLED;
 }
 
 int __ufshcd_write_ee_control(struct ufs_hba *hba, u32 ee_ctrl_mask)
@@ -6570,6 +6614,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	spin_lock_irqsave(host->host_lock, flags);
 
 	task_tag = req->tag;
+	WARN_ONCE(task_tag < 0 || task_tag >= hba->nutmrs, "Invalid tag %d\n",
+		  task_tag);
 	hba->tmf_rqs[req->tag] = req;
 	treq->upiu_req.req_header.dword_0 |= cpu_to_be32(task_tag);
 
@@ -8133,7 +8179,9 @@ static 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,
@@ -9422,6 +9470,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 		err = -ENOMEM;
 		goto out_error;
 	}
+	host->nr_maps = HCTX_TYPE_POLL + 1;
 	hba = shost_priv(host);
 	hba->host = host;
 	hba->dev = dev;

Re: [PATCH v3 00/17] UFS patches for kernel v5.17

[Bean Huo]

Reply in the email to cover each sub-patch:

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


On Tue, 2021-11-30 at 15:33 -0800, Bart Van Assche wrote:
> Hi Martin,
> 
> This patch series includes the following changes:
> - Fix a deadlock in the UFS error handler.
> - Add polling support in the UFS driver.
> - Several smaller fixes for the UFS driver.
> 
> Please consider these UFS driver kernel patches for kernel v5.17.
> 
> Thanks,
> 
> Bart.
> 
> Changes compared to v2:
> - Dropped SCSI core patches that add support for internal commands.
> - Reworked patch "Fix a deadlock in the error handler" such that it
> uses a
>   reserved tag as proposed by Adrian.
> - Split patch "ufs: Introduce ufshcd_release_scsi_cmd()" into two
> patches.
> 
> Changes compared to v1:
> - Add internal command support to the SCSI core.
> - Reworked patch "ufs: Optimize the command queueing code".
> 
> Bart Van Assche (17):
>   scsi: core: Fix scsi_device_max_queue_depth()
>   scsi: core: Fix a race between scsi_done() and scsi_times_out()
>   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: ufs: Fix race conditions related to driver data
>   scsi: ufs: Remove ufshcd_any_tag_in_use()
>   scsi: ufs: Rework ufshcd_change_queue_depth()
>   scsi: ufs: Fix a deadlock in the error handler
>   scsi: ufs: Remove the 'update_scaling' local variable
>   scsi: ufs: Introduce ufshcd_release_scsi_cmd()
>   scsi: ufs: Improve SCSI abort handling further
>   scsi: ufs: Fix a kernel crash during shutdown
>   scsi: ufs: Stop using the clock scaling lock in the error handler
>   scsi: ufs: Optimize the command queueing code
>   scsi: ufs: Implement polling support
> 
>  drivers/scsi/scsi.c                |   4 +-
>  drivers/scsi/scsi_error.c          |  22 +--
>  drivers/scsi/ufs/tc-dwc-g210-pci.c |   1 -
>  drivers/scsi/ufs/ufs-exynos.c      |   4 +-
>  drivers/scsi/ufs/ufshcd-pci.c      |   2 -
>  drivers/scsi/ufs/ufshcd-pltfrm.c   |   2 -
>  drivers/scsi/ufs/ufshcd.c          | 268 ++++++++++++++++-----------
> --
>  drivers/scsi/ufs/ufshcd.h          |   7 +-
>  8 files changed, 165 insertions(+), 145 deletions(-)
>