[PATCH v2 0/2] ufs: mcq: Share first hwq for dev comamnd and IO request

[PATCH v2 0/2] ufs: mcq: Share first hwq for dev comamnd and IO request

[Po-Wen Kao]

v1 -> v2:
- Remove accidentally added ufshcd_add_command_trace() call in
  [PATCH v1 1/2]
- Add Test-by tag for both patch and refactor commit message

Po-Wen Kao (1):
  scsi: ufs: core: Remove dedicated hwq for dev command

Stanley Chu (1):
  scsi: ufs: mcq: Fix the incorrect OCS value for the device command

 drivers/ufs/core/ufs-mcq.c     | 14 ++++----------
 drivers/ufs/core/ufshcd-priv.h |  1 -
 drivers/ufs/core/ufshcd.c      | 14 +++++++++-----
 include/ufs/ufshcd.h           |  1 -
 4 files changed, 13 insertions(+), 17 deletions(-)

--
2.18.0

[PATCH v2 1/2] scsi: ufs: mcq: Fix the incorrect OCS value for the device command

[Po-Wen Kao]

From: Stanley Chu <stanley.chu@mediatek.com>

In MCQ mode, when a device command uses a hardware queue shared
with other commands, a race condition may occur in the following scenario:

1. A device command is completed in CQx with CQE entry "e".
2. The interrupt handler copies the "cqe" pointer to "hba->dev_cmd.cqe"
   and completes "hba->dev_cmd.complete".
3. The "ufshcd_wait_for_dev_cmd()" function is awakened and retrieves
   the OCS value from "hba->dev_cmd.cqe".

However, there is a possibility that the CQE entry "e" will be overwritten
by newly completed commands in CQx, resulting in an incorrect OCS value
being received by "ufshcd_wait_for_dev_cmd()".

To avoid this race condition, the OCS value should be immediately copied
to the struct "lrb" of the device command. Then "ufshcd_wait_for_dev_cmd()"
can retrieve the OCS value from the struct "lrb".

Fixes: b5167638ec82 ("scsi: ufs: core: mcq: Add support to allocate multiple queues")
Suggested-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Tested-by: Po-Wen Kao <powen.kao@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 10 +++++++---
 include/ufs/ufshcd.h      |  1 -
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5da62248ebc4..593790fa4837 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3086,7 +3086,7 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 		 * not trigger any race conditions.
 		 */
 		hba->dev_cmd.complete = NULL;
-		err = ufshcd_get_tr_ocs(lrbp, hba->dev_cmd.cqe);
+		err = ufshcd_get_tr_ocs(lrbp, NULL);
 		if (!err)
 			err = ufshcd_dev_cmd_completion(hba, lrbp);
 	} else {
@@ -3182,7 +3182,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 		goto out;
 
 	hba->dev_cmd.complete = &wait;
-	hba->dev_cmd.cqe = NULL;
 
 	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
 
@@ -5431,6 +5430,7 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 {
 	struct ufshcd_lrb *lrbp;
 	struct scsi_cmnd *cmd;
+	enum utp_ocs ocs;
 
 	lrbp = &hba->lrb[task_tag];
 	lrbp->compl_time_stamp = ktime_get();
@@ -5446,7 +5446,11 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 	} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
 		   lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
 		if (hba->dev_cmd.complete) {
-			hba->dev_cmd.cqe = cqe;
+			if (cqe) {
+				ocs = le32_to_cpu(cqe->status) & MASK_OCS;
+				lrbp->utr_descriptor_ptr->header.dword_2 =
+					cpu_to_le32(ocs);
+			}
 			complete(hba->dev_cmd.complete);
 			ufshcd_clk_scaling_update_busy(hba);
 		}
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 9b2d1859f885..602615e6d1bf 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -225,7 +225,6 @@ struct ufs_dev_cmd {
 	struct mutex lock;
 	struct completion *complete;
 	struct ufs_query query;
-	struct cq_entry *cqe;
 };
 
 /**
-- 
2.18.0

[PATCH v2 2/2] scsi: ufs: core: Remove dedicated hwq for dev command

[Po-Wen Kao]

This patch depends on patch
"scsi: ufs: mcq: Fix the incorrect OCS value for the device command"
which take care of OCS value of dev commands under mcq mode.

We are safe to share first hwq for dev commnad and IO request here.

Tested-by: Po-Wen Kao <powen.kao@mediatek.com>
Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
---
 drivers/ufs/core/ufs-mcq.c     | 14 ++++----------
 drivers/ufs/core/ufshcd-priv.h |  1 -
 drivers/ufs/core/ufshcd.c      |  4 ++--
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 785fc9762cad..9f70abbaa4ad 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -20,12 +20,10 @@
 #define MAX_QUEUE_SUP GENMASK(7, 0)
 #define UFS_MCQ_MIN_RW_QUEUES 2
 #define UFS_MCQ_MIN_READ_QUEUES 0
-#define UFS_MCQ_NUM_DEV_CMD_QUEUES 1
 #define UFS_MCQ_MIN_POLL_QUEUES 0
 #define QUEUE_EN_OFFSET 31
 #define QUEUE_ID_OFFSET 16
 
-#define MAX_DEV_CMD_ENTRIES	2
 #define MCQ_CFG_MAC_MASK	GENMASK(16, 8)
 #define MCQ_QCFG_SIZE		0x40
 #define MCQ_ENTRY_SIZE_IN_DWORD	8
@@ -115,8 +113,7 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 	u32 utag = blk_mq_unique_tag(req);
 	u32 hwq = blk_mq_unique_tag_to_hwq(utag);
 
-	/* uhq[0] is used to serve device commands */
-	return &hba->uhq[hwq + UFSHCD_MCQ_IO_QUEUE_OFFSET];
+	return &hba->uhq[hwq];
 }
 
 /**
@@ -160,8 +157,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
 	/* maxq is 0 based value */
 	hba_maxq = FIELD_GET(MAX_QUEUE_SUP, hba->mcq_capabilities) + 1;
 
