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

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

[Bart Van Assche]

Hi Martin,

This patch series includes the following changes:
- Introduce internal command and reserved tag support in the SCSI core.
- 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 v1:
- Introduced the symbolic constant UFSHCD_NUM_RESERVED.
- Changed the behavior of the patch that removes the clock scaling lock from
  ufshcd_queuecommand() such that it again waits until all pending commands
  have finished before changing the clock frequency.
- Split the patch that optimizes ufshcd_queuecommand().
- Added patches that introduce support for internal command management in the
  SCSI core.
- Introduced a new function ufshcd_release_scsi_cmd().

Bart Van Assche (18):
  scsi: core: Unexport scsi_track_queue_full()
  scsi: core: Fix scsi_device_max_queue_depth()
  scsi: core: Fix a race between scsi_done() and scsi_times_out()
  scsi: core: Add support for reserved tags
  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: Switch to scsi_(get|put)_internal_cmd()
  scsi: ufs: Rework ufshcd_change_queue_depth()
  scsi: ufs: Fix a deadlock in the error handler
  scsi: ufs: Introduce ufshcd_release_scsi_cmd()
  scsi: ufs: Improve SCSI abort handling
  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
  scsi: ufs: Fix race conditions related to driver data

Hannes Reinecke (2):
  block: Add a flag for internal commands
  scsi: core: Add support for internal commands

 block/blk-exec.c                   |   5 +
 drivers/scsi/scsi.c                |   5 +-
 drivers/scsi/scsi_error.c          |  29 +--
 drivers/scsi/scsi_lib.c            |  50 +++-
 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          | 375 ++++++++++++++++-------------
 drivers/scsi/ufs/ufshcd.h          |   5 +-
 include/linux/blk-mq.h             |   5 +
 include/linux/blk_types.h          |   2 +
 include/scsi/scsi_device.h         |   4 +
 include/scsi/scsi_host.h           |   9 +-
 14 files changed, 301 insertions(+), 197 deletions(-)

[PATCH v2 01/20] block: Add a flag for internal commands

[Bart Van Assche]

From: Hannes Reinecke <hare@suse.de>

Some drivers use a single tag space for requests submitted by the block
layer and driver-internal requests. Driver-internal requests will never
pass through the block layer but require a valid tag. This patch adds a
new request flag REQ_INTERNAL to mark such requests and a terminates any
such commands in blk_execute_rq_nowait() with a WARN_ON_ONCE() to signal
such an invalid usage.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: John Garry <john.garry@huawei.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
[ bvanassche: modified patch description ]
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-exec.c          | 5 +++++
 include/linux/blk-mq.h    | 5 +++++
 include/linux/blk_types.h | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/block/blk-exec.c b/block/blk-exec.c
index 1b8b47f6e79b..27d2e3779c13 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -53,6 +53,11 @@ void blk_execute_rq_nowait(struct gendisk *bd_disk, struct request *rq,
 	rq->rq_disk = bd_disk;
 	rq->end_io = done;
 
+	if (WARN_ON_ONCE(blk_rq_is_internal(rq))) {
+		blk_mq_end_request(rq, BLK_STS_NOTSUPP);
+		return;
+	}
+
 	blk_account_io_start(rq);
 
 	/*
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 2949d9ac7484..3b42fcdf0c15 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -208,6 +208,11 @@ static inline bool blk_rq_is_passthrough(struct request *rq)
 	return blk_op_is_passthrough(req_op(rq));
 }
 
+static inline bool blk_rq_is_internal(struct request *rq)
+{
+	return rq->cmd_flags & REQ_INTERNAL;
+}
+
 static inline unsigned short req_get_ioprio(struct request *req)
 {
 	return req->ioprio;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index fe065c394fff..1ae2365e02d1 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -411,6 +411,7 @@ enum req_flag_bits {
 	/* for driver use */
 	__REQ_DRV,
 	__REQ_SWAP,		/* swapping request. */
+	__REQ_INTERNAL,		/* driver-internal command */
 	__REQ_NR_BITS,		/* stops here */
 };
 
@@ -435,6 +436,7 @@ enum req_flag_bits {
 
 #define REQ_DRV			(1ULL << __REQ_DRV)
 #define REQ_SWAP		(1ULL << __REQ_SWAP)
+#define REQ_INTERNAL		(1ULL << __REQ_INTERNAL)
 
 #define REQ_FAILFAST_MASK \
 	(REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER)

Re: [PATCH v2 01/20] block: Add a flag for internal commands

[John Garry]

On 19/11/2021 19:57, Bart Van Assche wrote:
> From: Hannes Reinecke <hare@suse.de> >
> Some drivers use a single tag space for requests submitted by the block
> layer and driver-internal requests. Driver-internal requests will never
> pass through the block layer but require a valid tag. This patch adds a
> new request flag REQ_INTERNAL.

I'm not sure on the name. Don't we already use term "internal" for 
elevator request tag?

> to mark such requests and a terminates any
> such commands in blk_execute_rq_nowait() with a WARN_ON_ONCE() to signal
> such an invalid usage.
FYI, I have been working on a different stream, that allows us to send 
the reserved request through the block layer, as we need it for poll 
mode support. The reason is that we need to send reserved requests on 
specific HW queues, which may be polling. However poll mode support only 
allows us to poll requests with bios, so that's a problem ATM.

> 
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: John Garry <john.garry@huawei.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> [ bvanassche: modified patch description ]
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   block/blk-exec.c          | 5 +++++
>   include/linux/blk-mq.h    | 5 +++++
>   include/linux/blk_types.h | 2 ++
>   3 files changed, 12 insertions(+)
> 
> diff --git a/block/blk-exec.c b/block/blk-exec.c
> index 1b8b47f6e79b..27d2e3779c13 100644
> --- a/block/blk-exec.c
> +++ b/block/blk-exec.c
> @@ -53,6 +53,11 @@ void blk_execute_rq_nowait(struct gendisk *bd_disk, struct request *rq,
>   	rq->rq_disk = bd_disk;
>   	rq->end_io = done;
>   
> +	if (WARN_ON_ONCE(blk_rq_is_internal(rq))) {
> +		blk_mq_end_request(rq, BLK_STS_NOTSUPP);
> +		return;
> +	}
> +
>   	blk_account_io_start(rq);
>   
>   	/*
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 2949d9ac7484..3b42fcdf0c15 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -208,6 +208,11 @@ static inline bool blk_rq_is_passthrough(struct request *rq)
>   	return blk_op_is_passthrough(req_op(rq));
>   }
>   
> +static inline bool blk_rq_is_internal(struct request *rq)
> +{
> +	return rq->cmd_flags & REQ_INTERNAL;
> +}
> +
>   static inline unsigned short req_get_ioprio(struct request *req)
>   {
>   	return req->ioprio;
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index fe065c394fff..1ae2365e02d1 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -411,6 +411,7 @@ enum req_flag_bits {
>   	/* for driver use */
>   	__REQ_DRV,
>   	__REQ_SWAP,		/* swapping request. */
> +	__REQ_INTERNAL,		/* driver-internal command */
>   	__REQ_NR_BITS,		/* stops here */
>   };
>   
> @@ -435,6 +436,7 @@ enum req_flag_bits {
>   
>   #define REQ_DRV			(1ULL << __REQ_DRV)
>   #define REQ_SWAP		(1ULL << __REQ_SWAP)
> +#define REQ_INTERNAL		(1ULL << __REQ_INTERNAL)
>   
>   #define REQ_FAILFAST_MASK \
>   	(REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER)
> .
> 

Re: [PATCH v2 01/20] block: Add a flag for internal commands

[Bart Van Assche]

On 11/22/21 12:46 AM, John Garry wrote:
> On 19/11/2021 19:57, Bart Van Assche wrote:
>> From: Hannes Reinecke <hare@suse.de> >
>> Some drivers use a single tag space for requests submitted by the block
>> layer and driver-internal requests. Driver-internal requests will never
>> pass through the block layer but require a valid tag. This patch adds a
>> new request flag REQ_INTERNAL.
> 
> I'm not sure on the name. Don't we already use term "internal" for 
> elevator request tag?

I don't see how any confusion could arise between "internal_tag" and 
"REQ_INTERNAL" since in both cases the context is made clear - either 
the request tag or the request in its entirety.

>> to mark such requests and a terminates any
>> such commands in blk_execute_rq_nowait() with a WARN_ON_ONCE() to signal
>> such an invalid usage.
 >
> FYI, I have been working on a different stream, that allows us to send 
> the reserved request through the block layer, as we need it for poll 
> mode support. The reason is that we need to send reserved requests on 
> specific HW queues, which may be polling. However poll mode support only 
> allows us to poll requests with bios, so that's a problem ATM.

Please use REQ_OP_DRV_* without REQ_INTERNAL for requests that need to 
be sent through the block layer.

Thanks,

Bart.

[PATCH v2 02/20] scsi: core: Unexport scsi_track_queue_full()

[Bart Van Assche]

Commit 408dea0ed41c ("[PATCH] kill scsi_syms.c") exported
scsi_track_queue_full() and introduced a call to that function in
drivers/scsi/tmscsim.c. Commit 71bd849dbac9 ("tmscsim: replace by am53c974
driver") removed source file drivers/scsi/tmscsim.c. Since
scsi_track_queue_full() is no longer used outside the SCSI core, unexport
this function.

Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index f6af1562cba4..ace6d477034b 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -276,7 +276,6 @@ int scsi_track_queue_full(struct scsi_device *sdev, int depth)
 
 	return scsi_change_queue_depth(sdev, depth);
 }
-EXPORT_SYMBOL(scsi_track_queue_full);
 
 /**
  * scsi_vpd_inquiry - Request a device provide us with a VPD page

[PATCH v2 03/20] 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 ace6d477034b..0d2a91aa1f13 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -201,11 +201,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);
 }
 
 /**

[PATCH v2 04/20] 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);

[PATCH v2 05/20] scsi: core: Add support for internal commands

[Bart Van Assche]

From: Hannes Reinecke <hare@suse.de>

Add helper functions to allow LLDs to allocate and free internal commands.
This patch is based on Hannes' patch with the subject "scsi: add
scsi_{get,put}_internal_cmd() helper".

Cc: John Garry <john.garry@huawei.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
[ bvanassche: changed type of the first argument of the new functions from
  struct scsi_device * into struct request_queue * and included changes for
  the timeout handlers in this patch. ]
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_error.c  |  7 ++++++
 drivers/scsi/scsi_lib.c    | 46 +++++++++++++++++++++++++++++++++++++-
 include/scsi/scsi_device.h |  4 ++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index cd05f2db3339..3703ee9c89dd 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -339,6 +339,13 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 	if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
 		return BLK_EH_DONE;
 
+	/*
+	 * The code below is for documentation purposes only since the
+	 * dereference above of the scmd->device pointer triggers a kernel
+	 * oops for internal commands.
+	 */
+	WARN_ON_ONCE(blk_rq_is_internal(scsi_cmd_to_rq(scmd)));
+
 	trace_scsi_dispatch_cmd_timeout(scmd);
 	scsi_log_completion(scmd, TIMEOUT_ERROR);
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 621d841d819a..59c3c4fbcfc0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1756,8 +1756,9 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 static enum blk_eh_timer_return scsi_timeout(struct request *req,
 		bool reserved)
 {
-	if (reserved)
+	if (blk_rq_is_internal(req) || WARN_ON_ONCE(reserved))
 		return BLK_EH_RESET_TIMER;
+
 	return scsi_times_out(req);
 }
 
@@ -1957,6 +1958,49 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
 	blk_mq_free_tag_set(&shost->tag_set);
 }
 
+/**
+ * scsi_get_internal_cmd - Allocate an internal SCSI command
+ * @q: request queue from which to allocate the command. This request queue may
+ *	but does not have to be associated with a SCSI device. This request
+ *	queue must be associated with a SCSI tag set. See also
+ *	scsi_mq_setup_tags().
+ * @data_direction: Data direction for the allocated command.
+ * @flags: Zero or more BLK_MQ_REQ_* flags.
+ *
+ * Allocates a request for driver-internal use. The tag of the returned SCSI
+ * command is guaranteed to be unique.
+ */
+struct scsi_cmnd *scsi_get_internal_cmd(struct request_queue *q,
+					enum dma_data_direction data_direction,
+					blk_mq_req_flags_t flags)
+{
+	unsigned int opf = REQ_INTERNAL;
+	struct request *rq;
+
+	opf |= data_direction == DMA_TO_DEVICE ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
+	rq = blk_mq_alloc_request(q, opf, flags);
+	if (IS_ERR(rq))
+		return ERR_CAST(rq);
+	return blk_mq_rq_to_pdu(rq);
+}
+EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);
+
+/**
+ * scsi_put_internal_cmd - Free an internal SCSI command
+ * @scmd: SCSI command to be freed
+ *
+ * Check if @scmd is an internal command and call blk_mq_free_request() if true.
+ */
+void scsi_put_internal_cmd(struct scsi_cmnd *scmd)
+{
+	struct request *rq = blk_mq_rq_from_pdu(scmd);
+
+	if (WARN_ON_ONCE(!blk_rq_is_internal(rq)))
+		return;
+	blk_mq_free_request(rq);
+}
+EXPORT_SYMBOL_GPL(scsi_put_internal_cmd);
+
 /**
  * scsi_device_from_queue - return sdev associated with a request_queue
  * @q: The request queue to return the sdev from
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d1c6fc83b1e3..348c12274324 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -9,6 +9,7 @@
 #include <scsi/scsi.h>
 #include <linux/atomic.h>
 #include <linux/sbitmap.h>
+#include <linux/dma-direction.h>
 
 struct bsg_device;
 struct device;
@@ -470,6 +471,9 @@ static inline int scsi_execute_req(struct scsi_device *sdev,
 	return scsi_execute(sdev, cmd, data_direction, buffer,
 		bufflen, NULL, sshdr, timeout, retries,  0, 0, resid);
 }
+struct scsi_cmnd *scsi_get_internal_cmd(struct request_queue *q,
+	enum dma_data_direction data_direction, blk_mq_req_flags_t flags);
+void scsi_put_internal_cmd(struct scsi_cmnd *scmd);
 extern void sdev_disable_disk_events(struct scsi_device *sdev);
 extern void sdev_enable_disk_events(struct scsi_device *sdev);
 extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[John Garry]

On 19/11/2021 19:57, Bart Van Assche wrote:
> +/**
> + * scsi_get_internal_cmd - Allocate an internal SCSI command
> + * @q: request queue from which to allocate the command. This request queue may
> + *	but does not have to be associated with a SCSI device. This request
> + *	queue must be associated with a SCSI tag set. See also
> + *	scsi_mq_setup_tags().
> + * @data_direction: Data direction for the allocated command.
> + * @flags: Zero or more BLK_MQ_REQ_* flags.
> + *
> + * Allocates a request for driver-internal use. The tag of the returned SCSI
> + * command is guaranteed to be unique.
> + */
> +struct scsi_cmnd *scsi_get_internal_cmd(struct request_queue *q,
> +					enum dma_data_direction data_direction,
> +					blk_mq_req_flags_t flags)

