| 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
| ||
|
Message-ID: <877by0p28h.wl-tiwai@suse.de>
Date: Mon, 15 Sep 2025 10:08:14 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Cryolitia PukNgae <cryolitia@...ontech.com>
Cc: Takashi Iwai <tiwai@...e.de>,
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 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@...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.
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
Powered by blists - more mailing lists