-	tot_queues = UFS_MCQ_NUM_DEV_CMD_QUEUES + read_queues + poll_queues +
-			rw_queues;
+	tot_queues = read_queues + poll_queues + rw_queues;
 
 	if (hba_maxq < tot_queues) {
 		dev_err(hba->dev, "Total queues (%d) exceeds HC capacity (%d)\n",
@@ -169,7 +165,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
 		return -EOPNOTSUPP;
 	}
 
-	rem = hba_maxq - UFS_MCQ_NUM_DEV_CMD_QUEUES;
+	rem = hba_maxq;
 
 	if (rw_queues) {
 		hba->nr_queues[HCTX_TYPE_DEFAULT] = rw_queues;
@@ -195,7 +191,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
 	for (i = 0; i < HCTX_MAX_TYPES; i++)
 		host->nr_hw_queues += hba->nr_queues[i];
 
-	hba->nr_hw_queues = host->nr_hw_queues + UFS_MCQ_NUM_DEV_CMD_QUEUES;
+	hba->nr_hw_queues = host->nr_hw_queues;
 	return 0;
 }
 
@@ -445,8 +441,6 @@ int ufshcd_mcq_init(struct ufs_hba *hba)
 
 	/* The very first HW queue serves device commands */
 	hba->dev_cmd_queue = &hba->uhq[0];
-	/* Give dev_cmd_queue the minimal number of entries */
-	hba->dev_cmd_queue->max_entries = MAX_DEV_CMD_ENTRIES;
 
 	host->host_tagset = 1;
 	return 0;
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index aa88e60ea1f6..9566a95aeed9 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -84,7 +84,6 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag);
 void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
 			     struct ufshcd_lrb *lrbp);
 
-#define UFSHCD_MCQ_IO_QUEUE_OFFSET	1
 #define SD_ASCII_STD true
 #define SD_RAW false
 int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 593790fa4837..f10318b7cbc1 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5503,7 +5503,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	struct ufs_hw_queue *hwq;
 
 	if (is_mcq_enabled(hba)) {
-		hwq = &hba->uhq[queue_num + UFSHCD_MCQ_IO_QUEUE_OFFSET];
+		hwq = &hba->uhq[queue_num];
 
 		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
 	}
@@ -5557,7 +5557,7 @@ static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba,
 
 		utag = blk_mq_unique_tag(scsi_cmd_to_rq(cmd));
 		hwq_num = blk_mq_unique_tag_to_hwq(utag);
-		hwq = &hba->uhq[hwq_num + UFSHCD_MCQ_IO_QUEUE_OFFSET];
+		hwq = &hba->uhq[hwq_num];
 
 		if (force_compl) {
 			ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
-- 
2.18.0

Re: [PATCH v2 2/2] scsi: ufs: core: Remove dedicated hwq for dev command

[Stanley Chu]

On Sat, Jun 10, 2023 at 10:17 AM Po-Wen Kao <powen.kao@mediatek.com> wrote:
>
> This patch depends on patch
> "scsi: ufs: mcq: Fix the incorrect OCS value for the device command"
> which take care of OCS value of dev commands under mcq mode.
>
> We are safe to share first hwq for dev commnad and IO request here.
>
> Tested-by: Po-Wen Kao <powen.kao@mediatek.com>
> Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH v2 1/2] scsi: ufs: mcq: Fix the incorrect OCS value for the device command

[Bart Van Assche]

