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.
What about something like this?
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 82321b8b32bc..301c0b2a406d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10049,13 +10049,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)
{
+ struct scsi_host_template sht = ufshcd_driver_template;
struct Scsi_Host *host;
struct ufs_hba *hba;
int err = 0;
@@ -10067,8 +10070,10 @@ 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->nonblocking)
+ sht.queuecommand_may_block = false;
+
+ host = scsi_host_alloc(&sht, sizeof(struct ufs_hba));
if (!host) {
dev_err(dev, "scsi_host_alloc failed\n");
err = -ENOMEM;
@@ -10078,6 +10083,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 +10095,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/drivers/ufs/host/ufshcd-pci.c b/drivers/ufs/host/ufshcd-pci.c
index 9c911787f84c..ce42b822fcb6 100644
--- a/drivers/ufs/host/ufshcd-pci.c
+++ b/drivers/ufs/host/ufshcd-pci.c
@@ -447,6 +447,7 @@ static int ufs_intel_mtl_init(struct ufs_hba *hba)
static struct ufs_hba_variant_ops ufs_intel_cnl_hba_vops = {
.name = "intel-pci",
+ .nonblocking = true,
.init = ufs_intel_common_init,
.exit = ufs_intel_common_exit,
.link_startup_notify = ufs_intel_link_startup_notify,
@@ -455,6 +456,7 @@ static struct ufs_hba_variant_ops ufs_intel_cnl_hba_vops = {
static struct ufs_hba_variant_ops ufs_intel_ehl_hba_vops = {
.name = "intel-pci",
+ .nonblocking = true,
.init = ufs_intel_ehl_init,
.exit = ufs_intel_common_exit,
.link_startup_notify = ufs_intel_link_startup_notify,
@@ -463,6 +465,7 @@ static struct ufs_hba_variant_ops ufs_intel_ehl_hba_vops = {
static struct ufs_hba_variant_ops ufs_intel_lkf_hba_vops = {
.name = "intel-pci",
+ .nonblocking = true,
.init = ufs_intel_lkf_init,
.exit = ufs_intel_common_exit,
.hce_enable_notify = ufs_intel_hce_enable_notify,
@@ -475,6 +478,7 @@ static struct ufs_hba_variant_ops ufs_intel_lkf_hba_vops = {
static struct ufs_hba_variant_ops ufs_intel_adl_hba_vops = {
.name = "intel-pci",
+ .nonblocking = true,
.init = ufs_intel_adl_init,
.exit = ufs_intel_common_exit,
.link_startup_notify = ufs_intel_link_startup_notify,
@@ -484,6 +488,7 @@ static struct ufs_hba_variant_ops ufs_intel_adl_hba_vops = {
static struct ufs_hba_variant_ops ufs_intel_mtl_hba_vops = {
.name = "intel-pci",
+ .nonblocking = true,
.init = ufs_intel_mtl_init,
.exit = ufs_intel_common_exit,
.hce_enable_notify = ufs_intel_hce_enable_notify,
@@ -538,6 +543,7 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
static int
ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
+ const struct ufs_hba_variant_ops *vops;
struct ufs_host *ufs_host;
struct ufs_hba *hba;
void __iomem *mmio_base;
@@ -559,14 +565,14 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
mmio_base = pcim_iomap_table(pdev)[0];
- err = ufshcd_alloc_host(&pdev->dev, &hba);
+ vops = (const struct ufs_hba_variant_ops *)id->driver_data;
+
+ err = __ufshcd_alloc_host(&pdev->dev, &hba, vops);
if (err) {
dev_err(&pdev->dev, "Allocation failed\n");
return err;
}
- hba->vops = (struct ufs_hba_variant_ops *)id->driver_data;
-
err = ufshcd_init(hba, mmio_base, pdev->irq);
if (err) {
dev_err(&pdev->dev, "Initialization failed\n");
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 8039c2b72502..dbf2312ad756 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -274,6 +274,8 @@ struct ufs_pwr_mode_info {
/**
* struct ufs_hba_variant_ops - variant specific callbacks
* @name: variant name
+ * @nonblocking: queuecommand will not block (incompatible 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 +312,7 @@ struct ufs_pwr_mode_info {
*/
struct ufs_hba_variant_ops {
const char *name;
+ bool nonblocking;
int (*init)(struct ufs_hba *);
void (*exit)(struct ufs_hba *);
u32 (*get_ufs_hci_version)(struct ufs_hba *);
@@ -1225,7 +1228,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);