I'd pass the Scsi_Host or scsi_device rather than a request q, so maybe:

struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, ..)
struct scsi_cmnd *scsi_host_get_internal_cmd(struct Scsi_Host *shost, ..)

> +{
> +	unsigned int opf = REQ_INTERNAL;
> +	struct request *rq;
> +
> +	opf |= data_direction == DMA_TO_DEVICE ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
> +	rq = blk_mq_alloc_request(q, opf, flags);
> +	if (IS_ERR(rq))
> +		return ERR_CAST(rq);

I think that Christoph suggested elsewhere that we should poison all the 
scsi_cmnd

> +	return blk_mq_rq_to_pdu(rq);
> +}
> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[Bart Van Assche]

On 11/22/21 12:58 AM, John Garry wrote:
> On 19/11/2021 19:57, Bart Van Assche wrote:
>> +/**
>> + * scsi_get_internal_cmd - Allocate an internal SCSI command
>> + * @q: request queue from which to allocate the command. This request 
>> queue may
>> + *    but does not have to be associated with a SCSI device. This 
>> request
>> + *    queue must be associated with a SCSI tag set. See also
>> + *    scsi_mq_setup_tags().
>> + * @data_direction: Data direction for the allocated command.
>> + * @flags: Zero or more BLK_MQ_REQ_* flags.
>> + *
>> + * Allocates a request for driver-internal use. The tag of the 
>> returned SCSI
>> + * command is guaranteed to be unique.
>> + */
>> +struct scsi_cmnd *scsi_get_internal_cmd(struct request_queue *q,
>> +                    enum dma_data_direction data_direction,
>> +                    blk_mq_req_flags_t flags)
> 
> I'd pass the Scsi_Host or scsi_device rather than a request q, so maybe:
> 
> struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, ..)
> struct scsi_cmnd *scsi_host_get_internal_cmd(struct Scsi_Host *shost, ..)

Passing a request queue pointer as first argument instead of a struct 
scsi_device is a deliberate choice. In the UFS driver (and probably also 
in other SCSI LLDs) we want to allocate internal requests without these 
requests being visible in any existing SCSI device statistics. Creating 
a new SCSI device for the allocation of internal requests is not a good 
choice because that new SCSI device would have to be assigned a LUN 
number and would be visible in sysfs. Hence the choice to allocate 
internal requests from a request queue that is not associated with any 
SCSI device.

>> +{
>> +    unsigned int opf = REQ_INTERNAL;
>> +    struct request *rq;
>> +
>> +    opf |= data_direction == DMA_TO_DEVICE ? REQ_OP_DRV_OUT : 
>> REQ_OP_DRV_IN;
>> +    rq = blk_mq_alloc_request(q, opf, flags);
>> +    if (IS_ERR(rq))
>> +        return ERR_CAST(rq);
> 
> I think that Christoph suggested elsewhere that we should poison all the 
> scsi_cmnd

I had overlooked that comment. I will look into this.

Thanks,

Bart.

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[John Garry]

On 22/11/2021 17:46, Bart Van Assche wrote:
>>
>> struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, ..)
>> struct scsi_cmnd *scsi_host_get_internal_cmd(struct Scsi_Host *shost, ..)
> 
> Passing a request queue pointer as first argument instead of a struct 
> scsi_device is a deliberate choice. In the UFS driver (and probably also 
> in other SCSI LLDs) we want to allocate internal requests without these 
> requests being visible in any existing SCSI device statistics. 


> Creating 
> a new SCSI device for the allocation of internal requests is not a good 
> choice because that new SCSI device would have to be assigned a LUN 
> number and would be visible in sysfs.

But I was not suggesting that.

Hannes series which he points us at recently added an "admin" 
request_queue for a scsi_host, for when we don't want or need a reserved 
commands associated with an sdev. I assumed that you would do something 
similar to this, which would replace ufs_hba.cmd_queue.

Thanks,
John

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[Bart Van Assche]

On 11/22/21 9:46 AM, Bart Van Assche wrote:
> On 11/22/21 12:58 AM, John Garry wrote:
>> On 19/11/2021 19:57, Bart Van Assche wrote:
>>> +{
>>> +    unsigned int opf = REQ_INTERNAL;
>>> +    struct request *rq;
>>> +
>>> +    opf |= data_direction == DMA_TO_DEVICE ? REQ_OP_DRV_OUT : 
>>> REQ_OP_DRV_IN;
>>> +    rq = blk_mq_alloc_request(q, opf, flags);
>>> +    if (IS_ERR(rq))
>>> +        return ERR_CAST(rq);
>>
>> I think that Christoph suggested elsewhere that we should poison all 
>> the scsi_cmnd
> 
> I had overlooked that comment. I will look into this.

If anyone comes up with a good approach for poisoning the scsi_cmnd I 
will look into this. Only overwriting struct scsi_cmnd is not acceptable 
since that would result in a memory leak. See also the memory allocation
statements in scsi_mq_init_request().

Thanks,

Bart.

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[Hannes Reinecke]

On 11/22/21 6:46 PM, Bart Van Assche wrote:
> On 11/22/21 12:58 AM, John Garry wrote:
>> On 19/11/2021 19:57, Bart Van Assche wrote:
>>> +/**
>>> + * scsi_get_internal_cmd - Allocate an internal SCSI command
>>> + * @q: request queue from which to allocate the command. This
>>> request queue may
>>> + *    but does not have to be associated with a SCSI device. This
>>> request
>>> + *    queue must be associated with a SCSI tag set. See also
>>> + *    scsi_mq_setup_tags().
>>> + * @data_direction: Data direction for the allocated command.
>>> + * @flags: Zero or more BLK_MQ_REQ_* flags.
>>> + *
>>> + * Allocates a request for driver-internal use. The tag of the
>>> returned SCSI
>>> + * command is guaranteed to be unique.
>>> + */
>>> +struct scsi_cmnd *scsi_get_internal_cmd(struct request_queue *q,
>>> +                    enum dma_data_direction data_direction,
>>> +                    blk_mq_req_flags_t flags)
>>
>> I'd pass the Scsi_Host or scsi_device rather than a request q, so maybe:
>>
>> struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, ..)
>> struct scsi_cmnd *scsi_host_get_internal_cmd(struct Scsi_Host *shost, ..)
> 
> Passing a request queue pointer as first argument instead of a struct
> scsi_device is a deliberate choice. In the UFS driver (and probably also
> in other SCSI LLDs) we want to allocate internal requests without these
> requests being visible in any existing SCSI device statistics. Creating
> a new SCSI device for the allocation of internal requests is not a good
> choice because that new SCSI device would have to be assigned a LUN
> number and would be visible in sysfs. Hence the choice to allocate
> internal requests from a request queue that is not associated with any
> SCSI device.
> 
It's actually a bit more involved.

The biggest issue is that the SCSI layer is littered with the assumption
that there _will_ be a ->device pointer in struct scsi_cmnd.
If we make up a scsi_cmnd structure _without_ that we'll have to audit
the entire stack to ensure we're not tripping over a NULL device pointer.
And to make matters worse, we also need to audit the completion path in
the driver, which typically have the same 'issue'.

Case in point:

# git grep -- '->device' drivers/scsi | wc --lines
2712

Which was the primary reason for adding a stub device to the SCSI Host;
simply to avoid all the pointless churn and have a valid device for all
commands.

The only way I can see how to avoid getting dragged down into that
rat-hole is to _not_ returning a scsi_cmnd, but rather something else
entirely; that's the avenue I've exploited with my last patchset which
would just return a tag number.
But as there are drivers which really need a scsi_cmnd I can't se how we
can get away with not having a stub scsi_device for the scsi host.

And that won't even show up in sysfs if we assign it a LUN number beyond
the addressable range; 'max_id':0 tends to be a safe choice here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[Bart Van Assche]

On 11/23/21 12:13 AM, Hannes Reinecke wrote:
> It's actually a bit more involved.
> 
> The biggest issue is that the SCSI layer is littered with the assumption
> that there _will_ be a ->device pointer in struct scsi_cmnd.
> If we make up a scsi_cmnd structure _without_ that we'll have to audit
> the entire stack to ensure we're not tripping over a NULL device pointer.
> And to make matters worse, we also need to audit the completion path in
> the driver, which typically have the same 'issue'.
> 
> Case in point:
> 
> # git grep -- '->device' drivers/scsi | wc --lines
> 2712
> 
> Which was the primary reason for adding a stub device to the SCSI Host;
> simply to avoid all the pointless churn and have a valid device for all
> commands.
> 
> The only way I can see how to avoid getting dragged down into that
> rat-hole is to _not_ returning a scsi_cmnd, but rather something else
> entirely; that's the avenue I've exploited with my last patchset which
> would just return a tag number.
> But as there are drivers which really need a scsi_cmnd I can't se how we
> can get away with not having a stub scsi_device for the scsi host.
> 
> And that won't even show up in sysfs if we assign it a LUN number beyond
> the addressable range; 'max_id':0 tends to be a safe choice here.

There is no risk that the scsi_cmnd.device member will be dereferenced for
internal requests allocated by the UFS driver. But I understand from your
email that making sure that the scsi_cmnd.device member is not NULL is
important for other SCSI LLDs. I will look into the approach of associating
a stub SCSI device with internal requests.

Thanks,

Bart.

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[Bart Van Assche]

On 11/23/21 9:46 AM, Bart Van Assche wrote:
> On 11/23/21 12:13 AM, Hannes Reinecke wrote:
>> It's actually a bit more involved.
>>
>> The biggest issue is that the SCSI layer is littered with the assumption
>> that there _will_ be a ->device pointer in struct scsi_cmnd.
>> If we make up a scsi_cmnd structure _without_ that we'll have to audit
>> the entire stack to ensure we're not tripping over a NULL device pointer.
>> And to make matters worse, we also need to audit the completion path in
>> the driver, which typically have the same 'issue'.
>>
>> Case in point:
>>
>> # git grep -- '->device' drivers/scsi | wc --lines
>> 2712
>>
>> Which was the primary reason for adding a stub device to the SCSI Host;
>> simply to avoid all the pointless churn and have a valid device for all
>> commands.
>>
>> The only way I can see how to avoid getting dragged down into that
>> rat-hole is to _not_ returning a scsi_cmnd, but rather something else
>> entirely; that's the avenue I've exploited with my last patchset which
>> would just return a tag number.
>> But as there are drivers which really need a scsi_cmnd I can't se how we
>> can get away with not having a stub scsi_device for the scsi host.
>>
>> And that won't even show up in sysfs if we assign it a LUN number beyond
>> the addressable range; 'max_id':0 tends to be a safe choice here.
> 
> There is no risk that the scsi_cmnd.device member will be dereferenced for
> internal requests allocated by the UFS driver. But I understand from your
> email that making sure that the scsi_cmnd.device member is not NULL is
> important for other SCSI LLDs. I will look into the approach of associating
> a stub SCSI device with internal requests.

(replying to my own email)

Hi Hannes,

Allocating a struct scsi_device for internal requests seems tricky to me. The
most straightforward approach would be to call scsi_alloc_sdev(). However, that
function accepts a scsi_target pointer and calls .slave_alloc(). So a
scsi_target structure would have to be set up before that function is called and
SCSI LLDs would have to be audited to verify that .slave_alloc() works fine for
the H:C:I:L tuple assigned to the fake SCSI device. Additionally, how should the
inquiry data be initialized that is filled in by scsi_add_lun()?

