Re: [PATCH v2 3/4] scsi: ufs: Enable the BLK_MQ_F_BLOCKING flag

[Adrian Hunter <adrian.hunter@intel.com>]

On 26/05/23 00:16, Bart Van Assche wrote:
> On 5/23/23 22:55, Adrian Hunter wrote:
>> On 23/05/23 22:57, Bart Van Assche wrote:
>>> On 5/23/23 12:19, Adrian Hunter wrote:
>>>> On 23/05/23 20:10, Bart Van Assche wrote:
>>>>> The overhead of BLK_MQ_F_BLOCKING is small relative to the
>>>>> time required to queue a UFS command so I think enabling BLK_MQ_F_BLOCKING for all UFS host controllers is fine.
>>>>
>>>> Doesn't it also force the queue to be run asynchronously always?
>>>>
>>>> But in any case, it doesn't seem like something to force on drivers just because it would take a bit more coding to make it optional.
>>>
>>> Making BLK_MQ_F_BLOCKING optional would complicate testing of the UFS driver. Although it is possible to make BLK_MQ_F_BLOCKING optional, I'm wondering whether it is worth it? I haven't noticed any performance difference in my tests with BLK_MQ_F_BLOCKING enabled compared to BLK_MQ_F_BLOCKING disabled.
>>
>> It is hard to know the effects, especially wrt to future hardware.
> 
> Are you perhaps referring to performance effects? I think the block
> layer can be modified to run the queue synchronously in most cases even
> if BLK_MQ_F_BLOCKING is set. The patch below works fine but is probably
> not acceptable for upstream because it uses in_atomic(): [ ... ]

Why would we want to when we don't have to?

>> What about something like this? [ ... ]
> 
> This introduces a redundancy and a potential for a conflict between the
> "nonblocking" flag and the UFSHCD_CAP_CLK_GATING flag. It is unfortunate
> that hba->caps is set so late otherwise we could use the result of
> (hba->caps & UFSHCD_CAP_CLK_GATING) to check whether or not
> BLK_MQ_F_BLOCKING is needed.
> 
> Additionally, this patch introduces a use-after-free issue since it
> causes scsi_host_alloc() to store a pointer to a stack variable (sht)
> into a SCSI host structure.

So with those fixed and the vops->may_block instead of vops->nonblocking:


diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 82321b8b32bc..6a4389f02b4e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2027,6 +2027,11 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 	if (!ufshcd_is_clkgating_allowed(hba))
 		return;
 
+	if (!hba->host->hostt->queuecommand_may_block) {
+		dev_warn(hba->dev, "Nonblocking queue, so clock gating is not enabled!\n");
+		return;
+	}
+
 	hba->clk_gating.state = CLKS_ON;
 
 	hba->clk_gating.delay_ms = 150;
@@ -8708,33 +8713,41 @@ static struct ufs_hba_variant_params ufs_hba_vps = {
 	.ondemand_data.downdifferential	= 5,
 };
 
-static const 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,
-	.change_queue_depth	= ufshcd_change_queue_depth,
-	.eh_abort_handler	= ufshcd_abort,
-	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
-	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
-	.eh_timed_out		= ufshcd_eh_timed_out,
-	.this_id		= -1,
-	.sg_tablesize		= SG_ALL,
-	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
-	.can_queue		= UFSHCD_CAN_QUEUE,
-	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
-	.max_sectors		= (1 << 20) / SECTOR_SIZE, /* 1 MiB */
-	.max_host_blocked	= 1,
-	.track_queue_depth	= 1,
-	.skip_settle_delay	= 1,
+#define UFSHCD_SHT_COMMON \
+	.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,			\
+	.change_queue_depth	= ufshcd_change_queue_depth,		\
+	.eh_abort_handler	= ufshcd_abort,				\
+	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,	\
+	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,	\
+	.eh_timed_out		= ufshcd_eh_timed_out,			\
+	.this_id		= -1,					\
+	.sg_tablesize		= SG_ALL,				\
+	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,			\
+	.can_queue		= UFSHCD_CAN_QUEUE,			\
+	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,		\
+	.max_sectors		= (1 << 20) / SECTOR_SIZE, /* 1 MiB */	\
+	.max_host_blocked	= 1,					\
+	.track_queue_depth	= 1,					\
+	.skip_settle_delay	= 1,					\
+	.sdev_groups		= ufshcd_driver_groups,			\
+	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS
+
+static const struct scsi_host_template ufshcd_sht_blocking = {
+	UFSHCD_SHT_COMMON,
 	.queuecommand_may_block = true,
-	.sdev_groups		= ufshcd_driver_groups,
-	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
+};
+
+static const struct scsi_host_template ufshcd_sht_nonblocking = {
+	UFSHCD_SHT_COMMON,
+	.queuecommand_may_block = false,
 };
 
 static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
