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

[Takashi Iwai]

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).

> +	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.

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;
}

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;			
		}


thanks,

Takashi