[PATCH v2 1/1] scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter

[PATCH v2 1/1] scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter

[Po-Wen Kao]

A dedicated queue for dev commands is not mandatory, hence let
UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues`
to allow sharing first hw queue for dev commands.

When `dev_cmd_queues` is set to 0, the hwq 0 will be shared for I/O
requests and dev commands. In the same hwq, commands are processed based
on submission order hence might take longer to dispatch dev command
under heavy traffic. For the host with dedicated hwq for dev commands
can benefit in such scenario.

Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>
---
 drivers/ufs/core/ufs-mcq.c     | 35 +++++++++++++++++++++++++++-------
 drivers/ufs/core/ufshcd-priv.h |  2 +-
 drivers/ufs/core/ufshcd.c      |  2 +-
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 51b3c6ae781d..4ef48c84e62f 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -16,7 +16,8 @@
 #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_MAX_DEV_CMD_QUEUES 1
+#define UFS_MCQ_MIN_DEV_CMD_QUEUES 0
 #define UFS_MCQ_MIN_POLL_QUEUES 0
 #define QUEUE_EN_OFFSET 31
 #define QUEUE_ID_OFFSET 16
@@ -75,6 +76,23 @@ module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644);
 MODULE_PARM_DESC(poll_queues,
 		 "Number of poll queues used for r/w. Default value is 1");
 
+static int dev_cmd_queue_count_set(const char *val, const struct kernel_param *kp)
+{
+	return param_set_uint_minmax(val, kp,
+				     UFS_MCQ_MIN_DEV_CMD_QUEUES,
+				     UFS_MCQ_MAX_DEV_CMD_QUEUES);
+}
+
+static const struct kernel_param_ops dev_cmd_queue_count_ops = {
+	.set = dev_cmd_queue_count_set,
+	.get = param_get_uint,
+};
+
+unsigned int dev_cmd_queues = 1;
+module_param_cb(dev_cmd_queues, &dev_cmd_queue_count_ops, &dev_cmd_queues, 0644);
+MODULE_PARM_DESC(dev_cmd_queues,
+		 "Number of queues used for dev command. Default value is 1");
+
 /**
  * ufshcd_mcq_config_mac - Set the #Max Activ Cmds.
  * @hba: per adapter instance
@@ -109,7 +127,7 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 	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 + dev_cmd_queues];
 }
 
 /**
@@ -153,7 +171,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 +
+	tot_queues = dev_cmd_queues + read_queues + poll_queues +
 			rw_queues;
 
 	if (hba_maxq < tot_queues) {
@@ -162,7 +180,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 - dev_cmd_queues;
 
 	if (rw_queues) {
 		hba->nr_queues[HCTX_TYPE_DEFAULT] = rw_queues;
@@ -188,7 +206,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 + dev_cmd_queues;
 	return 0;
 }
 
@@ -424,8 +442,11 @@ 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;
+	if (dev_cmd_queues) {
+		/* Give dedicated 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 d53b93c21a0c..b490d645f12c 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -78,7 +78,6 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
 				       struct ufs_hw_queue *hwq);
 
-#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,
@@ -287,6 +286,7 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
+extern unsigned int dev_cmd_queues;
 extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /**
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4ec8dacb447c..6361e21ca1c5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5493,7 +5493,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 + dev_cmd_queues];
 
 		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
 	}
-- 
2.18.0

Re: [PATCH v2 1/1] scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter

[AngeloGioacchino Del Regno]

Il 30/05/23 04:35, Po-Wen Kao ha scritto:
> A dedicated queue for dev commands is not mandatory, hence let
> UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues`
> to allow sharing first hw queue for dev commands.
> 
> When `dev_cmd_queues` is set to 0, the hwq 0 will be shared for I/O
> requests and dev commands. In the same hwq, commands are processed based
> on submission order hence might take longer to dispatch dev command
> under heavy traffic. For the host with dedicated hwq for dev commands
> can benefit in such scenario.
> 
> Signed-off-by: Po-Wen Kao <powen.kao@mediatek.com>


I imagine that MediaTek's UFS IP does not support dev_cmd_queues=1, does it?

In that case, this should not be a UFS module parameter, but a setting that
you provide from ufs-mediatek instead.

Regards,
Angelo

