Re: [PATCH 4/9] ALSA: usb-audio: Support string-descriptor-based quirk table entry

Rong Zhang
2026-03-02T12:36:54Z

Hi Takashi,

Thanks for your review.

On Mon, 2026-03-02 at 10:54 +0100, Takashi Iwai wrote:
> On Sun, 01 Mar 2026 22:37:20 +0100,
> Rong Zhang wrote:
> > @@ -2558,14 +2585,69 @@ void snd_usb_apply_flag_dbg(const char *reason,
> > }
> > }
> >
> > +#define USB_STRING_SIZE 128
> > +
> > +static inline int snd_usb_get_strings(struct snd_usb_audio *chip,
> > + char *manufacturer, char *product)
> > +{
> > + int ret;
> > +
> > + if (manufacturer) {
> > + ret = usb_string(chip->dev, chip->dev->descriptor.iManufacturer,
> > + manufacturer, USB_STRING_SIZE);
> > + if (ret < 0) {
> > + usb_audio_warn(chip, "failed to get manufacturer string: %d\n", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + if (product) {
> > + ret = usb_string(chip->dev, chip->dev->descriptor.iProduct,
> > + product, USB_STRING_SIZE);
> > + if (ret < 0) {
> > + usb_audio_warn(chip, "failed to get product string: %d\n", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + return 1; /* ok */
> > +}
> > +
> > void snd_usb_init_quirk_flags_table(struct snd_usb_audio *chip)
> > {
> > const struct usb_audio_quirk_flags_table *p;
> > + char manufacturer[USB_STRING_SIZE];
> > + char product[USB_STRING_SIZE];
>
> Keeping those a bit largish strings on the stack doesn't look good.
> Maybe better to do kalloc with __free(kfree).

Makes sense.

> > + int got_usb_strings = 0; /* <0: error, 0: pending, >0: ok */
>
> I think this global flag could go wrong since...
>
> > for (p = quirk_flags_table; p->id; p++) {
> > if (chip->usb_id == p->id ||
> > (!USB_ID_PRODUCT(p->id) &&
> > USB_ID_VENDOR(chip->usb_id) == USB_ID_VENDOR(p->id))) {
> > + if (!p->usb_string_match)
> > + goto apply; /* DEVICE_FLG or VENDOR_FLG */
> > +
> > + /* DEVICE_STRING_FLG or VENDOR_STRING_FLG */
> > + if (!got_usb_strings) {
> > + got_usb_strings = snd_usb_get_strings(chip,
> > + p->usb_string_match->manufacturer ? manufacturer : NULL,
> > + p->usb_string_match->product ? product : NULL);
>
> ... here you try to extract strings only once, but this retrieval
> depends on the current entry. If the current entry has only
> manufacturer and the next entry has only product, it won't work.

Nice catch! Thanks. Will fix it in v2.

> IMO, a simpler way would be something like:
>
> #define USB_STRING_SIZE 128
>
> static char *snd_usb_get_string(struct snd_usb_audio *chip, int id)
> {
> char *buf = kmalloc(USB_STRING_SIZE, GFP_KERNEL);
> int ret;
>
> if (!buf)
> return ERR_PTR(-ENOMEM);
> ret = usb_string(chip->dev, id, buf, USB_STRING_SIZE);
> if (ret < 0) {
> usb_audio_warn(chip, "failed to get string for id%d: %d\n", ret);
> kfree(buf);
> return ERR_PTR(ret);
> }
> return buf;
> }

I guess we may use usb_cache_string() so that we don't need to
kmalloc() the buffer ourselves and can get rid of USB_STRING_SIZE.

> void snd_usb_init_quirk_flags_table(struct snd_usb_audio *chip)
> {
> ....
> char *manufacturer __free(kfree) = NULL;
> char *product __free(kfree) = NULL;
>
> for (p = quirk_flags_table; p->id; p++) {
> ....
> if (p->usb_string_match->manufacturer) {
> if (!manufacturer)
> manufaturer =
> snd_usb_get_string(chip,
> chip->dev->descriptor.iManufacturer);
> if (!IS_ERR_OR_NULL(manufacturer) &&
> strcmp(p->usb_string_match->manufacturer, manufacturer))
> continue;
> }
> if (p->usb_string_match->product) {
> if (!product)
> product =
> snd_usb_get_string(chip,
> chip->dev->descriptor.iProduct);
> if (!IS_ERR_OR_NULL(product) &&
> strcmp(p->usb_string_match->product, product))
> continue;
> }
>
>

Sounds neat. Will do in v2. I will also skip the warning if id == 0
since string-descriptor-based quirk table entries has nothing to do
with devices without corresponding descriptors.

Thanks,
Rong

> thanks,
>
> Takashi