Since I do not use SCSI hardware that needs a scsi_device to be associated with
internal requests, I prefer that this functionality is implemented in a future
patch series. Changing the hba->host->internal_queue occurrences in the UFS
driver into something like hba->host->internal_sdev->request_queue once this
functionality is implemented should be easy.

Thanks,

Bart.

Re: [PATCH v2 05/20] scsi: core: Add support for internal commands

[Hannes Reinecke]

On 11/23/21 8:18 PM, Bart Van Assche wrote:
> On 11/23/21 9:46 AM, Bart Van Assche wrote:
>> On 11/23/21 12:13 AM, Hannes Reinecke wrote:
>>> It's actually a bit more involved.
>>>
>>> The biggest issue is that the SCSI layer is littered with the assumption
>>> that there _will_ be a ->device pointer in struct scsi_cmnd.
>>> If we make up a scsi_cmnd structure _without_ that we'll have to audit
>>> the entire stack to ensure we're not tripping over a NULL device 
>>> pointer.
>>> And to make matters worse, we also need to audit the completion path in
>>> the driver, which typically have the same 'issue'.
>>>
>>> Case in point:
>>>
>>> # git grep -- '->device' drivers/scsi | wc --lines
>>> 2712
>>>
>>> Which was the primary reason for adding a stub device to the SCSI Host;
>>> simply to avoid all the pointless churn and have a valid device for all
>>> commands.
>>>
>>> The only way I can see how to avoid getting dragged down into that
>>> rat-hole is to _not_ returning a scsi_cmnd, but rather something else
>>> entirely; that's the avenue I've exploited with my last patchset which
>>> would just return a tag number.
>>> But as there are drivers which really need a scsi_cmnd I can't se how we
>>> can get away with not having a stub scsi_device for the scsi host.
>>>
>>> And that won't even show up in sysfs if we assign it a LUN number beyond
>>> the addressable range; 'max_id':0 tends to be a safe choice here.
>>
>> There is no risk that the scsi_cmnd.device member will be dereferenced 
>> for
>> internal requests allocated by the UFS driver. But I understand from your
>> email that making sure that the scsi_cmnd.device member is not NULL is
>> important for other SCSI LLDs. I will look into the approach of 
>> associating
>> a stub SCSI device with internal requests.
> 
> (replying to my own email)
> 
> Hi Hannes,
> 
> Allocating a struct scsi_device for internal requests seems tricky to 
> me. The
> most straightforward approach would be to call scsi_alloc_sdev(). 
> However, that
> function accepts a scsi_target pointer and calls .slave_alloc(). So a
> scsi_target structure would have to be set up before that function is 
> called and
> SCSI LLDs would have to be audited to verify that .slave_alloc() works 
> fine for
> the H:C:I:L tuple assigned to the fake SCSI device. Additionally, how 
> should the
> inquiry data be initialized that is filled in by scsi_add_lun()?
> 
> Since I do not use SCSI hardware that needs a scsi_device to be 
> associated with
> internal requests, I prefer that this functionality is implemented in a 
> future
> patch series. Changing the hba->host->internal_queue occurrences in the UFS
> driver into something like hba->host->internal_sdev->request_queue once 
> this
> functionality is implemented should be easy.
> 
Well, I still do have the patchset for implementing it.
Will be reposting it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH v2 06/20] scsi: core: Add support for reserved tags

[Bart Van Assche]

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

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

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

Re: [PATCH v2 06/20] scsi: core: Add support for reserved tags

[John Garry]

