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

[Maurizio Lombardi]

Hello Christoph,

On Mon Feb 2, 2026 at 8:17 AM CET, Christoph Hellwig wrote:
>
> Nit: this commit message reads a little odd because line lengths are so
> inconsistent, please use up all 73/75 chracaters consistently.
>
>
>> +			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.

Ok

>
>> +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.

Changed it to used the passed value

>
>> +	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?

That's right, I cleaned it up

>
>> +	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.

Ok

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

I tried but IMO doesn't make it much 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 ~.

Should all be fixed in V2, please take a look and let me know


Thanks,
Maurizio