@@ -10049,13 +10062,16 @@ static int ufshcd_set_dma_mask(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
+ * __ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
  * @dev: pointer to device handle
  * @hba_handle: driver private handle
+ * @vops: pointer to variant specific operations
  * Returns 0 on success, non-zero value on failure
  */
-int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
+int __ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle,
+			const struct ufs_hba_variant_ops *vops)
 {
+	const struct scsi_host_template *sht;
 	struct Scsi_Host *host;
 	struct ufs_hba *hba;
 	int err = 0;
@@ -10067,8 +10083,12 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 		goto out_error;
 	}
 
-	host = scsi_host_alloc(&ufshcd_driver_template,
-				sizeof(struct ufs_hba));
+	if (vops && vops->may_block)
+		sht = &ufshcd_sht_blocking;
+	else
+		sht = &ufshcd_sht_nonblocking;
+
+	host = scsi_host_alloc(sht, sizeof(struct ufs_hba));
 	if (!host) {
 		dev_err(dev, "scsi_host_alloc failed\n");
 		err = -ENOMEM;
@@ -10078,6 +10098,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 	hba = shost_priv(host);
 	hba->host = host;
 	hba->dev = dev;
+	hba->vops = vops;
 	hba->dev_ref_clk_freq = REF_CLK_FREQ_INVAL;
 	hba->nop_out_timeout = NOP_OUT_TIMEOUT;
 	ufshcd_set_sg_entry_size(hba, sizeof(struct ufshcd_sg_entry));
@@ -10089,7 +10110,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 out_error:
 	return err;
 }
-EXPORT_SYMBOL(ufshcd_alloc_host);
+EXPORT_SYMBOL(__ufshcd_alloc_host);
 
 /* This function exists because blk_mq_alloc_tag_set() requires this. */
 static blk_status_t ufshcd_queue_tmf(struct blk_mq_hw_ctx *hctx,
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 8039c2b72502..dbeb879b47d1 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -274,6 +274,7 @@ struct ufs_pwr_mode_info {
 /**
  * struct ufs_hba_variant_ops - variant specific callbacks
  * @name: variant name
+ * @may_block: queuecommand may block (required with UFSHCD_CAP_CLK_GATING)
  * @init: called when the driver is initialized
  * @exit: called to cleanup everything done in init
  * @get_ufs_hci_version: called to get UFS HCI version
@@ -310,6 +311,7 @@ struct ufs_pwr_mode_info {
  */
 struct ufs_hba_variant_ops {
 	const char *name;
+	bool	may_block;
 	int	(*init)(struct ufs_hba *);
 	void    (*exit)(struct ufs_hba *);
 	u32	(*get_ufs_hci_version)(struct ufs_hba *);
@@ -1225,7 +1227,20 @@ static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
 	ufshcd_writel(hba, tmp, reg);
 }
 
-int ufshcd_alloc_host(struct device *, struct ufs_hba **);
+int __ufshcd_alloc_host(struct device *, struct ufs_hba **,
+			const struct ufs_hba_variant_ops *);
+
+/**
+ * ufshcd_alloc_host - allocate Host Bus Adapter (HBA)
+ * @dev: pointer to device handle
+ * @hba_handle: driver private handle
+ * Returns 0 on success, non-zero value on failure
+ */
+static inline int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
+{
+	return __ufshcd_alloc_host(dev, hba_handle, NULL);
+}
+
 void ufshcd_dealloc_host(struct ufs_hba *);
 int ufshcd_hba_enable(struct ufs_hba *hba);
 int ufshcd_init(struct ufs_hba *, void __iomem *, unsigned int);