On 19/11/2021 19:57, Bart Van Assche wrote:
> Allow SCSI LLDs to allocate reserved tags by passing the BLK_MQ_REQ_RESERVED
> flag to blk_mq_alloc_request().
> 
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org > ---
>   drivers/scsi/scsi_lib.c  | 4 +++-
>   include/scsi/scsi_host.h | 9 ++++++++-
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 59c3c4fbcfc0..44489ddc646c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1925,6 +1925,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>   {
>   	unsigned int cmd_size, sgl_size;
>   	struct blk_mq_tag_set *tag_set = &shost->tag_set;
> +	unsigned int reserved_tags = shost->hostt->reserved_tags;
>   
>   	sgl_size = max_t(unsigned int, sizeof(struct scatterlist),
>   				scsi_mq_inline_sgl_size(shost));
> @@ -1940,7 +1941,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>   		tag_set->ops = &scsi_mq_ops_no_commit;
>   	tag_set->nr_hw_queues = shost->nr_hw_queues ? : 1;
>   	tag_set->nr_maps = shost->nr_maps ? : 1;
> -	tag_set->queue_depth = shost->can_queue;
> +	tag_set->queue_depth = shost->can_queue + reserved_tags;
> +	tag_set->reserved_tags = reserved_tags;
>   	tag_set->cmd_size = cmd_size;
>   	tag_set->numa_node = NUMA_NO_NODE;
>   	tag_set->flags = BLK_MQ_F_SHOULD_MERGE;
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 72e1a347baa6..ec0f7705e06a 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -367,10 +367,17 @@ struct scsi_host_template {

why no field in struct Scsi_Host?

>   	/*
>   	 * This determines if we will use a non-interrupt driven
>   	 * or an interrupt driven scheme.  It is set to the maximum number
> -	 * of simultaneous commands a single hw queue in HBA will accept.
> +	 * of simultaneous commands a single hw queue in HBA will accept. Does
> +	 * not include @reserved_tags.
>   	 */
>   	int can_queue;
>   
> +	/*
> +	 * Number of tags to reserve.


> A reserved tag can be allocated by passing
> +	 * the BLK_MQ_REQ_RESERVED flag to blk_get_request().

I don't see why we need this comment.

> +	 */
> +	unsigned reserved_tags;

I thought that unsigned int was preferred

> +
>   	/*
>   	 * In many instances, especially where disconnect / reconnect are
>   	 * supported, our host also has an ID on the SCSI bus.  If this is
> .
> 

Re: [PATCH v2 06/20] scsi: core: Add support for reserved tags

[Bart Van Assche]

On 11/22/21 12:15 AM, John Garry wrote:
> On 19/11/2021 19:57, Bart Van Assche wrote:
>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>> index 72e1a347baa6..ec0f7705e06a 100644
>> --- a/include/scsi/scsi_host.h
>> +++ b/include/scsi/scsi_host.h
>> @@ -367,10 +367,17 @@ struct scsi_host_template {
> 
> why no field in struct Scsi_Host?

Why to duplicate this field in struct Scsi_Host? Do we expect that there
will be SCSI drivers in the future for which the number of reserved tags
is only known at runtime? This seems unlikely to me.

>> +    unsigned reserved_tags;
> 
> I thought that unsigned int was preferred

I will change 'unsigned' into 'unsigned int'.

Bart.

Re: [PATCH v2 06/20] scsi: core: Add support for reserved tags

[John Garry]

On 22/11/2021 17:25, Bart Van Assche wrote:
> On 11/22/21 12:15 AM, John Garry wrote:
>> On 19/11/2021 19:57, Bart Van Assche wrote:
>>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>>> index 72e1a347baa6..ec0f7705e06a 100644
>>> --- a/include/scsi/scsi_host.h
>>> +++ b/include/scsi/scsi_host.h
>>> @@ -367,10 +367,17 @@ struct scsi_host_template {
>>
>> why no field in struct Scsi_Host?
> 
> Why to duplicate this field in struct Scsi_Host? Do we expect that there
> will be SCSI drivers in the future for which the number of reserved tags
> is only known at runtime? This seems unlikely to me.


Fine, I was using this but I suppose it's not needed ATM.

Thanks,
John

[PATCH v2 07/20] 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>
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 54750d72c8fb..ebe9e197d804 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -338,7 +338,8 @@ struct ufs_hba_variant_ops {
 					enum ufs_notify_change_status status,
 					struct ufs_pa_layer_attr *,
 					struct ufs_pa_layer_attr *);
-	void	(*setup_xfer_req)(struct ufs_hba *, int, bool);
+	void	(*setup_xfer_req)(struct ufs_hba *hba, int tag,
+				  bool is_scsi_cmd);
 	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);
 	void    (*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
 					enum ufs_notify_change_status);

Re: [PATCH v2 07/20] scsi: ufs: Rename a function argument

[Bean Huo]

On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
> 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>
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

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

[PATCH v2 08/20] 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 afd38142b1c0..a3ca9ada0adc 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 v2 09/20] 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 a3ca9ada0adc..ff9532968ae7 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 ebe9e197d804..e9bc07c69a80 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 v2 10/20] 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, 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ff9532968ae7..fced4528ee90 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,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
-out:
-	blk_mq_free_request(req);
 out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;

Re: [PATCH v2 10/20] scsi: ufs: Remove dead code

[Adrian Hunter]

On 19/11/2021 21:57, Bart Van Assche wrote:
> Commit 7252a3603015 ("scsi: ufs: Avoid busy-waiting by eliminating tag
> conflicts") guarantees that 'tag' is not in use by any SCSI command.
> Remove the check that returns early if a conflict occurs.
> 
> 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, 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ff9532968ae7..fced4528ee90 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,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>  	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
>  				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
>  
> -out:
> -	blk_mq_free_request(req);

Removing blk_mq_free_request() looks unintended

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

Re: [PATCH v2 10/20] scsi: ufs: Remove dead code

[Bart Van Assche]

On 11/24/21 3:11 AM, Adrian Hunter wrote:
> On 19/11/2021 21:57, Bart Van Assche wrote:
>> -out:
>> -	blk_mq_free_request(req);
> 
> Removing blk_mq_free_request() looks unintended

Oops, that removal was indeed unintended. Will fix.

Thanks,

Bart.

[PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bart Van Assche]

The only functional change in this patch is the addition of a
blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fced4528ee90..dfa5f127342b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 {
 	struct request_queue *q = hba->cmd_queue;
 	DECLARE_COMPLETION_ONSTACK(wait);
+	struct scsi_cmnd *scmd;
 	struct request *req;
 	struct ufshcd_lrb *lrbp;
 	int err;
@@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	 * 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);
+	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
+	if (IS_ERR(scmd)) {
+		err = PTR_ERR(scmd);
 		goto out_unlock;
 	}
+	req = scsi_cmd_to_rq(scmd);
 	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);
+	/*
+	 * Start the request such that blk_mq_tag_idle() is called when the
+	 * device management request finishes.
+	 */
 	blk_mq_start_request(req);
 
 	lrbp = &hba->lrb[tag];
@@ -2972,7 +2976,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
 out:
-	blk_mq_free_request(req);
+	scsi_put_internal_cmd(scmd);
+
 out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;
@@ -6573,17 +6578,16 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	struct request_queue *q = hba->tmf_queue;
 	struct Scsi_Host *host = hba->host;
 	DECLARE_COMPLETION_ONSTACK(wait);
+	struct scsi_cmnd *scmd;
 	struct request *req;
 	unsigned long flags;
 	int task_tag, err;
 
-	/*
-	 * blk_mq_alloc_request() is used here only to get a free tag.
-	 */
-	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
+	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
+	if (IS_ERR(scmd))
+		return PTR_ERR(scmd);
 
+	req = scsi_cmd_to_rq(scmd);
 	req->end_io_data = &wait;
 	ufshcd_hold(hba, false);
 
@@ -6636,7 +6640,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	ufshcd_release(hba);
-	blk_mq_free_request(req);
+
+	scsi_put_internal_cmd(scmd);
 
 	return err;
 }
@@ -6714,6 +6719,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 {
 	struct request_queue *q = hba->cmd_queue;
 	DECLARE_COMPLETION_ONSTACK(wait);
+	struct scsi_cmnd *scmd;
 	struct request *req;
 	struct ufshcd_lrb *lrbp;
 	int err = 0;
@@ -6722,13 +6728,19 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 
 	down_read(&hba->clk_scaling_lock);
 
-	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
-	if (IS_ERR(req)) {
-		err = PTR_ERR(req);
+	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
+	if (IS_ERR(scmd)) {
+		err = PTR_ERR(scmd);
 		goto out_unlock;
 	}
+	req = scsi_cmd_to_rq(scmd);
 	tag = req->tag;
 	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
+	/*
+	 * Start the request such that blk_mq_tag_idle() is called when the
+	 * device management request finishes.
+	 */
+	blk_mq_start_request(req);
 
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
@@ -6797,6 +6809,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);
 
+	scsi_put_internal_cmd(scmd);
+
 out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bean Huo]

On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
> The only functional change in this patch is the addition of a
> blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 46 +++++++++++++++++++++++++----------
> ----
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fced4528ee90..dfa5f127342b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>  {
>  	struct request_queue *q = hba->cmd_queue;
>  	DECLARE_COMPLETION_ONSTACK(wait);
> +	struct scsi_cmnd *scmd;
>  	struct request *req;
>  	struct ufshcd_lrb *lrbp;
>  	int err;
> @@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>  	 * 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);
> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> +	if (IS_ERR(scmd)) {
> +		err = PTR_ERR(scmd);
>  		goto out_unlock;
>  	}
> +	req = scsi_cmd_to_rq(scmd);
>  	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);
> +	/*
> +	 * Start the request such that blk_mq_tag_idle() is called when
> the
> +	 * device management request finishes.
> +	 */
>  	blk_mq_start_request(req);
>  
>  	lrbp = &hba->lrb[tag];
> @@ -2972,7 +2976,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>  				    (struct utp_upiu_req *)lrbp-
> >ucd_rsp_ptr);
>  
>  out:
> -	blk_mq_free_request(req);
> +	scsi_put_internal_cmd(scmd);
> +
>  out_unlock:
>  	up_read(&hba->clk_scaling_lock);
>  	return err;
> @@ -6573,17 +6578,16 @@ static int __ufshcd_issue_tm_cmd(struct
> ufs_hba *hba,
>  	struct request_queue *q = hba->tmf_queue;
>  	struct Scsi_Host *host = hba->host;
>  	DECLARE_COMPLETION_ONSTACK(wait);
> +	struct scsi_cmnd *scmd;
>  	struct request *req;
>  	unsigned long flags;
>  	int task_tag, err;
>  
> -	/*
> -	 * blk_mq_alloc_request() is used here only to get a free tag.
> -	 */
> -	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
> -	if (IS_ERR(req))
> -		return PTR_ERR(req);
> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> +	if (IS_ERR(scmd))
> +		return PTR_ERR(scmd);
>  
> +	req = scsi_cmd_to_rq(scmd);
>  	req->end_io_data = &wait;
>  	ufshcd_hold(hba, false);
>  
> @@ -6636,7 +6640,8 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba
> *hba,
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
>  
>  	ufshcd_release(hba);
> -	blk_mq_free_request(req);
> +
> +	scsi_put_internal_cmd(scmd);
>  
>  	return err;
>  }
> @@ -6714,6 +6719,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>  {
>  	struct request_queue *q = hba->cmd_queue;
>  	DECLARE_COMPLETION_ONSTACK(wait);
> +	struct scsi_cmnd *scmd;
>  	struct request *req;
>  	struct ufshcd_lrb *lrbp;
>  	int err = 0;
> @@ -6722,13 +6728,19 @@ static int
> ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>  
>  	down_read(&hba->clk_scaling_lock);
>  
> -	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
> -	if (IS_ERR(req)) {
> -		err = PTR_ERR(req);
> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> +	if (IS_ERR(scmd)) {
> +		err = PTR_ERR(scmd);
>  		goto out_unlock;
>  	}
> +	req = scsi_cmd_to_rq(scmd);
>  	tag = req->tag;
>  	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> +	/*
> +	 * Start the request such that blk_mq_tag_idle() is called when
> the
> +	 * device management request finishes.
> +	 */
> +	blk_mq_start_request(req);

Bart,

Calling blk_mq_start_request() will inject the trace print of the block
issued, but we do not have its paired completion trace print.
In addition, blk_mq_tag_idle() will not be called after the device
management request is completed, it will be called after the timer
expires.

I remember that we used to not allow this kind of LLD internal commands
to be attached to the block layer. I now find that to be correct way. 

Bean

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bart Van Assche]

On 11/23/21 4:20 AM, Bean Huo wrote:
> On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
>> @@ -6722,13 +6728,19 @@ static int
>> ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>>   
>>   	down_read(&hba->clk_scaling_lock);
>>   
>> -	req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
>> -	if (IS_ERR(req)) {
>> -		err = PTR_ERR(req);
>> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
>> +	if (IS_ERR(scmd)) {
>> +		err = PTR_ERR(scmd);
>>   		goto out_unlock;
>>   	}
>> +	req = scsi_cmd_to_rq(scmd);
>>   	tag = req->tag;
>>   	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
>> +	/*
>> +	 * Start the request such that blk_mq_tag_idle() is called when
>> the
>> +	 * device management request finishes.
>> +	 */
>> +	blk_mq_start_request(req);
> 
> Bart,
> 
> Calling blk_mq_start_request() will inject the trace print of the block
> issued, but we do not have its paired completion trace print.
> In addition, blk_mq_tag_idle() will not be called after the device
> management request is completed, it will be called after the timer
> expires.
> 
> I remember that we used to not allow this kind of LLD internal commands
> to be attached to the block layer. I now find that to be correct way.

Hi Bean,

As you may remember commit d0b2b70eb12e ("scsi: ufs: core: Increase the
usable queue depth") introduced a blk_mq_start_request() call in
ufshcd_exec_dev_cmd() to restore the queue depth from 16 to 32. I think
we need the same fix in ufshcd_issue_devman_upiu_cmd(). How about modifying
patch 1/20 of this series such that tracing is skipped for internal
requests? Would that address your concern?

Thanks,

Bart.

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bart Van Assche]

On 11/23/21 4:20 AM, Bean Huo wrote:
> Calling blk_mq_start_request() will inject the trace print of the block
> issued, but we do not have its paired completion trace print.
> In addition, blk_mq_tag_idle() will not be called after the device
> management request is completed, it will be called after the timer
> expires.
> 
> I remember that we used to not allow this kind of LLD internal commands
> to be attached to the block layer. I now find that to be correct way.

Hi Bean,

How about modifying the block layer such that blk_mq_tag_busy() is not
called for requests with operation type REQ_OP_DRV_*? I think that will
allow to leave out the blk_mq_start_request() calls from the UFS driver.
These are the changes I currently have in mind (on top of this patch
series):

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3ab34c4f20da..a7090b509f2d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -433,6 +433,7 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,

  static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
  {
+	const bool is_passthrough = blk_op_is_passthrough(data->cmd_flags);
  	struct request_queue *q = data->q;
  	u64 alloc_time_ns = 0;
  	struct request *rq;
@@ -455,8 +456,7 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
  		 * dispatch list. Don't include reserved tags in the
  		 * limiting, as it isn't useful.
  		 */
-		if (!op_is_flush(data->cmd_flags) &&
-		    !blk_op_is_passthrough(data->cmd_flags) &&
+		if (!op_is_flush(data->cmd_flags) && !is_passthrough &&
  		    e->type->ops.limit_depth &&
  		    !(data->flags & BLK_MQ_REQ_RESERVED))
  			e->type->ops.limit_depth(data->cmd_flags, data);
@@ -465,7 +465,7 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
  retry:
  	data->ctx = blk_mq_get_ctx(q);
  	data->hctx = blk_mq_map_queue(q, data->cmd_flags, data->ctx);
-	if (!(data->rq_flags & RQF_ELV))
+	if (!(data->rq_flags & RQF_ELV) && !is_passthrough)
  		blk_mq_tag_busy(data->hctx);

  	/*
@@ -575,10 +575,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
  	cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask);
  	data.ctx = __blk_mq_get_ctx(q, cpu);

-	if (!q->elevator)
-		blk_mq_tag_busy(data.hctx);
-	else
+	if (q->elevator)
  		data.rq_flags |= RQF_ELV;
+	else if (!blk_op_is_passthrough(data.cmd_flags))
+		blk_mq_tag_busy(data.hctx);

  	ret = -EWOULDBLOCK;
  	tag = blk_mq_get_tag(&data);
@@ -1369,7 +1369,8 @@ static bool __blk_mq_alloc_driver_tag(struct request *rq)
  	unsigned int tag_offset = rq->mq_hctx->tags->nr_reserved_tags;
  	int tag;

-	blk_mq_tag_busy(rq->mq_hctx);
+	if (!blk_rq_is_passthrough(rq))
+		blk_mq_tag_busy(rq->mq_hctx);

  	if (blk_mq_tag_is_reserved(rq->mq_hctx->sched_tags, rq->internal_tag)) {
  		bt = &rq->mq_hctx->tags->breserved_tags;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fcecbc4ee81b..2c9e9c79ca34 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1360,25 +1360,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->host->internal_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)
  {
@@ -1778,7 +1759,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;

@@ -2996,12 +2977,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
  	req = scsi_cmd_to_rq(scmd);
  	tag = req->tag;
  	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
-	/*
-	 * Start the request such that blk_mq_tag_idle() is called when the
-	 * device management request finishes.
-	 */
-	blk_mq_start_request(req);
-
  	lrbp = &hba->lrb[tag];
  	WARN_ON(lrbp->cmd);
  	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
@@ -6792,12 +6767,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
  	req = scsi_cmd_to_rq(scmd);
  	tag = req->tag;
  	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
-	/*
-	 * Start the request such that blk_mq_tag_idle() is called when the
-	 * device management request finishes.
-	 */
-	blk_mq_start_request(req);
-
  	lrbp = &hba->lrb[tag];
  	WARN_ON(lrbp->cmd);
  	lrbp->cmd = NULL;

Thanks,

Bart.

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bean Huo]

On Tue, 2021-11-23 at 11:41 -0800, Bart Van Assche wrote:
> On 11/23/21 4:20 AM, Bean Huo wrote:
> > Calling blk_mq_start_request() will inject the trace print of the
> > block
> > issued, but we do not have its paired completion trace print.
> > In addition, blk_mq_tag_idle() will not be called after the device
> > management request is completed, it will be called after the timer
> > expires.
> > 
> > I remember that we used to not allow this kind of LLD internal
> > commands
> > to be attached to the block layer. I now find that to be correct
> > way.
> 
> Hi Bean,
> 
> How about modifying the block layer such that blk_mq_tag_busy() is
> not
> called for requests with operation type REQ_OP_DRV_*? I think that
> will
> allow to leave out the blk_mq_start_request() calls from the UFS
> driver.
> These are the changes I currently have in mind (on top of this patch
> series):
> 

Hi Bart,

Yes, the following patch can solve these two problems, but you need to
change block layer code. Why do we have to fly to the block layer to
get this tag? and what is the benefit? This is a device management
request. As for the patch recommended by Adrian, that is the way I
think.


Kind regards,
Bean

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3ab34c4f20da..a7090b509f2d 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -433,6 +433,7 @@ __blk_mq_alloc_requests_batch(struct
> blk_mq_alloc_data *data,
> 
>   static struct request *__blk_mq_alloc_requests(struct
> blk_mq_alloc_data *data)
>   {
> +	const bool is_passthrough = blk_op_is_passthrough(data-
> >cmd_flags);
>   	struct request_queue *q = data->q;
>   	u64 alloc_time_ns = 0;
>   	struct request *rq;
> @@ -455,8 +456,7 @@ static struct request
> *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
>   		 * dispatch list. Don't include reserved tags in the
>   		 * limiting, as it isn't useful.
>   		 */
> -		if (!op_is_flush(data->cmd_flags) &&
> -		    !blk_op_is_passthrough(data->cmd_flags) &&
> +		if (!op_is_flush(data->cmd_flags) && !is_passthrough &&
>   		    e->type->ops.limit_depth &&
>   		    !(data->flags & BLK_MQ_REQ_RESERVED))
>   			e->type->ops.limit_depth(data->cmd_flags,
> data);
> @@ -465,7 +465,7 @@ static struct request
> *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
>   retry:
>   	data->ctx = blk_mq_get_ctx(q);
>   	data->hctx = blk_mq_map_queue(q, data->cmd_flags, data->ctx);
> -	if (!(data->rq_flags & RQF_ELV))
> +	if (!(data->rq_flags & RQF_ELV) && !is_passthrough)
>   		blk_mq_tag_busy(data->hctx);
> 
>   	/*
> @@ -575,10 +575,10 @@ struct request
> *blk_mq_alloc_request_hctx(struct request_queue *q,
>   	cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask);
>   	data.ctx = __blk_mq_get_ctx(q, cpu);
> 
> -	if (!q->elevator)
> -		blk_mq_tag_busy(data.hctx);
> -	else
> +	if (q->elevator)
>   		data.rq_flags |= RQF_ELV;
> +	else if (!blk_op_is_passthrough(data.cmd_flags))
> +		blk_mq_tag_busy(data.hctx);
> 
>   	ret = -EWOULDBLOCK;
>   	tag = blk_mq_get_tag(&data);
> @@ -1369,7 +1369,8 @@ static bool __blk_mq_alloc_driver_tag(struct
> request *rq)
>   	unsigned int tag_offset = rq->mq_hctx->tags->nr_reserved_tags;
>   	int tag;
> 
> -	blk_mq_tag_busy(rq->mq_hctx);
> +	if (!blk_rq_is_passthrough(rq))
> +		blk_mq_tag_busy(rq->mq_hctx);
> 
>   	if (blk_mq_tag_is_reserved(rq->mq_hctx->sched_tags, rq-
> >internal_tag)) {
>   		bt = &rq->mq_hctx->tags->breserved_tags;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fcecbc4ee81b..2c9e9c79ca34 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1360,25 +1360,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->host->internal_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)
>   {
> @@ -1778,7 +1759,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;
> 
> @@ -2996,12 +2977,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>   	req = scsi_cmd_to_rq(scmd);
>   	tag = req->tag;
>   	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n",
> tag);
> -	/*
> -	 * Start the request such that blk_mq_tag_idle() is called when
> the
> -	 * device management request finishes.
> -	 */
> -	blk_mq_start_request(req);
> -
>   	lrbp = &hba->lrb[tag];
>   	WARN_ON(lrbp->cmd);
>   	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
> @@ -6792,12 +6767,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>   	req = scsi_cmd_to_rq(scmd);
>   	tag = req->tag;
>   	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n",
> tag);
> -	/*
> -	 * Start the request such that blk_mq_tag_idle() is called when
> the
> -	 * device management request finishes.
> -	 */
> -	blk_mq_start_request(req);
> -
>   	lrbp = &hba->lrb[tag];
>   	WARN_ON(lrbp->cmd);
>   	lrbp->cmd = NULL;
> 
> Thanks,
> 
> Bart.

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Adrian Hunter]

On 19/11/2021 21:57, Bart Van Assche wrote:
> The only functional change in this patch is the addition of a
> blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 46 +++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fced4528ee90..dfa5f127342b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>  {
>  	struct request_queue *q = hba->cmd_queue;
>  	DECLARE_COMPLETION_ONSTACK(wait);
> +	struct scsi_cmnd *scmd;
>  	struct request *req;
>  	struct ufshcd_lrb *lrbp;
>  	int err;
> @@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>  	 * 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);
> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);

We do not need the block layer, nor SCSI commands which begs the question,
why involve them at all?

For example, the following is much simpler and seems to work:


 drivers/scsi/ufs/ufshcd.c | 52 +++++++--------------------------------
 drivers/scsi/ufs/ufshcd.h |  1 +
 2 files changed, 10 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d125d8792ace5..bdfac3e9991ee 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[] = {
@@ -2189,6 +2190,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);
@@ -2931,31 +2933,13 @@ 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;
 	struct ufshcd_lrb *lrbp;
 	int err;
-	int tag;
+	int tag = hba->reserved_slot;
 
 	down_read(&hba->clk_scaling_lock);
 
-	/*
-	 * Get free slot, sleep if slots are unavailable.
-	 * Even though we use wait_event() which sleeps indefinitely,
-	 * the maximum wait time is bounded by SCSI request timeout.
-	 */
-	req = blk_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);
-
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
@@ -2970,10 +2954,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
 	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;
 }
@@ -4955,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)
@@ -6706,24 +6683,14 @@ 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;
 	struct ufshcd_lrb *lrbp;
 	int err = 0;
