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

[Christoph Hellwig <hch@lst.de>]

On Wed, Jan 28, 2026 at 11:33:18AM +0100, Maurizio Lombardi wrote:
> 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.

Nit: this commit message reads a little odd because line lengths are so
inconsistent, please use up all 73/75 chracaters consistently.

> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt

I thought those file only used to document always built-in paramters
using __setup and not module parameters, but grabbing random ones that
look modular disproves that.

> +			list of quirk names separated by commas. A quirk name can
> +			be prefixed by '^', meaning that the specified quirk must

Overly long lines.

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

Can we get away without a forware declaration here?

> +static struct quirk_entry *quirk_list;
> +static unsigned int quirk_count;

Please use nvme_pci_ prefixes for global variables and functions.

> +static int quirks_param_set(const char *value, const struct kernel_param *kp)
> +{
> +	char *val, *outer_ptr, *inner_ptr, *field;
> +	char *q_name;
> +	u16 vid, did;
> +	u32 *flags;
> +	size_t i = 0, q_len, field_len;
> +	int err, b;
> +
> +	val = kstrdup(value, GFP_KERNEL);
> +	if (!val)
> +		return -ENOMEM;
> +
> +	err = param_set_copystring(val, kp);
> +	if (err)
> +		goto exit;

All other callers of param_set_copystring except for usbhid set
it to the passsed in value and not a copy.  Given that this
function has no documentation I'm not sure what is right, but
this smells a bit fishy.

> +	kfree(quirk_list);
> +	quirk_list = NULL;

How can quirk_list be non-NULL here given that we don't allow
runtime-changes to the paramter?

> +	if (!*val) {
> +		quirk_count = 0;
> +		goto exit;
> +	}

> +	for (quirk_count = 1, i = 0; val[i]; i++)
> +		if (val[i] == '-')
> +			quirk_count++;

initializing the quirk_count in the loop looks odd, compare to:

	quirk_count = 1;
	for (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;
> +	}

Instead of zeroing the global variable on error, please only set it
from a local one on success to make the code more robust.

> +	i = 0;
> +	outer_ptr = val;
> +	while ((field = strsep(&outer_ptr, "-"))) {
> +		inner_ptr = field;
> +
> +		/* Each entry consists of VID:DID:quirk_names */
> +		field = strsep(&inner_ptr, ":");

This juggling between field and innter_ptr looks odd.  I'd scan directly
into innter_ptr and keep field declarated locally in the loop.  Same
for any other variable not used globally.  Also splitting out the
loop bodies into well named helpers would help readability a lot here.

> +			field_len = strlen(field);
> +			for (b = 0; ; ++b) {
> +				q_name = nvme_quirk_name(BIT(b));

Urrg.  Please avoid use of BIT in general, but especially here where
you're actually doing arithmetics amd just declare constants.

Given that we're unlikely to add tons of quirk lines, simply reallocing
the array vs pre-sizing it might also be simpler.

> +static struct quirk_entry *detect_dynamic_quirks(struct pci_dev *pdev)
> +{
> +	int i;
> +
> +	for (i = 0; i < quirk_count; ++i) {

The nvme driver like most of the kernel uses post-increments in for
loops.  (We already had at least one more instance above).

> +		if (pdev->vendor == quirk_list[i].vendor_id &&
> +		    pdev->device == quirk_list[i].dev_id) {
> +			return &quirk_list[i];
> +		}
> +	}

No need for either braces, and at least the inner ones are counter to
the usual kernel style.

> +	qentry = detect_dynamic_quirks(pdev);
> +	if (qentry) {
> +		quirks |= qentry->enabled_quirks;
> +		quirks &= ~(qentry->disabled_quirks);

No need for the braces after the ~.