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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 15 Sep 2025 09:43:05 +0200,
Cryolitia PukNgae wrote:
> 
> 
> 
> 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@uniontech.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.

That option works just like other options of the card arrays -- each
value is passed sequentially to the device of the given probe slot.
That is, the first probed device takes quirk_flags[0], the second
probed device takes quirk_flats[1], and so on.
Admittedly, although this works fine for the static probe
configuration like PCI devices or such, it's not ideal with
USB-audio.  So the new format is requested.


Takashi