-	int tag;
+	int tag = hba->reserved_slot;
 	u8 upiu_flags;
 
 	down_read(&hba->clk_scaling_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);
-
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
 	lrbp->cmd = NULL;
@@ -6791,7 +6758,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
-out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;
 }
@@ -9516,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 e9bc07c69a801..1addb2c906bae 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -836,6 +836,7 @@ struct ufs_hba {
 	u32 capabilities;
 	int nutrs;
 	int nutmrs;
+	int reserved_slot; /* Protected by dev_cmd.lock */
 	u32 ufs_version;
 	const struct ufs_hba_variant_ops *vops;
 	struct ufs_hba_variant_params *vps;

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Adrian Hunter]

On 24/11/2021 13:02, Adrian Hunter wrote:
> On 19/11/2021 21:57, Bart Van Assche wrote:
>> The only functional change in this patch is the addition of a
>> blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 46 +++++++++++++++++++++++++--------------
>>  1 file changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index fced4528ee90..dfa5f127342b 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>  {
>>  	struct request_queue *q = hba->cmd_queue;
>>  	DECLARE_COMPLETION_ONSTACK(wait);
>> +	struct scsi_cmnd *scmd;
>>  	struct request *req;
>>  	struct ufshcd_lrb *lrbp;
>>  	int err;
>> @@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>  	 * 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);
>> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> 
> We do not need the block layer, nor SCSI commands which begs the question,
> why involve them at all?
> 
> For example, the following is much simpler and seems to work:

Applied on top of patches 7 - 10, where patch 10 has the unintended
removal of blk_mq_free_request() that would have been removed here
instead.

> 
> 
>  drivers/scsi/ufs/ufshcd.c | 52 +++++++--------------------------------
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  2 files changed, 10 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d125d8792ace5..bdfac3e9991ee 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[] = {
> @@ -2189,6 +2190,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);
> @@ -2931,31 +2933,13 @@ 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;
>  	struct ufshcd_lrb *lrbp;
>  	int err;
> -	int tag;
> +	int tag = hba->reserved_slot;
>  
>  	down_read(&hba->clk_scaling_lock);
>  
> -	/*
> -	 * Get free slot, sleep if slots are unavailable.
> -	 * Even though we use wait_event() which sleeps indefinitely,
> -	 * the maximum wait time is bounded by SCSI request timeout.
> -	 */
> -	req = blk_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);
> -
>  	lrbp = &hba->lrb[tag];
>  	WARN_ON(lrbp->cmd);
>  	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
> @@ -2970,10 +2954,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>  	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
>  	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;
>  }
> @@ -4955,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)
> @@ -6706,24 +6683,14 @@ 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;
>  	struct ufshcd_lrb *lrbp;
>  	int err = 0;
> -	int tag;
> +	int tag = hba->reserved_slot;
>  	u8 upiu_flags;
>  
>  	down_read(&hba->clk_scaling_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);
> -
>  	lrbp = &hba->lrb[tag];
>  	WARN_ON(lrbp->cmd);
>  	lrbp->cmd = NULL;
> @@ -6791,7 +6758,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>  	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
>  				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
>  
> -out_unlock:
>  	up_read(&hba->clk_scaling_lock);
>  	return err;
>  }
> @@ -9516,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 e9bc07c69a801..1addb2c906bae 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -836,6 +836,7 @@ struct ufs_hba {
>  	u32 capabilities;
>  	int nutrs;
>  	int nutmrs;
> +	int reserved_slot; /* Protected by dev_cmd.lock */
>  	u32 ufs_version;
>  	const struct ufs_hba_variant_ops *vops;
>  	struct ufs_hba_variant_params *vps;
> 

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bart Van Assche]

On 11/24/21 3:02 AM, Adrian Hunter wrote:
> On 19/11/2021 21:57, Bart Van Assche wrote:
>> The only functional change in this patch is the addition of a
>> blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/ufs/ufshcd.c | 46 +++++++++++++++++++++++++--------------
>>   1 file changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index fced4528ee90..dfa5f127342b 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>   {
>>   	struct request_queue *q = hba->cmd_queue;
>>   	DECLARE_COMPLETION_ONSTACK(wait);
>> +	struct scsi_cmnd *scmd;
>>   	struct request *req;
>>   	struct ufshcd_lrb *lrbp;
>>   	int err;
>> @@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>   	 * 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);
>> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> 
> We do not need the block layer, nor SCSI commands which begs the question,
> why involve them at all?
> 
> For example, the following is much simpler and seems to work:
> [ ... ]

That patch bypasses the block layer for device management commands. So that
patch breaks a very basic assumption on which the block layer has been built,
namely that the block layer core knows whether or not any requests are ongoing.
That patch breaks at least the following functionality:
* Run-time power management. blk_pre_runtime_suspend() checks whether
   q_usage_counter is zero before initiating runtime power management.
   q_usage_counter is increased by blk_mq_alloc_request() and decreased by
   blk_mq_free_request(). I don't think it is safe to change the power state
   while a device management request is in progress.
* The code in blk_cleanup_queue() that waits for pending requests to finish
   before resources associated with the request queue are freed.
   ufshcd_remove() calls blk_cleanup_queue(hba->cmd_queue) and hence waits until
   pending device management commands have finished. That would no longer be the
   case if the block layer is bypassed to submit device management commands.

Bart.

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Adrian Hunter]

On 29/11/2021 21:32, Bart Van Assche wrote:
> On 11/24/21 3:02 AM, Adrian Hunter wrote:
>> On 19/11/2021 21:57, Bart Van Assche wrote:
>>> The only functional change in this patch is the addition of a
>>> blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().
>>>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   drivers/scsi/ufs/ufshcd.c | 46 +++++++++++++++++++++++++--------------
>>>   1 file changed, 30 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index fced4528ee90..dfa5f127342b 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>>   {
>>>       struct request_queue *q = hba->cmd_queue;
>>>       DECLARE_COMPLETION_ONSTACK(wait);
>>> +    struct scsi_cmnd *scmd;
>>>       struct request *req;
>>>       struct ufshcd_lrb *lrbp;
>>>       int err;
>>> @@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>>        * 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);
>>> +    scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
>>
>> We do not need the block layer, nor SCSI commands which begs the question,
>> why involve them at all?
>>
>> For example, the following is much simpler and seems to work:
>> [ ... ]
> 
> That patch bypasses the block layer for device management commands. So that
> patch breaks a very basic assumption on which the block layer has been built,
> namely that the block layer core knows whether or not any requests are ongoing.
> That patch breaks at least the following functionality:
> * Run-time power management. blk_pre_runtime_suspend() checks whether
>   q_usage_counter is zero before initiating runtime power management.
>   q_usage_counter is increased by blk_mq_alloc_request() and decreased by
>   blk_mq_free_request(). I don't think it is safe to change the power state
>   while a device management request is in progress.

hba->cmd_queue does not have runtime PM.

I suspect making it do runtime PM might open up deadlock issues similar to
the issues that were seen with sending sending clear WLUN UA from the
UFS error handler.

> * The code in blk_cleanup_queue() that waits for pending requests to finish
>   before resources associated with the request queue are freed.
>   ufshcd_remove() calls blk_cleanup_queue(hba->cmd_queue) and hence waits until
>   pending device management commands have finished. That would no longer be the
>   case if the block layer is bypassed to submit device management commands.

cmd_queue is used only by the UFS driver, so if the driver is racing with
itself at "remove", then that should be fixed. The risk is not that the UFS
driver might use requests, but that it might still be operating when hba or
other resources get freed.

So the question remains, for device commands, we do not need the block
layer, nor SCSI commands which still begs the question, why involve them
at all?

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bart Van Assche]

On 11/29/21 10:41 PM, Adrian Hunter wrote:
> On 29/11/2021 21:32, Bart Van Assche wrote:
>> * The code in blk_cleanup_queue() that waits for pending requests to finish
>>    before resources associated with the request queue are freed.
>>    ufshcd_remove() calls blk_cleanup_queue(hba->cmd_queue) and hence waits until
>>    pending device management commands have finished. That would no longer be the
>>    case if the block layer is bypassed to submit device management commands.
> 
> cmd_queue is used only by the UFS driver, so if the driver is racing with
> itself at "remove", then that should be fixed. The risk is not that the UFS
> driver might use requests, but that it might still be operating when hba or
> other resources get freed.
> 
> So the question remains, for device commands, we do not need the block
> layer, nor SCSI commands which still begs the question, why involve them
> at all?

By using the block layer request allocation functions the block layer guarantees
that each tag is in use in only one context. When bypassing the block layer code
would have to be inserted in ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd()
to serialize these functions. In other words, we would be duplicating existing
functionality if we bypass the block layer. The recommended approach in the Linux
kernel is not to duplicate existing functionality.

Thanks,

Bart.

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Adrian Hunter]

On 30/11/2021 19:51, Bart Van Assche wrote:
> On 11/29/21 10:41 PM, Adrian Hunter wrote:
>> On 29/11/2021 21:32, Bart Van Assche wrote:
>>> * The code in blk_cleanup_queue() that waits for pending requests to finish
>>>    before resources associated with the request queue are freed.
>>>    ufshcd_remove() calls blk_cleanup_queue(hba->cmd_queue) and hence waits until
>>>    pending device management commands have finished. That would no longer be the
>>>    case if the block layer is bypassed to submit device management commands.
>>
>> cmd_queue is used only by the UFS driver, so if the driver is racing with
>> itself at "remove", then that should be fixed. The risk is not that the UFS
>> driver might use requests, but that it might still be operating when hba or
>> other resources get freed.
>>
>> So the question remains, for device commands, we do not need the block
>> layer, nor SCSI commands which still begs the question, why involve them
>> at all?
> 
> By using the block layer request allocation functions the block layer guarantees
> that each tag is in use in only one context. When bypassing the block layer code
> would have to be inserted in ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd()
> to serialize these functions.

They already are serialized, but you are essentially saying the functionality
being duplicated is just a lock.  What you are proposing seems awfully
complicated just to get the functionality of a lock.

> In other words, we would be duplicating existing
> functionality if we bypass the block layer. The recommended approach in the Linux
> kernel is not to duplicate existing functionality.

More accurately, the functionality would not be being used at all, so not
really any duplication.

Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Bart Van Assche]

On 11/30/21 11:15 AM, Adrian Hunter wrote:
> On 30/11/2021 19:51, Bart Van Assche wrote:
>> By using the block layer request allocation functions the block layer guarantees
>> that each tag is in use in only one context. When bypassing the block layer code
>> would have to be inserted in ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd()
>> to serialize these functions.
> 
> They already are serialized, but you are essentially saying the functionality
> being duplicated is just a lock.  What you are proposing seems awfully
> complicated just to get the functionality of a lock.

Are you perhaps referring to hba->dev_cmd.lock? I had overlooked that lock
when I wrote my previous email. I will take a closer look at the reserved
slot approach.

Thanks,

Bart.

