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] [thread-next>] [day] [month] [year] [list]

Message-ID: <408B48A84E5811C4+555ae002-5b34-4f90-9452-a3458a11f10f@uniontech.com>
Date: Mon, 15 Sep 2025 15:43:05 +0800
From: Cryolitia PukNgae <cryolitia@...ontech.com>
To: Takashi Iwai <tiwai@...e.de>
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 12/09/2025 23.09, Takashi Iwai wrote:
> 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.
> 

Thanks for your review. To be honest, I haven't understand how
`static unsigned int quirk_flags[SNDRV_CARDS]` works. e.g., based on the
current array form, how to pass a flag, and what does the index of the
array means.

Could you please explain it for me. thx.

> Takashi
> 

Cryolitia PukNgae