| Message ID | 20230328103000.10757-1-powen.kao@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: ufs: core: Make UFS_MCQ_NUM_DEV_CMD_QUEUES a module parameter | expand |
Hi, Powen, On Tue, Mar 28, 2023 at 6:38 PM Po-Wen Kao <powen.kao@mediatek.com> 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. > > 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(-) Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
On 3/28/23 03:29, 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. Which queue is selected for device management commands? What is the impact of this change? If a device command is queued on a queue with multiple pending commands, does that mean that it can take long for the device command to reach the UFS device? The answer to the above questions should be in the patch description. Please expand the patch description. > +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"); I prefer a solution that does not require any new kernel module parameters. That means either a dedicated device command queue for all host controllers or no dedicated device command queue for any host controller. Thanks, Bart.
On Tue, 2023-03-28 at 20:17 -0700, Bart Van Assche wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > On 3/28/23 03:29, 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. > > Which queue is selected for device management commands? Queue 0 is selected by default. @@ -423,8 +441,11 @@ int ufshcd_mcq_init(struct ufs_hba *hba) /* The very first HW queue serves device commands */ hba->dev_cmd_queue = &hba->uhq[0]; > > What is the impact of this change? If a device command is queued on a > queue with multiple pending commands, does that mean that it can take > long for the device command to reach the UFS device? > > The answer to the above questions should be in the patch description. > Please expand the patch description. I will make commit message clear in next update. > > +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"); > > I prefer a solution that does not require any new kernel module > parameters. That means either a dedicated device command queue for > all > host controllers or no dedicated device command queue for any host > controller. The motivation of this patch is to adapt UFS driver to different host hardware. IMHO, some host may implement a dedicated (extra) queue for dev commands but not all does. In general, one hw queue per CPU (IO queue) topology is desired, hence sharing a hw queue is inevitable on those host without a dedicated dev cmd queue. The host with dedicated queue will keep the benifit of having fast channel for dev commands. Thanks
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 31df052fbc41..911dc1cbe4bb 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]; } /** @@ -152,7 +170,7 @@ static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) hba_maxq = FIELD_GET(MAX_QUEUE_SUP, hba->mcq_capabilities); - 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) { @@ -161,7 +179,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; @@ -187,7 +205,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; } @@ -423,8 +441,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 529f8507a5e4..bad611ac390e 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 acae4e194ec4..f2c62a41bb33 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -5513,7 +5513,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); }
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. 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(-)