[PATCH v2 12/20] 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 dfa5f127342b..a241ef6bbc6f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4960,11 +4960,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 v2 13/20] 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 | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a241ef6bbc6f..03f4772fc2e2 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[] = {
@@ -2941,12 +2942,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 
 	down_read(&hba->clk_scaling_lock);
 
-	/*
-	 * Get free slot, sleep if slots are unavailable.
-	 * Even though we use wait_event() which sleeps indefinitely,
-	 * the maximum wait time is bounded by SCSI request timeout.
-	 */
-	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
+	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, BLK_MQ_REQ_RESERVED);
 	if (IS_ERR(scmd)) {
 		err = PTR_ERR(scmd);
 		goto out_unlock;
@@ -8171,6 +8167,7 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.sg_tablesize		= SG_ALL,
 	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
 	.can_queue		= UFSHCD_CAN_QUEUE,
+	.reserved_tags		= UFSHCD_NUM_RESERVED,
 	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
 	.max_host_blocked	= 1,
 	.track_queue_depth	= 1,
@@ -9531,8 +9528,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;

Re: [PATCH v2 13/20] scsi: ufs: Fix a deadlock in the error handler

[Bean Huo]

Bart,

The concern of this patch is that it reduces the UFS data transmission
queue depth. The cost is a bit high. We are looking for alternative
methods: for example, to fix this problem from the SCSI layer;
Add a new dedicated hardware device management queue on the UFS device
side.

Kind regards,
Bean

On Fri, 2021-11-19 at 11:57 -0800, 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>
> ---
>  drivers/scsi/ufs/ufshcd.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a241ef6bbc6f..03f4772fc2e2 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[] = {
> @@ -2941,12 +2942,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>  
>  	down_read(&hba->clk_scaling_lock);
>  
> -	/*
> -	 * Get free slot, sleep if slots are unavailable.
> -	 * Even though we use wait_event() which sleeps indefinitely,
> -	 * the maximum wait time is bounded by SCSI request timeout.
> -	 */
> -	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
> +	scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE,
> BLK_MQ_REQ_RESERVED);
>  	if (IS_ERR(scmd)) {
>  		err = PTR_ERR(scmd);
>  		goto out_unlock;
> @@ -8171,6 +8167,7 @@ static struct scsi_host_template
> ufshcd_driver_template = {
>  	.sg_tablesize		= SG_ALL,
>  	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
>  	.can_queue		= UFSHCD_CAN_QUEUE,
> +	.reserved_tags		= UFSHCD_NUM_RESERVED,
>  	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
>  	.max_host_blocked	= 1,
>  	.track_queue_depth	= 1,
> @@ -9531,8 +9528,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;

Re: [PATCH v2 13/20] scsi: ufs: Fix a deadlock in the error handler

[Bart Van Assche]

On 11/30/21 12:54 AM, Bean Huo wrote:
> The concern of this patch is that it reduces the UFS data transmission
> queue depth. The cost is a bit high. We are looking for alternative
> methods: for example, to fix this problem from the SCSI layer;
> Add a new dedicated hardware device management queue on the UFS device
> side.

Are any performance numbers available? My performance measurements did not
report a significant difference between the 31 and 32 queue depths.

Thanks,

Bart.

Re: [PATCH v2 13/20] scsi: ufs: Fix a deadlock in the error handler

[Bart Van Assche]

On 11/30/21 12:54 AM, Bean Huo wrote:
> We are looking for alternative
> methods: for example, to fix this problem from the SCSI layer;
> Add a new dedicated hardware device management queue on the UFS device
> side.

Hi Bean,

I don't think that there are any alternatives. If all host controller tags
are in use, it is not possible to implement a mechanism in software to
create an additional tag. Additionally, stealing a tag is not possible since
no new request can be submitted to a UFS controller if all tags are in use.

Thanks,

Bart.

Re: [PATCH v2 13/20] scsi: ufs: Fix a deadlock in the error handler

[Bean Huo]

On Tue, 2021-11-30 at 11:32 -0800, Bart Van Assche wrote:
> On 11/30/21 12:54 AM, Bean Huo wrote:
> > We are looking for alternative
> > methods: for example, to fix this problem from the SCSI layer;
> > Add a new dedicated hardware device management queue on the UFS
> > device
> > side.
> 
> Hi Bean,
> 
> I don't think that there are any alternatives. If all host controller
> tags
> are in use, it is not possible to implement a mechanism in software
> to
> create an additional tag. Additionally, stealing a tag is not
> possible since
> no new request can be submitted to a UFS controller if all tags are
> in use.
> 
> Thanks,
> 
> Bart.

Bart, 

How about adding a hardware-specific UFS device management queue to the
UFS device? Compared with NVMe and eMMC CMDQ, they both have dedicated
tags for device management commands. NVMe is queue 0, eMMC CMDQ is CMDQ
slot 31.

I'm thinking about the benefits of using this device manage queue. If
the benefits are significant, we can submit the Jedec proposal for the
UFS equipment management queue.

Kind regards,
Bean

Re: [PATCH v2 13/20] scsi: ufs: Fix a deadlock in the error handler

[Bart Van Assche]

On 12/1/21 5:44 AM, Bean Huo wrote:
> How about adding a hardware-specific UFS device management queue to the
> UFS device? Compared with NVMe and eMMC CMDQ, they both have dedicated
> tags for device management commands. NVMe is queue 0, eMMC CMDQ is CMDQ
> slot 31.
> 
> I'm thinking about the benefits of using this device manage queue. If
> the benefits are significant, we can submit the Jedec proposal for the
> UFS equipment management queue.

Hi Bean,

Do you want to modify UFSHCI 4.0 or UFSHCI 3.0? As you may know UFSHCI 4.0
will support multiple queues. The proposal is called multi-circular queue
(MCQ). Since UFSHCI 4.0 has not yet been ratified the draft is only available
to JEDEC members.

For UFSHCI 3.0, how about increasing the number of tags instead of adding an
additional queue? Increasing the number of tags probably will be easier to
implement in existing UFS host controllers than adding a new queue.

Thanks,

Bart.

[PATCH v2 14/20] 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().

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 | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 03f4772fc2e2..39dcf997a638 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5248,6 +5248,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
@@ -5258,9 +5270,7 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 {
 	struct ufshcd_lrb *lrbp;
 	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];
@@ -5270,26 +5280,19 @@ 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);
-			update_scaling = true;
 		} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
 			lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
 			if (hba->dev_cmd.complete) {
 				ufshcd_add_command_trace(hba, index,
 							 UFS_DEV_COMP);
 				complete(hba->dev_cmd.complete);
-				update_scaling = true;
+				ufshcd_clk_scaling_update_busy(hba);
 			}
 		}
-		if (update_scaling)
-			ufshcd_clk_scaling_update_busy(hba);
 	}
 }
 

Re: [PATCH v2 14/20] scsi: ufs: Introduce ufshcd_release_scsi_cmd()

[Adrian Hunter]

On 19/11/2021 21:57, 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().
> 
> 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 | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 03f4772fc2e2..39dcf997a638 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5248,6 +5248,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);
> +}

That seems to leave a gap in the handling of tracing.

Wouldn't we be better served to tweak the monitoring code
in __ufshcd_transfer_req_compl() and then use
 __ufshcd_transfer_req_compl()? i.e.

	result = ufshcd_transfer_rsp_status(hba, lrbp);
	if (unlikely(!result && ufshcd_should_inform_monitor(hba, lrbp)))
		ufshcd_update_monitor(hba, lrbp);


> +
>  /**
>   * __ufshcd_transfer_req_compl - handle SCSI and query command completion
>   * @hba: per adapter instance
> @@ -5258,9 +5270,7 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>  {
>  	struct ufshcd_lrb *lrbp;
>  	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];
> @@ -5270,26 +5280,19 @@ 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);
> -			update_scaling = true;
>  		} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
>  			lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
>  			if (hba->dev_cmd.complete) {
>  				ufshcd_add_command_trace(hba, index,
>  							 UFS_DEV_COMP);
>  				complete(hba->dev_cmd.complete);
> -				update_scaling = true;
> +				ufshcd_clk_scaling_update_busy(hba);
>  			}
>  		}
> -		if (update_scaling)
> -			ufshcd_clk_scaling_update_busy(hba);
>  	}
>  }
>  
> 

Re: [PATCH v2 14/20] scsi: ufs: Introduce ufshcd_release_scsi_cmd()

[Bart Van Assche]

On 11/24/21 4:03 AM, Adrian Hunter wrote:
> On 19/11/2021 21:57, Bart Van Assche wrote:
>> +/* 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);
>> +}
> 
> That seems to leave a gap in the handling of tracing.
> 
> Wouldn't we be better served to tweak the monitoring code
> in __ufshcd_transfer_req_compl() and then use
>   __ufshcd_transfer_req_compl()? i.e.
> 
> 	result = ufshcd_transfer_rsp_status(hba, lrbp);
> 	if (unlikely(!result && ufshcd_should_inform_monitor(hba, lrbp)))
> 		ufshcd_update_monitor(hba, lrbp);

Which gap are you referring to? ufshcd_abort() only calls
ufshcd_release_scsi_cmd() after ufshcd_try_to_abort_task() succeeded.
That means that the command has not completed and hence that
ufshcd_update_monitor() must not be called.

Bart.

Re: [PATCH v2 14/20] scsi: ufs: Introduce ufshcd_release_scsi_cmd()

[Adrian Hunter]

On 30/11/2021 20:00, Bart Van Assche wrote:
> On 11/24/21 4:03 AM, Adrian Hunter wrote:
>> On 19/11/2021 21:57, Bart Van Assche wrote:
>>> +/* 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);
>>> +}
>>
>> That seems to leave a gap in the handling of tracing.
>>
>> Wouldn't we be better served to tweak the monitoring code
>> in __ufshcd_transfer_req_compl() and then use
>>   __ufshcd_transfer_req_compl()? i.e.
>>
>>     result = ufshcd_transfer_rsp_status(hba, lrbp);
>>     if (unlikely(!result && ufshcd_should_inform_monitor(hba, lrbp)))
>>         ufshcd_update_monitor(hba, lrbp);
> 
> Which gap are you referring to? 

As in: you are not handling tracing.

ufshcd_abort() only calls
> ufshcd_release_scsi_cmd() after ufshcd_try_to_abort_task() succeeded.
> That means that the command has not completed and hence that
> ufshcd_update_monitor() must not be called.

AFAICT the monitor is for successful commands, which is why I suggested
checking the 'result'.

So make that change to __ufshcd_transfer_req_compl() and then it will
work for ufshcd_abort() and provide tracing.

Re: [PATCH v2 14/20] scsi: ufs: Introduce ufshcd_release_scsi_cmd()

[Bart Van Assche]

On 11/30/21 11:02 AM, Adrian Hunter wrote:
> On 30/11/2021 20:00, Bart Van Assche wrote:
>> ufshcd_abort() only calls
>> ufshcd_release_scsi_cmd() after ufshcd_try_to_abort_task() succeeded.
>> That means that the command has not completed and hence that
>> ufshcd_update_monitor() must not be called.
> 
> AFAICT the monitor is for successful commands, which is why I suggested
> checking the 'result'.
> 
> So make that change to __ufshcd_transfer_req_compl() and then it will
> work for ufshcd_abort() and provide tracing.

ufshcd_abort() does not set cmd->result because it doesn't have to.
Additionally, __ufshcd_transfer_req_compl() calls scsi_done() while an
abort handler should not call scsi_done(). In other words, my point of
view is that ufshcd_abort() should not call __ufshcd_transfer_req_compl().

Thanks,

Bart.

[PATCH v2 15/20] scsi: ufs: Improve SCSI abort handling

[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.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 39dcf997a638..7e27d6436889 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7042,8 +7042,12 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 
 	ufshcd_hold(hba, false);
 	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))) {
+	/*
+	 * If the command is already aborted/completed, return FAILED. This
+	 * should never happen since the SCSI core serializes error handling
+	 * and scsi_done() calls.
+	 */
+	if (WARN_ON_ONCE(!(test_bit(tag, &hba->outstanding_reqs)))) {
 		dev_err(hba->dev,
 			"%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
 			__func__, tag, hba->outstanding_reqs, reg);
@@ -7113,6 +7117,16 @@ 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);
+	__clear_bit(tag, &hba->outstanding_reqs);
+	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
+
+	ufshcd_release_scsi_cmd(hba, lrbp);
+
 	err = SUCCESS;
 
 release:

Re: [PATCH v2 15/20] scsi: ufs: Improve SCSI abort handling

[Adrian Hunter]

