linux-kernel - Re: [PATCH v2 0/3] ALSA: usb-audio: add module param device_quirk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87a52zr9kq.wl-tiwai@suse.de>
Date: Fri, 12 Sep 2025 17:09:57 +0200
From: Takashi Iwai <tiwai@...e.de>
To: cryolitia@...ontech.com
Cc: Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	Jonathan Corbet <corbet@....net>,
	linux-sound@...r.kernel.org,
	linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org,
	Mingcong Bai <jeffbai@...c.io>,
	Kexy Biscuit <kexybiscuit@...c.io>,
	Nie Cheng <niecheng1@...ontech.com>,
	Zhan Jun <zhanjun@...ontech.com>,
	Feng Yuan <fengyuan@...ontech.com>,
	qaqland <anguoli@...ontech.com>,
	kernel@...ontech.com
Subject: Re: [PATCH v2 0/3] ALSA: usb-audio: add module param device_quirk_flags

On Fri, 12 Sep 2025 08:48:57 +0200,
Cryolitia PukNgae via B4 Relay wrote:
> 
> As an implementation of what has been discussed previously[1].
> 
> > An open question is whether we may want yet a new module option or
> > rather extend the existing quirk option to accept the strings
> > instead.  Basically, when the given argument has a colon, it's a new
> > syntax.  If it's only a number, it's an old syntax, and parse like
> > before.  But, I'm open for either way (a new option or extend the
> > existing one).
> 
> I would like to add a new param. The existed param
> `static unsigned int quirk_flags[SNDRV_CARDS]` seems to related to
> some sequence the card probed. To be honest, I havn't fully understood
> it. And it seems hard to improve it while keeping compatibility.
> 
> 1. https://lore.kernel.org/all/87h5xm5g7f.wl-tiwai@suse.de/
> 
> Signed-off-by: Cryolitia PukNgae <cryolitia@...ontech.com>
> ---
> Changes in v2:
> - Cleaned up some internal rebase confusion, sorry for that
> - Link to v1: https://lore.kernel.org/r/20250912-sound-v1-0-cc9cfd9f2d01@uniontech.com
> 
> ---
> Cryolitia PukNgae (3):
>       ALSA: usb-audio: add two-way convert between name and bit for QUIRK_FLAG_*
>       ALSA: usb-audio: add module param device_quirk_flags
>       ALSA: doc: add docs about device_device_quirk_flags in snd-usb-audio

Well, what I had in mind is something like:

--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -73,7 +73,7 @@ static bool lowlatency = true;
 static char *quirk_alias[SNDRV_CARDS];
 static char *delayed_register[SNDRV_CARDS];
 static bool implicit_fb[SNDRV_CARDS];
-static unsigned int quirk_flags[SNDRV_CARDS];
+static char *quirk_flags[SNDRV_CARDS];
 
 bool snd_usb_use_vmalloc = true;
 bool snd_usb_skip_validation;
@@ -103,8 +103,8 @@ module_param_array(delayed_register, charp, NULL, 0444);
 MODULE_PARM_DESC(delayed_register, "Quirk for delayed registration, given by id:iface, e.g. 0123abcd:4.");
 module_param_array(implicit_fb, bool, NULL, 0444);
 MODULE_PARM_DESC(implicit_fb, "Apply generic implicit feedback sync mode.");
-module_param_array(quirk_flags, uint, NULL, 0444);
-MODULE_PARM_DESC(quirk_flags, "Driver quirk bit flags.");
+module_param_array(quirk_flags, charp, NULL, 0444);
+MODULE_PARM_DESC(quirk_flags, "Driver quirk overrides.");
 module_param_named(use_vmalloc, snd_usb_use_vmalloc, bool, 0444);
 MODULE_PARM_DESC(use_vmalloc, "Use vmalloc for PCM intermediate buffers (default: yes).");
 module_param_named(skip_validation, snd_usb_skip_validation, bool, 0444);
@@ -692,6 +692,22 @@ static void usb_audio_make_longname(struct usb_device *dev,
 	}
 }
 
+static void set_quirk_flags(struct snd_usb_audio *chip, int idx)
+{
+	int i;
+
+	/* old style option found: the position-based integer value */
+	if (quirk_flags[idx] &&
+	    !kstrtou32(quirk_flags[idx], 0, &chip->quirk_flags))
+		return;
+
+	/* take the default quirk from the quirk table */
+	snd_usb_init_quirk_flags(chip);
+	/* add or correct quirk bits from options */
+	for (i = 0; i < ARRAY_SIZE(quirk_flags); i++)
+		snd_usb_apply_quirk_option(chip, quirk_flags[i]);
+}
+
 /*
  * create a chip instance and set its names.
  */
@@ -750,10 +766,7 @@ static int snd_usb_audio_create(struct usb_interface *intf,
 	INIT_LIST_HEAD(&chip->midi_v2_list);
 	INIT_LIST_HEAD(&chip->mixer_list);
 
-	if (quirk_flags[idx])
-		chip->quirk_flags = quirk_flags[idx];
-	else
-		snd_usb_init_quirk_flags(chip);
+	set_quirk_flags(chip, idx);
 
 	card->private_free = snd_usb_audio_free;
 
.... and snd_usb_apply_quirk_option() adds or corrects the quirk bits
based on the string value if it matches with the probed device.
This function will be similar like your parser.

In that way, the old quirk_flags options work as-is, while you can use
a new style by passing values with "X:Y:Z" style.


Takashi