[RFC PATCH 0/1] enable quirks via module parameter

[RFC PATCH 0/1] enable quirks via module parameter

[Maurizio Lombardi]

On some occasions, our customers have complained about misbehaving
NVMe devices. We had to compile and provide them
with a test kernel just to see if a quirk could resolve the issue.

What do you think about allowing users to enable or disable a
quirk via a module parameter, similar to how usbcore does it?

This patch has undergone limited testing, but you get the idea.
Let me know if there's any interest and eventually I can
prepare a formal patch.

Maurizio Lombardi (1):
  nvme: add support for dynamic quirk configuration via module parameter

 drivers/nvme/host/pci.c | 201 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

-- 
2.43.5

[RFC PATCH 1/1] nvme: add support for dynamic quirk configuration via module parameter

[Maurizio Lombardi]

Introduce support for enabling or disabling specific NVMe quirks at
module load time through the `quirks` module parameter.
This mechanism allows users to apply known quirks dynamically
based on the device's PCI vendor and device IDs,
without requiring to add hardcoded entries in the driver
and recompiling the kernel.

The `quirks` parameter accepts a comma-separated list of quirk
specifications in the following format:

    <vendorID>:<deviceID>:<quirk_flags>[,<vendorID>:<deviceID>:<quirk_flags>,..]

Each quirk flag is represented by a single character, and can be prefixed with
`!` to indicate that the quirk should be disabled.

Example usage (enable BOGUS_NID and BROKEN_MSI, disable DEALLOCATE_ZEROES):

    $ modprobe nvme quirks=7170:2210:sv!c

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/nvme/host/pci.c | 201 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 2e30e9be7408..77e21089897c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -45,6 +45,13 @@
 #define NVME_MAX_META_SEGS 15
 #define NVME_MAX_NR_ALLOCATIONS	5
 
+struct quirk_entry {
+	u16 vendor_id;
+	u16 dev_id;
+	u32 enabled_quirks;
+	u32 disabled_quirks;
+};
+
 static int use_threaded_interrupts;
 module_param(use_threaded_interrupts, int, 0444);
 
@@ -75,6 +82,179 @@ static unsigned int io_queue_depth = 1024;
 module_param_cb(io_queue_depth, &io_queue_depth_ops, &io_queue_depth, 0644);
 MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2 and < 4096");
 