On 6/9/23 19:15, Po-Wen Kao wrote:
> From: Stanley Chu <stanley.chu@mediatek.com>
> 
> In MCQ mode, when a device command uses a hardware queue shared
> with other commands, a race condition may occur in the following scenario:
> 
> 1. A device command is completed in CQx with CQE entry "e".
> 2. The interrupt handler copies the "cqe" pointer to "hba->dev_cmd.cqe"
>     and completes "hba->dev_cmd.complete".
> 3. The "ufshcd_wait_for_dev_cmd()" function is awakened and retrieves
>     the OCS value from "hba->dev_cmd.cqe".
> 
> However, there is a possibility that the CQE entry "e" will be overwritten
> by newly completed commands in CQx, resulting in an incorrect OCS value
> being received by "ufshcd_wait_for_dev_cmd()".
> 
> To avoid this race condition, the OCS value should be immediately copied
> to the struct "lrb" of the device command. Then "ufshcd_wait_for_dev_cmd()"
> can retrieve the OCS value from the struct "lrb".

Since with this patch applied ufs_dev_cmd.cqe is always NULL, please 
remove the 'cqe' member from struct ufs_dev_cmd.

Thanks,

Bart.

Re: [PATCH v2 2/2] scsi: ufs: core: Remove dedicated hwq for dev command

[Bart Van Assche]

On 6/9/23 19:15, Po-Wen Kao wrote:
> This patch depends on patch
> "scsi: ufs: mcq: Fix the incorrect OCS value for the device command"
> which take care of OCS value of dev commands under mcq mode.
> 
> We are safe to share first hwq for dev commnad and IO request here.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 1/2] scsi: ufs: mcq: Fix the incorrect OCS value for the device command

[Stanley Chu]

Hi Bart,

On Sun, Jun 11, 2023 at 10:04 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 6/9/23 19:15, Po-Wen Kao wrote:
> > From: Stanley Chu <stanley.chu@mediatek.com>
> >
> > In MCQ mode, when a device command uses a hardware queue shared
> > with other commands, a race condition may occur in the following scenario:
> >
> > 1. A device command is completed in CQx with CQE entry "e".
> > 2. The interrupt handler copies the "cqe" pointer to "hba->dev_cmd.cqe"
> >     and completes "hba->dev_cmd.complete".
> > 3. The "ufshcd_wait_for_dev_cmd()" function is awakened and retrieves
> >     the OCS value from "hba->dev_cmd.cqe".
> >
> > However, there is a possibility that the CQE entry "e" will be overwritten
> > by newly completed commands in CQx, resulting in an incorrect OCS value
> > being received by "ufshcd_wait_for_dev_cmd()".
> >
> > To avoid this race condition, the OCS value should be immediately copied
> > to the struct "lrb" of the device command. Then "ufshcd_wait_for_dev_cmd()"
> > can retrieve the OCS value from the struct "lrb".
>
> Since with this patch applied ufs_dev_cmd.cqe is always NULL, please
> remove the 'cqe' member from struct ufs_dev_cmd.

This patch removes the 'cqe' member from the struct ufs_dev_cmd, as
shown in the code section below. Please let us know if anything else
has been missed. Very appreciated.

diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 9b2d1859f885..602615e6d1bf 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -225,7 +225,6 @@ struct ufs_dev_cmd {
        struct mutex lock;
        struct completion *complete;
        struct ufs_query query;
-       struct cq_entry *cqe;
 };

Thanks,

Stanley

Re: [PATCH v2 1/2] scsi: ufs: mcq: Fix the incorrect OCS value for the device command

[Bart Van Assche]

On 6/11/23 07:49, Stanley Chu wrote:
> This patch removes the 'cqe' member from the struct ufs_dev_cmd, as
> shown in the code section below. Please let us know if anything else
> has been missed. Very appreciated.
> 
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 9b2d1859f885..602615e6d1bf 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -225,7 +225,6 @@ struct ufs_dev_cmd {
>          struct mutex lock;
>          struct completion *complete;
>          struct ufs_query query;
> -       struct cq_entry *cqe;
>   };

I misread the patch. Since the patch looks fine to me after having taken 
a second look:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 0/2] ufs: mcq: Share first hwq for dev comamnd and IO request

[Martin K. Petersen]

Po-Wen,

> v1 -> v2:
> - Remove accidentally added ufshcd_add_command_trace() call in
>   [PATCH v1 1/2]
> - Add Test-by tag for both patch and refactor commit message

Applied to 6.5/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 0/2] ufs: mcq: Share first hwq for dev comamnd and IO request

[Martin K. Petersen]

On Sat, 10 Jun 2023 10:15:50 +0800, Po-Wen Kao wrote:

> v1 -> v2:
> - Remove accidentally added ufshcd_add_command_trace() call in
>   [PATCH v1 1/2]
> - Add Test-by tag for both patch and refactor commit message
> 
> Po-Wen Kao (1):
>   scsi: ufs: core: Remove dedicated hwq for dev command
> 
> [...]

Applied to 6.5/scsi-queue, thanks!

[1/2] scsi: ufs: mcq: Fix the incorrect OCS value for the device command
      https://git.kernel.org/mkp/scsi/c/0fef6bb730c4
[2/2] scsi: ufs: core: Remove dedicated hwq for dev command
      https://git.kernel.org/mkp/scsi/c/ccb23dc3435a

-- 
Martin K. Petersen	Oracle Linux Engineering