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
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 <[email protected]>
Signed-off-by: Po-Wen Kao <[email protected]>
---
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
From: Stanley Chu <[email protected]>
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 <[email protected]>
Signed-off-by: Stanley Chu <[email protected]>
Tested-by: Po-Wen Kao <[email protected]>
---
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
On Sat, Jun 10, 2023 at 10:17 AM Po-Wen Kao <[email protected]> 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 <[email protected]>
> Signed-off-by: Po-Wen Kao <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
On 6/9/23 19:15, Po-Wen Kao wrote:
> From: Stanley Chu <[email protected]>
>
> 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.
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 <[email protected]>
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 <[email protected]>
Hi Bart,
On Sun, Jun 11, 2023 at 10:04 PM Bart Van Assche <[email protected]> wrote:
>
> On 6/9/23 19:15, Po-Wen Kao wrote:
> > From: Stanley Chu <[email protected]>
> >
> > 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
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
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