+static int quirks_param_set(const char *value, const struct kernel_param *kp);
+static char quirks_param[128];
+static const struct kernel_param_ops quirks_param_ops = {
+	.set = quirks_param_set,
+	.get = param_get_string,
+};
+
+static struct kparam_string quirks_param_string = {
+	.maxlen = sizeof(quirks_param),
+	.string = quirks_param,
+};
+
+static struct quirk_entry *quirk_list;
+static unsigned int quirk_count;
+module_param_cb(quirks, &quirks_param_ops, &quirks_param_string, 0644);
+MODULE_PARM_DESC(quirks, "Enable/disable NVMe quirks by specifying quirks=vendorID:deviceID:quirks");
+
+static int quirks_param_set(const char *value, const struct kernel_param *kp)
+{
+	char *val, *p, *field;
+	u16 vid, did;
+	u32 *flags;
+	size_t i;
+	int err;
+	bool neg = false;
+
+	val = kstrdup(value, GFP_KERNEL);
+	if (!val)
+		return -ENOMEM;
+
+	err = param_set_copystring(val, kp);
+	if (err)
+		goto exit;
+
+	kfree(quirk_list);
+	quirk_list = NULL;
+
+	if (!*val) {
+		quirk_count = 0;
+		goto exit;
+	}
+
+	for (quirk_count = 1, i = 0; val[i]; i++)
+		if (val[i] == ',')
+			quirk_count++;
+
+	quirk_list = kcalloc(quirk_count, sizeof(struct quirk_entry),
+			     GFP_KERNEL);
+	if (!quirk_list) {
+		quirk_count = 0;
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	for (i = 0, p = val; p && *p; ++i) {
+		/* Each entry consists of VID:DID:flags */
+		field = strsep(&p, ":");
+		if (!field)
+			break;
+
+		if (kstrtou16(field, 16, &vid))
+			break;
+
+		field = strsep(&p, ":");
+		if (!field)
+			break;
+
+		if (kstrtou16(field, 16, &did))
+			break;
+
+		field = strsep(&p, ",");
+		if (!field || !*field)
+			break;
+
+		/* Collect the flags */
+		for (flags = 0; *field; field++) {
+			if (neg) {
+				flags = &quirk_list[i].disabled_quirks;
+				neg = false;
+			} else {
+				flags = &quirk_list[i].enabled_quirks;
+			}
+			switch (*field) {
+			case 'a':
+				*flags |= NVME_QUIRK_STRIPE_SIZE;
+				break;
+			case 'b':
+				*flags |= NVME_QUIRK_IDENTIFY_CNS;
+				break;
+			case 'c':
+				*flags |= NVME_QUIRK_DEALLOCATE_ZEROES;
+				break;
+			case 'd':
+				*flags |= NVME_QUIRK_DELAY_BEFORE_CHK_RDY;
+				break;
+			case 'e':
+				*flags |= NVME_QUIRK_NO_APST;
+				break;
+			case 'f':
+				*flags |= NVME_QUIRK_NO_DEEPEST_PS;
+				break;
+			case 'g':
+				*flags |= NVME_QUIRK_QDEPTH_ONE;
+				break;
+			case 'h':
+				*flags |= NVME_QUIRK_MEDIUM_PRIO_SQ;
+				break;
+			case 'i':
+				*flags |= NVME_QUIRK_IGNORE_DEV_SUBNQN;
+				break;
+			case 'j':
+				*flags |= NVME_QUIRK_DISABLE_WRITE_ZEROES;
+				break;
+			case 'k':
+				*flags |= NVME_QUIRK_SIMPLE_SUSPEND;
+				break;
+			case 'l':
+				*flags |= NVME_QUIRK_SINGLE_VECTOR;
+				break;
+			case 'm':
+				*flags |= NVME_QUIRK_128_BYTES_SQES;
+				break;
+			case 'n':
+				*flags |= NVME_QUIRK_SHARED_TAGS;
+				break;
+			case 'o':
+				*flags |= NVME_QUIRK_NO_TEMP_THRESH_CHANGE;
+				break;
+			case 'p':
+				*flags |= NVME_QUIRK_NO_NS_DESC_LIST;
+				break;
+			case 'q':
+				*flags |= NVME_QUIRK_DMA_ADDRESS_BITS_48;
+				break;
+			case 'r':
+				*flags |= NVME_QUIRK_SKIP_CID_GEN;
+				break;
+			case 's':
+				*flags |= NVME_QUIRK_BOGUS_NID;
+				break;
+			case 't':
+				*flags |= NVME_QUIRK_NO_SECONDARY_TEMP_THRESH;
+				break;
+			case 'u':
+				*flags |= NVME_QUIRK_FORCE_NO_SIMPLE_SUSPEND;
+				break;
+			case 'v':
+				*flags |= NVME_QUIRK_BROKEN_MSI;
+				break;
+			case 'z':
+				*flags |= NVME_QUIRK_DMAPOOL_ALIGN_512;
+				break;
+			case '!':
+				neg = true;
+				break;
+			default:
+				/* Ignore unrecognized flag characters */
+				break;
+			}
+		}
+
+		quirk_list[i].vendor_id = vid;
+		quirk_list[i].dev_id = did;
+	}
+
+	if (i < quirk_count)
+		quirk_count = i;
+
+exit:
+	kfree(val);
+	return err;
+}
+
 static int io_queue_count_set(const char *val, const struct kernel_param *kp)
 {
 	unsigned int n;
@@ -3175,12 +3355,27 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 	return 0;
 }
 
+static struct quirk_entry *detect_dynamic_quirks(struct pci_dev *pdev)
+{
+	int i;
+
+	for (i = 0; i < quirk_count; ++i) {
+		if (pdev->vendor == quirk_list[i].vendor_id &&
+		    pdev->device == quirk_list[i].dev_id) {
+			return &quirk_list[i];
+		}
+	}
+
+	return NULL;
+}
+
 static struct nvme_dev *nvme_pci_alloc_dev(struct pci_dev *pdev,
 		const struct pci_device_id *id)
 {
 	unsigned long quirks = id->driver_data;
 	int node = dev_to_node(&pdev->dev);
 	struct nvme_dev *dev;
+	struct quirk_entry *qentry;
 	int ret = -ENOMEM;
 
 	dev = kzalloc_node(sizeof(*dev), GFP_KERNEL, node);
@@ -3211,6 +3406,11 @@ static struct nvme_dev *nvme_pci_alloc_dev(struct pci_dev *pdev,
 			 "platform quirk: setting simple suspend\n");
 		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
 	}
+	qentry = detect_dynamic_quirks(pdev);
+	if (qentry) {
+		quirks |= qentry->enabled_quirks;
+		quirks &= ~(qentry->disabled_quirks);
+	}
 	ret = nvme_init_ctrl(&dev->ctrl, &pdev->dev, &nvme_pci_ctrl_ops,
 			     quirks);
 	if (ret)
@@ -3814,6 +4014,7 @@ static int __init nvme_init(void)
 
 static void __exit nvme_exit(void)
 {
+	kfree(quirk_list);
 	pci_unregister_driver(&nvme_driver);
 	flush_workqueue(nvme_wq);
 }
-- 
2.43.5

Re: [RFC PATCH 0/1] enable quirks via module parameter

[Keith Busch]

On Fri, May 16, 2025 at 05:50:24PM +0200, Maurizio Lombardi wrote:
> On some occasions, our customers have complained about misbehaving
> NVMe devices. We had to compile and provide them
> with a test kernel just to see if a quirk could resolve the issue.
> 
> What do you think about allowing users to enable or disable a
> quirk via a module parameter, similar to how usbcore does it?

Can't we already do this for pci using the "new_id" attribute of the
nvme-pci driver? It takes up to 7 parameters, the last one being the
"driver_data", which is what we use to setup the quirks, and you can set
that value to anything you want through that interface.

Re: [RFC PATCH 0/1] enable quirks via module parameter

[Keith Busch]

On Fri, May 16, 2025 at 09:59:10AM -0600, Keith Busch wrote:
> On Fri, May 16, 2025 at 05:50:24PM +0200, Maurizio Lombardi wrote:
> > On some occasions, our customers have complained about misbehaving
> > NVMe devices. We had to compile and provide them
> > with a test kernel just to see if a quirk could resolve the issue.
> > 
> > What do you think about allowing users to enable or disable a
> > quirk via a module parameter, similar to how usbcore does it?
> 
> Can't we already do this for pci using the "new_id" attribute of the
> nvme-pci driver? It takes up to 7 parameters, the last one being the
> "driver_data", which is what we use to setup the quirks, and you can set
> that value to anything you want through that interface.

Oops, maybe not "anything you want", but you can set it to any value
that exists in the driver's table. That may be an unnecessary limitation
in the new_id_store() function, but let's see if that's sufficient
before considering changing that.

Re: [RFC PATCH 0/1] enable quirks via module parameter

[Maurizio Lombardi]

On Fri May 16, 2025 at 6:22 PM CEST, Keith Busch wrote:
>> Can't we already do this for pci using the "new_id" attribute of the
>> nvme-pci driver? It takes up to 7 parameters, the last one being the
>> "driver_data", which is what we use to setup the quirks, and you can set
>> that value to anything you want through that interface.
>
> Oops, maybe not "anything you want", but you can set it to any value
> that exists in the driver's table. That may be an unnecessary limitation
> in the new_id_store() function, but let's see if that's sufficient
> before considering changing that.

Thanks for the suggestion, I really hadn't considered that.

So the idea would be to unbind the device and re-bind
it while specifying a different driver_data, correct?

If that's the case, I do have a concern: how can I do that
if the root filesystem is on the device that needs to be unbound?

Maurizio