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

[Rong Zhang]

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