[PATCH V2 0/1] Enable/Disable NVMe quirks via module parameter

[PATCH V2 0/1] Enable/Disable NVMe quirks via module parameter

[Maurizio Lombardi]

Recently, some of our customers were very vocal about wanting an
easy way to enable or disable specific nvme quirks.

I recall that during the ALPSS conference this topic has been briefly
discussed, and it was proposed to use the generic PCI sysfs "new_id" entry
to manipulate the quirks.

However, in some cases a quirk may prevent the system from properly booting,
the kernel may hang or fail to mount the root
filesystem before the system reaches a state where new_id can be written,
this makes this solution not exactly user-friendly for end users.

In particular, boot-time hangs have been reported involving the
bogus_nid quirk.

Therefore, I have been asked to re-propose this module parameter approach
with a few modifications suggested by Daniel Wagner.


# dmesg | grep nvme
[    0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 nvme.quirks=1b36:0010:^bogus_nxid
[    6.732692] nvme: unrecognized quirk bogus_nxid

# cat /sys/devices/pci0000:00/0000:00:05.0/nvme/nvme1/quirks
bogus_nid
                                                                                                                                                                                                                         
# dmesg | grep nvme
[    0.000000] Command line: root=/dev/vda1 console=ttyS0,115200n8 nvme.quirks=1b36:0010:^bogus_nid,no_apst
                                                                                                                                                                                                                         
# cat /sys/devices/pci0000:00/0000:00:05.0/nvme/nvme0/quirks                                                                                                                                             
no_apst

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

 .../admin-guide/kernel-parameters.txt         |  13 ++
 drivers/nvme/host/pci.c                       | 164 ++++++++++++++++++
 2 files changed, 177 insertions(+)

-- 
2.52.0

[PATCH V2 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.

While the generic PCI new_id sysfs interface exists for dynamic
configuration, it is insufficient for scenarios where the system fails
to boot (for example, this has been reported to happen because of the
bogus_nid quirk). The new_id attribute is writable only after the system
has booted and sysfs is mounted.

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

<VID>:<DID>:<quirk_names>[-<VID>:<DID>:<quirk_names>-..]

Each quirk is represented by its name and can be prefixed with `^` to
indicate that the quirk should be disabled; quirk names are separated by
a ',' character.

Example: enable BOGUS_NID and BROKEN_MSI, disable DEALLOCATE_ZEROES:

   $ modprobe nvme quirks=7170:2210:bogus_nid,broken_msi,^deallocate_zeroes

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
Tested-by: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---

V2: split quirk_param_set() to increase readability.
    Prefer post-increments.
    Remove braces where it makes sense.
    Avoid the BIT() macro.
    Use nvme_pci_ prefix for global variables.
    Break overly long lines, use 75-chars length consistently
    set 0444 perms to quirks param to prevent the user from changing it

 .../admin-guide/kernel-parameters.txt         |  13 ++
 drivers/nvme/host/pci.c                       | 164 ++++++++++++++++++
 2 files changed, 177 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1058f2a6d6a8..cb689fcfbc6f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -74,6 +74,7 @@
 	TPM	TPM drivers are enabled.
 	UMS	USB Mass Storage support is enabled.
 	USB	USB support is enabled.
+	NVME	NVMe support is enabled
 	USBHID	USB Human Interface Device support is enabled.
 	V4L	Video For Linux support is enabled.
 	VGA	The VGA console has been enabled.
@@ -4706,6 +4707,18 @@ Kernel parameters
 			This can be set from sysctl after boot.
 			See Documentation/admin-guide/sysctl/vm.rst for details.
 
+	nvme.quirks=    [NVME] A list of quirk entries to augment the built-in
+			nvme quirk list. List entries are separated by a
+			'-' character.
+			Each entry has the form VendorID:ProductID:quirk_names.
+			The IDs are 4-digits hex numbers and quirk_names is a
+			list of quirk names separated by commas. A quirk name
+			can be prefixed by '^', meaning that the specified
+			quirk must be disabled.
+
+			Example:
+			nvme.quirks=7710:2267:bogus_nid,^identify_cns-9900:7711:broken_msi
+
 	ohci1394_dma=early	[HW,EARLY] enable debugging via the ohci1394 driver.
 			See Documentation/core-api/debugging-via-ohci1394.rst for more
 			info.
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4bf3435c47a8..b20c540c970a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -72,6 +72,13 @@
 static_assert(MAX_PRP_RANGE / NVME_CTRL_PAGE_SIZE <=
 	(1 /* prp1 */ + NVME_MAX_NR_DESCRIPTORS * PRPS_PER_PAGE));
 
+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);
 
@@ -102,6 +109,144 @@ 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 struct quirk_entry *nvme_pci_quirk_list;
+static unsigned int nvme_pci_quirk_count;
+
+/* Helper to parse individual quirk names */
+static int nvme_parse_quirk_names(char *quirk_str, struct quirk_entry *entry)
+{
+	int i;
+	size_t field_len;
+	bool disabled, found;
+	char *p = quirk_str, *field;
+
+	while ((field = strsep(&p, ",")) && *field) {
+		disabled = false;
+		found = false;
+
+		if (*field == '^') {
+			/* Skip the '^' character */
+			disabled = true;
+			field++;
+		}
+
+		field_len = strlen(field);
+		for (i = 0; i < 32; i++) {
+			unsigned int bit = 1U << i;
+			char *q_name = nvme_quirk_name(bit);
+			size_t q_len = strlen(q_name);
+
+			if (!strcmp(q_name, "unknown"))
+				break;
+
+			if (!strcmp(q_name, field) &&
+				    q_len == field_len) {
+				if (disabled)
+					entry->disabled_quirks |= bit;
+				else
+					entry->enabled_quirks |= bit;
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			pr_err("nvme: unrecognized quirk %s\n", field);
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
+/* Helper to parse a single VID:DID:quirk_names entry */
+static int nvme_parse_quirk_entry(char *s, struct quirk_entry *entry)
+{
+	char *field;
+
+	field = strsep(&s, ":");
+	if (!field || kstrtou16(field, 16, &entry->vendor_id))
+		return -EINVAL;
+
+	field = strsep(&s, ":");
+	if (!field || kstrtou16(field, 16, &entry->dev_id))
+		return -EINVAL;
+
+	field = strsep(&s, ":");
+	if (!field)
+		return -EINVAL;
+
+	return nvme_parse_quirk_names(field, entry);
+}
+
+static int quirks_param_set(const char *value, const struct kernel_param *kp)
+{
+	int qcount, err, i;
+	struct quirk_entry *qlist= NULL;
+	char *field, *val = NULL;
+	char *sep_ptr;
+
+	err = param_set_copystring(value, kp);
+	if (err)
+		goto exit;
+
+	val = kstrdup(value, GFP_KERNEL);
+	if (!val) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	if (!*val)
+		goto exit;
+
+	qcount = 1;
+	for (i = 0; val[i]; i++) {
+		if (val[i] == '-')
+			qcount++;
+	}
+
+	qlist = kcalloc(qcount, sizeof(*qlist), GFP_KERNEL);
+	if (!qlist) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i = 0;
+	sep_ptr = val;
+	while ((field = strsep(&sep_ptr, "-"))) {
+		if (nvme_parse_quirk_entry(field, &qlist[i])) {
+			pr_err("nvme: failed to parse quirk string %s\n",
+								value);
+			goto exit;
+		}
+
+		i++;
+	}
+
+	nvme_pci_quirk_count = qcount;
+	nvme_pci_quirk_list  = qlist;
+	kfree(val);
+	return 0;
+exit:
+	kfree(val);
+	kfree(qlist);
+	return err;
+}
+
+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,
+};
+
+module_param_cb(quirks, &quirks_param_ops, &quirks_param_string, 0444);
+MODULE_PARM_DESC(quirks, "Enable/disable NVMe quirks by specifying "
+						"quirks=VID:DID:quirk_names");
+
 static int io_queue_count_set(const char *val, const struct kernel_param *kp)
 {
 	unsigned int n;
@@ -3442,12 +3587,25 @@ 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 < nvme_pci_quirk_count; i++)
+		if (pdev->vendor == nvme_pci_quirk_list[i].vendor_id &&
+		    pdev->device == nvme_pci_quirk_list[i].dev_id)
+			return &nvme_pci_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(struct_size(dev, descriptor_pools, nr_node_ids),
@@ -3479,6 +3637,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)
@@ -4079,6 +4242,7 @@ static int __init nvme_init(void)
 
 static void __exit nvme_exit(void)
 {
+	kfree(nvme_pci_quirk_list);
 	pci_unregister_driver(&nvme_driver);
 	flush_workqueue(nvme_wq);
 }
-- 
2.52.0

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

[Christoph Hellwig]

> +static int quirks_param_set(const char *value, const struct kernel_param *kp)
> +{
> +	int qcount, err, i;

qcount makes me thing of the queue count.  Given that this is the only
count, maybe just drop the q prefix?

> +	struct quirk_entry *qlist= NULL;

Missing space before the "=".

> +	char *field, *val = NULL;
> +	char *sep_ptr;
> +
> +	err = param_set_copystring(value, kp);
> +	if (err)
> +		goto exit;

Just return directly here.  Then you also don't need to initialize
val.

> +
> +	val = kstrdup(value, GFP_KERNEL);
> +	if (!val) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}

Also here.

> +
> +	if (!*val)
> +		goto exit;

and then use separate out_free_val and out_free_qlist labels
to avoid the need to initialize qlist.

> +		if (nvme_parse_quirk_entry(field, &qlist[i])) {
> +			pr_err("nvme: failed to parse quirk string %s\n",
> +								value);
> +			goto exit;

The indentation is a bit odd, usualy this would be a two-tab alignment
or after the opening brace.

> +
> +	nvme_pci_quirk_count = qcount;
> +	nvme_pci_quirk_list  = qlist;
> +	kfree(val);

And once you have the out_free_val label, it can be shared with
the regular return.

Otherwise this looks good, and the parsing is much more readable
being split up a bit like this.