On 19/11/2021 21:57, 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.
> 
> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 39dcf997a638..7e27d6436889 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7042,8 +7042,12 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  
>  	ufshcd_hold(hba, false);
>  	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))) {
> +	/*
> +	 * If the command is already aborted/completed, return FAILED. This
> +	 * should never happen since the SCSI core serializes error handling
> +	 * and scsi_done() calls.

I don't think that is correct. ufshcd_transfer_req_compl() gets called directly
by the interrupt handler.

> +	 */
> +	if (WARN_ON_ONCE(!(test_bit(tag, &hba->outstanding_reqs)))) {
>  		dev_err(hba->dev,
>  			"%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
>  			__func__, tag, hba->outstanding_reqs, reg);
> @@ -7113,6 +7117,16 @@ 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);
> +	__clear_bit(tag, &hba->outstanding_reqs);
> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> +
> +	ufshcd_release_scsi_cmd(hba, lrbp);

ufshcd_release_scsi_cmd() must not be called if the bit was already clear
i.e.

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

	if (rel)
		ufshcd_release_scsi_cmd(hba, lrbp);

> +
>  	err = SUCCESS;
>  
>  release:
> 

Re: [PATCH v2 15/20] scsi: ufs: Improve SCSI abort handling

[Bart Van Assche]

On 11/24/21 04:28, Adrian Hunter wrote:
> On 19/11/2021 21:57, 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.
>>
>> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 39dcf997a638..7e27d6436889 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7042,8 +7042,12 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>>   
>>   	ufshcd_hold(hba, false);
>>   	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))) {
>> +	/*
>> +	 * If the command is already aborted/completed, return FAILED. This
>> +	 * should never happen since the SCSI core serializes error handling
>> +	 * and scsi_done() calls.
> 
> I don't think that is correct. ufshcd_transfer_req_compl() gets called directly
> by the interrupt handler.

I will revert this change.

>> +	/*
>> +	 * Clear the corresponding bit from outstanding_reqs since the command
>> +	 * has been aborted successfully.
>> +	 */
>> +	spin_lock_irqsave(&hba->outstanding_lock, flags);
>> +	__clear_bit(tag, &hba->outstanding_reqs);
>> +	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>> +
>> +	ufshcd_release_scsi_cmd(hba, lrbp);
> 
> ufshcd_release_scsi_cmd() must not be called if the bit was already clear
> i.e.
> 
> 	spin_lock_irqsave(&hba->outstanding_lock, flags);
> 	rel = __test_and_clear_bit(tag, &hba->outstanding_reqs);
> 	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> 
> 	if (rel)
> 		ufshcd_release_scsi_cmd(hba, lrbp);

Will look into this.

Thanks,

Bart.

[PATCH v2 16/20] 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 7e27d6436889..6a743da7d2db 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1667,7 +1667,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++;
@@ -1827,7 +1828,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;
@@ -1962,11 +1963,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 v2 17/20] 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 6a743da7d2db..a6d3f71c6b00 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2702,6 +2702,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;
@@ -2780,7 +2786,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()) {
@@ -5986,8 +5995,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 v2 18/20] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

Remove the clock scaling lock from ufshcd_queuecommand() since it is a
performance bottleneck. Freeze request queues instead of polling the
doorbell registers to wait until pending commands have completed.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a6d3f71c6b00..9cf4a22f1950 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1070,65 +1070,6 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
 	return false;
 }
 
-static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
-					u64 wait_timeout_us)
-{
-	unsigned long flags;
-	int ret = 0;
-	u32 tm_doorbell;
-	u32 tr_doorbell;
-	bool timeout = false, do_last_check = false;
-	ktime_t start;
-
-	ufshcd_hold(hba, false);
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	/*
-	 * Wait for all the outstanding tasks/transfer requests.
-	 * Verify by checking the doorbell registers are clear.
-	 */
-	start = ktime_get();
-	do {
-		if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
-			ret = -EBUSY;
-			goto out;
-		}
-
-		tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
-		tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-		if (!tm_doorbell && !tr_doorbell) {
-			timeout = false;
-			break;
-		} else if (do_last_check) {
-			break;
-		}
-
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
-		schedule();
-		if (ktime_to_us(ktime_sub(ktime_get(), start)) >
-		    wait_timeout_us) {
-			timeout = true;
-			/*
-			 * We might have scheduled out for long time so make
-			 * sure to check if doorbells are cleared by this time
-			 * or not.
-			 */
-			do_last_check = true;
-		}
-		spin_lock_irqsave(hba->host->host_lock, flags);
-	} while (tm_doorbell || tr_doorbell);
-
-	if (timeout) {
-		dev_err(hba->dev,
-			"%s: timedout waiting for doorbell to clear (tm=0x%x, tr=0x%x)\n",
-			__func__, tm_doorbell, tr_doorbell);
-		ret = -EBUSY;
-	}
-out:
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	ufshcd_release(hba);
-	return ret;
-}
-
 /**
  * ufshcd_scale_gear - scale up/down UFS gear
  * @hba: per adapter instance
@@ -1176,37 +1117,63 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
 
 static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 {
-	#define DOORBELL_CLR_TOUT_US		(1000 * 1000) /* 1 sec */
-	int ret = 0;
+	struct scsi_device *sdev;
+
 	/*
-	 * make sure that there are no outstanding requests when
-	 * clock scaling is in progress
+	 * Make sure that no commands are in progress while the clock frequency
+	 * is being modified.
+	 *
+	 * Since ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd() lock
+	 * the clk_scaling_lock before calling blk_get_request(), lock
+	 * clk_scaling_lock before freezing the request queues to prevent lock
+	 * inversion.
 	 */
-	ufshcd_scsi_block_requests(hba);
 	down_write(&hba->clk_scaling_lock);
-
-	if (!hba->clk_scaling.is_allowed ||
-	    ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
-		ret = -EBUSY;
-		up_write(&hba->clk_scaling_lock);
-		ufshcd_scsi_unblock_requests(hba);
-		goto out;
-	}
-
+	if (!hba->clk_scaling.is_allowed)
+		goto busy;
+	blk_freeze_queue_start(hba->tmf_queue);
+	blk_freeze_queue_start(hba->cmd_queue);
+	shost_for_each_device(sdev, hba->host)
+		blk_freeze_queue_start(sdev->request_queue);
+	/*
+	 * Calling synchronize_rcu_expedited() reduces the time required to
+	 * freeze request queues from milliseconds to microseconds.
+	 */
+	synchronize_rcu_expedited();
+	shost_for_each_device(sdev, hba->host)
+		if (blk_mq_freeze_queue_wait_timeout(sdev->request_queue, HZ)
+		    <= 0)
+			goto unfreeze;
+	if (blk_mq_freeze_queue_wait_timeout(hba->cmd_queue, HZ) <= 0 ||
+	    blk_mq_freeze_queue_wait_timeout(hba->tmf_queue, HZ / 10) <= 0)
+		goto unfreeze;
 	/* let's not get into low power until clock scaling is completed */
 	ufshcd_hold(hba, false);
+	return 0;
 
-out:
-	return ret;
+unfreeze:
+	shost_for_each_device(sdev, hba->host)
+		blk_mq_unfreeze_queue(sdev->request_queue);
+	blk_mq_unfreeze_queue(hba->cmd_queue);
+	blk_mq_unfreeze_queue(hba->tmf_queue);
+
+busy:
+	up_write(&hba->clk_scaling_lock);
+	return -EBUSY;
 }
 
 static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
 {
+	struct scsi_device *sdev;
+
+	shost_for_each_device(sdev, hba->host)
+		blk_mq_unfreeze_queue(sdev->request_queue);
+	blk_mq_unfreeze_queue(hba->cmd_queue);
+	blk_mq_unfreeze_queue(hba->tmf_queue);
 	if (writelock)
 		up_write(&hba->clk_scaling_lock);
 	else
 		up_read(&hba->clk_scaling_lock);
-	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_release(hba);
 }
 
@@ -2699,9 +2666,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.
@@ -2790,8 +2754,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 e9bc07c69a80..7ec463c97d64 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -778,6 +778,7 @@ struct ufs_hba_monitor {
  * @clk_list_head: UFS host controller clocks list node head
  * @pwr_info: holds current power mode
  * @max_pwr_info: keeps the device max valid pwm
+ * @clk_scaling_lock: used to serialize device commands and clock scaling
  * @desc_size: descriptor sizes reported by device
  * @urgent_bkops_lvl: keeps track of urgent bkops level for device
  * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for

Re: [PATCH v2 18/20] scsi: ufs: Optimize the command queueing code

[Asutosh Das (asd)]

On 11/19/2021 11:57 AM, Bart Van Assche wrote:
> Remove the clock scaling lock from ufshcd_queuecommand() since it is a
> performance bottleneck. Freeze request queues instead of polling the
> doorbell registers to wait until pending commands have completed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/ufs/ufshcd.c | 124 +++++++++++++-------------------------
>   drivers/scsi/ufs/ufshcd.h |   1 +
>   2 files changed, 44 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a6d3f71c6b00..9cf4a22f1950 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1070,65 +1070,6 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
>   	return false;
>   }
>   
[...]
>   /**
>    * ufshcd_scale_gear - scale up/down UFS gear
>    * @hba: per adapter instance
> @@ -1176,37 +1117,63 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>   
>   static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
>   {
> -	#define DOORBELL_CLR_TOUT_US		(1000 * 1000) /* 1 sec */
> -	int ret = 0;
> +	struct scsi_device *sdev;
> +
>   	/*
> -	 * make sure that there are no outstanding requests when
> -	 * clock scaling is in progress
> +	 * Make sure that no commands are in progress while the clock frequency
> +	 * is being modified.
> +	 *
> +	 * Since ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd() lock
> +	 * the clk_scaling_lock before calling blk_get_request(), lock
> +	 * clk_scaling_lock before freezing the request queues to prevent lock
> +	 * inversion.
>   	 */
> -	ufshcd_scsi_block_requests(hba);
>   	down_write(&hba->clk_scaling_lock);
> -
> -	if (!hba->clk_scaling.is_allowed ||
> -	    ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
> -		ret = -EBUSY;
> -		up_write(&hba->clk_scaling_lock);
> -		ufshcd_scsi_unblock_requests(hba);
> -		goto out;
> -	}
> -
> +	if (!hba->clk_scaling.is_allowed)
> +		goto busy;
> +	blk_freeze_queue_start(hba->tmf_queue);
> +	blk_freeze_queue_start(hba->cmd_queue);
> +	shost_for_each_device(sdev, hba->host)
> +		blk_freeze_queue_start(sdev->request_queue);
This would still issue the requests present in the queue before freezing 
and that's a concern.
> +	/*
> +	 * Calling synchronize_rcu_expedited() reduces the time required to
> +	 * freeze request queues from milliseconds to microseconds.
> +	 */
> +	synchronize_rcu_expedited();
> +	shost_for_each_device(sdev, hba->host)
> +		if (blk_mq_freeze_queue_wait_timeout(sdev->request_queue, HZ)
> +		    <= 0)
> +			goto unfreeze;
> +	if (blk_mq_freeze_queue_wait_timeout(hba->cmd_queue, HZ) <= 0 ||
> +	    blk_mq_freeze_queue_wait_timeout(hba->tmf_queue, HZ / 10) <= 0)
> +		goto unfreeze;
>   	/* let's not get into low power until clock scaling is completed */
>   	ufshcd_hold(hba, false);
> +	return 0;
>   
> -out:
> -	return ret;
> +unfreeze:
> +	shost_for_each_device(sdev, hba->host)
> +		blk_mq_unfreeze_queue(sdev->request_queue);
> +	blk_mq_unfreeze_queue(hba->cmd_queue);
> +	blk_mq_unfreeze_queue(hba->tmf_queue);
> +
> +busy:
> +	up_write(&hba->clk_scaling_lock);
> +	return -EBUSY;
>   }
>   
>   static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
>   {
> +	struct scsi_device *sdev;
> +
> +	shost_for_each_device(sdev, hba->host)
> +		blk_mq_unfreeze_queue(sdev->request_queue);
> +	blk_mq_unfreeze_queue(hba->cmd_queue);
> +	blk_mq_unfreeze_queue(hba->tmf_queue);
>   	if (writelock)
>   		up_write(&hba->clk_scaling_lock);
>   	else
>   		up_read(&hba->clk_scaling_lock);
> -	ufshcd_scsi_unblock_requests(hba);
>   	ufshcd_release(hba);
>   }
>   
> @@ -2699,9 +2666,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.
> @@ -2790,8 +2754,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 e9bc07c69a80..7ec463c97d64 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -778,6 +778,7 @@ struct ufs_hba_monitor {
>    * @clk_list_head: UFS host controller clocks list node head
>    * @pwr_info: holds current power mode
>    * @max_pwr_info: keeps the device max valid pwm
> + * @clk_scaling_lock: used to serialize device commands and clock scaling
>    * @desc_size: descriptor sizes reported by device
>    * @urgent_bkops_lvl: keeps track of urgent bkops level for device
>    * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
> 


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

Re: [PATCH v2 18/20] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

On 11/22/21 9:46 AM, Asutosh Das (asd) wrote:
> On 11/19/2021 11:57 AM, Bart Van Assche wrote:
>> +    blk_freeze_queue_start(hba->tmf_queue);
>> +    blk_freeze_queue_start(hba->cmd_queue);
>> +    shost_for_each_device(sdev, hba->host)
>> +        blk_freeze_queue_start(sdev->request_queue);
>
> This would still issue the requests present in the queue before freezing 
> and that's a concern.

Isn't that exactly what the existing code is doing since the existing 
code waits until both doorbell registers are zero? See also 
ufshcd_wait_for_doorbell_clr().

Thanks,

Bart.

Re: [PATCH v2 18/20] scsi: ufs: Optimize the command queueing code

[Asutosh Das (asd)]

On 11/22/2021 10:13 AM, Bart Van Assche wrote:
> On 11/22/21 9:46 AM, Asutosh Das (asd) wrote:
>> On 11/19/2021 11:57 AM, Bart Van Assche wrote:
>>> +    blk_freeze_queue_start(hba->tmf_queue);
>>> +    blk_freeze_queue_start(hba->cmd_queue);
>>> +    shost_for_each_device(sdev, hba->host)
>>> +        blk_freeze_queue_start(sdev->request_queue);
>>
>> This would still issue the requests present in the queue before 
>> freezing and that's a concern.
> 
> Isn't that exactly what the existing code is doing since the existing 
> code waits until both doorbell registers are zero? See also 
> ufshcd_wait_for_doorbell_clr().
> 
> Thanks,
> 
> Bart.
Current code waits for the already issued requests to complete. It 
doesn't issue the yet-to-be issued requests. Wouldn't freezing the queue 
issue the requests in the context of scaling_{up/down}?
If yes, I don't think the current code is doing that.

-asd

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

Re: [PATCH v2 18/20] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

On 11/22/21 3:02 PM, Asutosh Das (asd) wrote:
> Current code waits for the already issued requests to complete. It 
> doesn't issue the yet-to-be issued requests. Wouldn't freezing the queue 
> issue the requests in the context of scaling_{up/down}?
> If yes, I don't think the current code is doing that.

Hi Asutosh,

How about the patch below that preserves most of the existing code for
preparing for clock scaling?

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, use synchronize_rcu_expedited() to wait
for ongoing ufshcd_queuecommand() calls.

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 5d214456bf82..1d929c28efaf 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;
  }