> ---
>   drivers/ufs/core/ufs-mcq.c     | 35 +++++++++++++++++++++++++++-------
>   drivers/ufs/core/ufshcd-priv.h |  2 +-
>   drivers/ufs/core/ufshcd.c      |  2 +-
>   3 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 51b3c6ae781d..4ef48c84e62f 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -16,7 +16,8 @@
>   #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_MAX_DEV_CMD_QUEUES 1
> +#define UFS_MCQ_MIN_DEV_CMD_QUEUES 0
>   #define UFS_MCQ_MIN_POLL_QUEUES 0
>   #define QUEUE_EN_OFFSET 31
>   #define QUEUE_ID_OFFSET 16
> @@ -75,6 +76,23 @@ module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644);
>   MODULE_PARM_DESC(poll_queues,
>   		 "Number of poll queues used for r/w. Default value is 1");
>   
> +static int dev_cmd_queue_count_set(const char *val, const struct kernel_param *kp)
> +{
> +	return param_set_uint_minmax(val, kp,
> +				     UFS_MCQ_MIN_DEV_CMD_QUEUES,
> +				     UFS_MCQ_MAX_DEV_CMD_QUEUES);
> +}
> +
> +static const struct kernel_param_ops dev_cmd_queue_count_ops = {
> +	.set = dev_cmd_queue_count_set,
> +	.get = param_get_uint,
> +};
> +
> +unsigned int dev_cmd_queues = 1;
> +module_param_cb(dev_cmd_queues, &dev_cmd_queue_count_ops, &dev_cmd_queues, 0644);
> +MODULE_PARM_DESC(dev_cmd_queues,
> +		 "Number of queues used for dev command. Default value is 1");
> +
>   /**
>    * ufshcd_mcq_config_mac - Set the #Max Activ Cmds.
>    * @hba: per adapter instance
> @@ -109,7 +127,7 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
>   	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 + dev_cmd_queues];
>   }
>   
>   /**
> @@ -153,7 +171,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 +
> +	tot_queues = dev_cmd_queues + read_queues + poll_queues +
>   			rw_queues;
>   
>   	if (hba_maxq < tot_queues) {
> @@ -162,7 +180,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 - dev_cmd_queues;
>   
>   	if (rw_queues) {
>   		hba->nr_queues[HCTX_TYPE_DEFAULT] = rw_queues;
> @@ -188,7 +206,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 + dev_cmd_queues;
>   	return 0;
>   }
>   
> @@ -424,8 +442,11 @@ 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;
> +	if (dev_cmd_queues) {
> +		/* Give dedicated 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 d53b93c21a0c..b490d645f12c 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -78,7 +78,6 @@ struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
>   unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
>   				       struct ufs_hw_queue *hwq);
>   
> -#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,
> @@ -287,6 +286,7 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
>   	return -EOPNOTSUPP;
>   }
>   
> +extern unsigned int dev_cmd_queues;
>   extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
>   
>   /**
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 4ec8dacb447c..6361e21ca1c5 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5493,7 +5493,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 + dev_cmd_queues];
>   
>   		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
>   	}

Re: [PATCH v2 1/1] scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter

[Powen Kao (高伯文)]

On Tue, 2023-05-30 at 09:24 +0200, AngeloGioacchino Del Regno wrote:
> 
> I imagine that MediaTek's UFS IP does not support dev_cmd_queues=1,
> does it?
> 
> In that case, this should not be a UFS module parameter, but a
> setting that
> you provide from ufs-mediatek instead.
> 
> Regards,
> Angelo

Hi Angelo

Our IP do work with dev_cmd_queues=1, but sacrifice one hwq
specifically for dev commands leaving (N-1) hwq to (N) CPUs for
submission doesn't make much sense.

Actually I would prefer making it a dts property than module parameter,
however, to match convention like `poll_queues` module parameters is
chosen.

Po-Wen

Re: [PATCH v2 1/1] scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter

[Bart Van Assche]

On 5/29/23 19:35, Po-Wen Kao wrote:
> A dedicated queue for dev commands is not mandatory, hence let
> UFS_MCQ_NUM_DEV_CMD_QUEUES become module parameter `dev_cmd_queues`
> to allow sharing first hw queue for dev commands.
> 
> When `dev_cmd_queues` is set to 0, the hwq 0 will be shared for I/O
> requests and dev commands. In the same hwq, commands are processed based
> on submission order hence might take longer to dispatch dev command
> under heavy traffic. For the host with dedicated hwq for dev commands
> can benefit in such scenario.

The UFS core has already too many kernel module parameters. I don't 
think that we need more kernel module parameters. Instead of adding a 
new kernel module parameter, I propose to never reserve a hardware queue 
for device commands. Reserving a hardware queue for device commands 
seems wasteful to me.

Thanks,

Bart.