@@ -2699,9 +2706,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.
@@ -2790,8 +2794,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 c13ae56fbff8..695bede14dac 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -777,6 +777,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 v2 18/20] scsi: ufs: Optimize the command queueing code

[Asutosh Das (asd)]

On 11/22/2021 3:48 PM, Bart Van Assche wrote:
> On 11/22/21 3:02 PM, Asutosh Das (asd) wrote:
>> Current code waits for the already issued requests to complete. It 
>> doesn't issue the yet-to-be issued requests. Wouldn't freezing the 
>> queue issue the requests in the context of scaling_{up/down}?
>> If yes, I don't think the current code is doing that.
> 
> Hi Asutosh,
> 
> How about the patch below that preserves most of the existing code for
> preparing for clock scaling?
> 
> Thanks,
> 
> Bart.
> 
Hi Bart,
This looks good to me. Please push a change and I can test it out.

-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, use synchronize_rcu_expedited() to wait
> for ongoing ufshcd_queuecommand() calls.
> 
> 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 5d214456bf82..1d929c28efaf 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;
>   }
> @@ -2699,9 +2706,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.
> @@ -2790,8 +2794,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 c13ae56fbff8..695bede14dac 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -777,6 +777,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 v2 18/20] scsi: ufs: Optimize the command queueing code

[Bart Van Assche]

On 11/23/21 10:24 AM, Asutosh Das (asd) wrote:
> This looks good to me. Please push a change and I can test it out.

Thanks for having taken a look. v3 of this patch series is available at
https://github.com/bvanassche/linux/tree/ufs-for-next.

Bart.

[PATCH v2 19/20] 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, 65 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9cf4a22f1950..14043d74389d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2629,6 +2629,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;
@@ -2664,7 +2694,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()
@@ -2925,7 +2955,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	}
 	req = scsi_cmd_to_rq(scmd);
 	tag = req->tag;
-	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
+	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
 	/*
 	 * Start the request such that blk_mq_tag_idle() is called when the
 	 * device management request finishes.
@@ -5272,6 +5302,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
@@ -5282,9 +5337,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
@@ -5299,21 +5351,9 @@ 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);
+	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)
@@ -6564,6 +6604,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);
 
@@ -6705,7 +6747,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	}
 	req = scsi_cmd_to_rq(scmd);
 	tag = req->tag;
-	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
+	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n", tag);
 	/*
 	 * Start the request such that blk_mq_tag_idle() is called when the
 	 * device management request finishes.
@@ -8147,7 +8189,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,
@@ -9437,6 +9481,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 		err = -ENOMEM;
 		goto out_error;
 	}
+	host->nr_maps = 3;
 	hba = shost_priv(host);
 	hba->host = host;
 	hba->dev = dev;

Re: [PATCH v2 19/20] scsi: ufs: Implement polling support

[Bean Huo]

Bart, 

We look forward to the support of UFSHCD polling, I did a review and
test. comments as below:


On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
> 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

For 4KB random read, direct IO, and iodepth=1, we did not see an
improvement in IOPS due to this patch. Maybe the test case above is not
sufficient.


> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 85 ++++++++++++++++++++++++++++++-------
> --
>  1 file changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9cf4a22f1950..14043d74389d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2629,6 +2629,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;
> @@ -2664,7 +2694,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()
> @@ -2925,7 +2955,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>  	}
>  	req = scsi_cmd_to_rq(scmd);
>  	tag = req->tag;
> -	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> +	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n",
> tag)
probably this change should not in this patch?

>  	/*
>  	 * Start the request such that blk_mq_tag_idle() is called when
> the
>  	 * device management request finishes.
> @@ -5272,6 +5302,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
> @@ -5282,9 +5337,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
> @@ -5299,21 +5351,9 @@ 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);
> +	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;
>  }
>  

ufshcd_transfer_req_compl() will never return IRQ_NONE,
but ufshcd_intr() needs this return to check the fake interrupt.


>  int __ufshcd_write_ee_control(struct ufs_hba *hba, u32 ee_ctrl_mask)
> @@ -6564,6 +6604,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);
>  
> @@ -6705,7 +6747,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>  	}
>  	req = scsi_cmd_to_rq(scmd);
>  	tag = req->tag;
> -	WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> +	WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n",
> tag);
>  	/*
>  	 * Start the request such that blk_mq_tag_idle() is called when
> the
>  	 * device management request finishes.
> @@ -8147,7 +8189,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,
> @@ -9437,6 +9481,7 @@ int ufshcd_alloc_host(struct device *dev,
> struct ufs_hba **hba_handle)
>  		err = -ENOMEM;
>  		goto out_error;
>  	}
> +	host->nr_maps = 3;

I don't understand why not directly uses HCTX_MAX_TYPES, 3 is already
maximum.

>  	hba = shost_priv(host);
>  	hba->host = host;
>  	hba->dev = dev;

RE: [PATCH v2 19/20] scsi: ufs: Implement polling support

[Avri Altman]

> 
> Bart,
> 
> We look forward to the support of UFSHCD polling, I did a review and test.
> comments as below:
> 
> 
> On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
> > 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
> 
> For 4KB random read, direct IO, and iodepth=1, we did not see an
> improvement in IOPS due to this patch. Maybe the test case above is not
> sufficient.
Does your setup contains commit af1830956dc3 (scsi: core: Add mq_poll support to SCSI layer) ?

Thanks,
Avri

> 
> 
> >
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 85 ++++++++++++++++++++++++++++++-------
> > --
> >  1 file changed, 65 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 9cf4a22f1950..14043d74389d 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -2629,6 +2629,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;
> > @@ -2664,7 +2694,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()
> > @@ -2925,7 +2955,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> > *hba,
> >       }
> >       req = scsi_cmd_to_rq(scmd);
> >       tag = req->tag;
> > -     WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> > +     WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n",
> > tag)
> probably this change should not in this patch?
> 
> >       /*
> >        * Start the request such that blk_mq_tag_idle() is called when
> > the
> >        * device management request finishes.
> > @@ -5272,6 +5302,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
> > @@ -5282,9 +5337,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 @@ -5299,21 +5351,9 @@ 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);
> > +     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;
> >  }
> >
> 
> ufshcd_transfer_req_compl() will never return IRQ_NONE, but ufshcd_intr()
> needs this return to check the fake interrupt.
> 
> 
> >  int __ufshcd_write_ee_control(struct ufs_hba *hba, u32 ee_ctrl_mask)
> > @@ -6564,6 +6604,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);
> >
> > @@ -6705,7 +6747,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> > ufs_hba *hba,
> >       }
> >       req = scsi_cmd_to_rq(scmd);
> >       tag = req->tag;
> > -     WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
> > +     WARN_ONCE(tag < 0 || tag >= hba->nutrs, "Invalid tag %d\n",
> > tag);
> >       /*
> >        * Start the request such that blk_mq_tag_idle() is called when
> > the
> >        * device management request finishes.
> > @@ -8147,7 +8189,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,
> > @@ -9437,6 +9481,7 @@ int ufshcd_alloc_host(struct device *dev, struct
> > ufs_hba **hba_handle)
> >               err = -ENOMEM;
> >               goto out_error;
> >       }
> > +     host->nr_maps = 3;
> 
> I don't understand why not directly uses HCTX_MAX_TYPES, 3 is already
> maximum.
> 
> >       hba = shost_priv(host);
> >       hba->host = host;
> >       hba->dev = dev;

Re: [PATCH v2 19/20] scsi: ufs: Implement polling support

[Bean Huo]

On Tue, 2021-11-30 at 08:57 +0000, Avri Altman wrote:
> > 
> > Bart,
> > 
> > We look forward to the support of UFSHCD polling, I did a review
> > and test.
> > comments as below:
> > 
> > 
> > On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
> > > 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
> > 
> > For 4KB random read, direct IO, and iodepth=1, we did not see an
> > improvement in IOPS due to this patch. Maybe the test case above is
> > not
> > sufficient.
> Does your setup contains commit af1830956dc3 (scsi: core: Add mq_poll
> support to SCSI layer) ?
> 
> Thanks,
> Avri
> 

yes, that commit was mainlined since v5.13, and I tested this patch on
v5.16.

Re: [PATCH v2 19/20] scsi: ufs: Implement polling support

[Bart Van Assche]

On 11/30/21 12:43 AM, Bean Huo wrote:
> On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
>> 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
> 
> For 4KB random read, direct IO, and iodepth=1, we did not see an
> improvement in IOPS due to this patch. Maybe the test case above is not
> sufficient.

Hi Bean,

Which test has been run? Polling is only enabled if --hipri=1 is specified
to fio and --ioengine=io_uring is the only I/O engine on Android that supports
that flag.

Thanks,

Bart.

Re: [PATCH v2 19/20] scsi: ufs: Implement polling support

[Bean Huo]

On Tue, 2021-11-30 at 06:26 -0800, Bart Van Assche wrote:
> On 11/30/21 12:43 AM, Bean Huo wrote:
> > On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
> > > 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
> > 
> > For 4KB random read, direct IO, and iodepth=1, we did not see an
> > improvement in IOPS due to this patch. Maybe the test case above is
> > not
> > sufficient.
> 
> Hi Bean,
> 
> Which test has been run? Polling is only enabled if --hipri=1 is
> specified
> to fio and --ioengine=io_uring is the only I/O engine on Android that
> supports
> that flag.
> 
> Thanks,
> 
> Bart.

Hi Bart,

It is the test case in your commit message. If iodepth=1, there is no
performance improvement. Increase the io-depth to multiple, for
example, let iodepth= CPU core counter. I see that the interrupt
overhead is significantly increased when the request is completed, so
IO_polling will win compared to the interrupt mode.

Kind regards,
Bean

Re: [PATCH v2 19/20] scsi: ufs: Implement polling support

[Bart Van Assche]

On 11/30/21 7:40 AM, Bean Huo wrote:
> It is the test case in your commit message. If iodepth=1, there is no
> performance improvement. Increase the io-depth to multiple, for
> example, let iodepth= CPU core counter. I see that the interrupt
> overhead is significantly increased when the request is completed, so
> IO_polling will win compared to the interrupt mode.

Hi Bean,

It is not guaranteed that polling will result in a significant performance
improvement. I assume that on my test setup the improvement is so
significant because the interrupt coalescing delay. Maybe the interrupt
coalescing delay is much smaller on your test setup.

Bart.

Re: [PATCH v2 19/20] scsi: ufs: Implement polling support

[Bart Van Assche]

On 11/30/21 12:43 AM, Bean Huo wrote:
> On Fri, 2021-11-19 at 11:57 -0800, Bart Van Assche wrote:
>> +	return IRQ_HANDLED;
>>   }
>>   
> 
> ufshcd_transfer_req_compl() will never return IRQ_NONE,
> but ufshcd_intr() needs this return to check the fake interrupt.

I don't think that it is possible to implement polling and detect
spurious interrupts without affecting performance negatively. Hence
the choice to never return IRQ_NONE. Is spurious interrupt detection
that important or is this only required for debugging host controllers?
I consider helping with hardware debugging as out of scope for the UFS
driver.

>> @@ -9437,6 +9481,7 @@ int ufshcd_alloc_host(struct device *dev,
>> struct ufs_hba **hba_handle)
>>   		err = -ENOMEM;
>>   		goto out_error;
>>   	}
>> +	host->nr_maps = 3;
> 
> I don't understand why not directly uses HCTX_MAX_TYPES, 3 is already
> maximum.

If new map types would be introduced in the future, we still need 3 maps.
Hence the choice for '3' instead of HCTX_MAX_TYPES.

Thanks,

Bart.

[PATCH v2 20/20] scsi: ufs: Fix race conditions related to driver data

[Bart Van Assche]

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 14043d74389d..b78e92298407 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9523,6 +9523,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");