Re: [PATCH] usb: gadget: f_midi: allow customizing the USB MIDI interface string through configfs

[Takashi Iwai <tiwai@suse.de>]

On Wed, 24 Sep 2025 17:48:21 +0200,
Victor Krawiec wrote:
> 
> When using f_midi from configfs the USB MIDI interface string is hardcoded
> to 'MIDI function'.
> 
> This USB string descriptor is used by some third-party OS or software to
> display the name of the MIDI device
> 
> Since we add an additional string option a new macro block was created to
> factorize declarations
> 
> Signed-off-by: Victor Krawiec <victor.krawiec@arturia.com>
> ---
>  drivers/usb/gadget/function/f_midi.c | 108 +++++++++++++++------------
>  drivers/usb/gadget/function/u_midi.h |   8 +-
>  2 files changed, 66 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index da82598fcef8..0a8af7d507d9 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -875,6 +875,7 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
>  	struct usb_composite_dev *cdev = c->cdev;
>  	struct f_midi *midi = func_to_midi(f);
>  	struct usb_string *us;
> +	struct f_midi_opts *opts;
>  	int status, n, jack = 1, i = 0, endpoint_descriptor_index = 0;
>  
>  	midi->gadget = cdev->gadget;
> @@ -883,6 +884,10 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
>  	if (status < 0)
>  		goto fail_register;
>  
> +	opts = container_of(f->fi, struct f_midi_opts, func_inst);
> +	if (opts->interface_string_allocated && opts->interface_string)
> +		midi_string_defs[STRING_FUNC_IDX].s = opts->interface_string;
> +
>  	/* maybe allocate device-global string ID */
>  	us = usb_gstrings_attach(c->cdev, midi_strings,
>  				 ARRAY_SIZE(midi_string_defs));
> @@ -1178,59 +1183,62 @@ end:									\
>  									\
>  CONFIGFS_ATTR(f_midi_opts_, name);
>  
> +#define F_MIDI_OPT_STRING(name)						\
> +static ssize_t f_midi_opts_##name##_show(struct config_item *item, char *page) \
> +{									\
> +	struct f_midi_opts *opts = to_f_midi_opts(item);		\
> +	ssize_t result;							\
> +									\
> +	mutex_lock(&opts->lock);					\
> +	if (opts->name) {						\
> +		result = strscpy(page, opts->name, PAGE_SIZE);		\
> +	} else {							\
> +		page[0] = 0;						\
> +		result = 0;						\
> +	}								\
> +									\
> +	mutex_unlock(&opts->lock);					\
> +									\
> +	return result;							\
> +}									\
> +									\
> +static ssize_t f_midi_opts_##name##_store(struct config_item *item,	\
> +					 const char *page, size_t len)	\
> +{									\
> +	struct f_midi_opts *opts = to_f_midi_opts(item);		\
> +	int ret;							\
> +	char *c;							\
> +									\
> +	mutex_lock(&opts->lock);					\
> +	if (opts->refcnt > 1) {						\
> +		ret = -EBUSY;						\
> +		goto end;						\
> +	}								\
> +									\
> +	c = kstrndup(page, len, GFP_KERNEL);				\
> +	if (!c) {							\
> +		ret = -ENOMEM;						\
> +		goto end;						\
> +	}								\
> +	if (opts->name##_allocated)					\
> +		kfree(opts->name);					\
> +	opts->name = c;							\
> +	opts->name##_allocated = true;					\
> +	ret = len;							\
> +end:									\
> +	mutex_unlock(&opts->lock);					\
> +	return ret;							\
> +}									\
> +									\
> +CONFIGFS_ATTR(f_midi_opts_, name);
> +
>  F_MIDI_OPT_SIGNED(index, true, SNDRV_CARDS);
>  F_MIDI_OPT(buflen, false, 0);
>  F_MIDI_OPT(qlen, false, 0);
>  F_MIDI_OPT(in_ports, true, MAX_PORTS);
>  F_MIDI_OPT(out_ports, true, MAX_PORTS);
> -
> -static ssize_t f_midi_opts_id_show(struct config_item *item, char *page)
> -{
> -	struct f_midi_opts *opts = to_f_midi_opts(item);
> -	ssize_t result;
> -
> -	mutex_lock(&opts->lock);
> -	if (opts->id) {
> -		result = strscpy(page, opts->id, PAGE_SIZE);
> -	} else {
> -		page[0] = 0;
> -		result = 0;
> -	}
> -
> -	mutex_unlock(&opts->lock);
> -
> -	return result;
> -}
> -
> -static ssize_t f_midi_opts_id_store(struct config_item *item,
> -				    const char *page, size_t len)
> -{
> -	struct f_midi_opts *opts = to_f_midi_opts(item);
> -	int ret;
> -	char *c;
> -
> -	mutex_lock(&opts->lock);
> -	if (opts->refcnt > 1) {
> -		ret = -EBUSY;
> -		goto end;
> -	}
> -
> -	c = kstrndup(page, len, GFP_KERNEL);
> -	if (!c) {
> -		ret = -ENOMEM;
> -		goto end;
> -	}
> -	if (opts->id_allocated)
> -		kfree(opts->id);
> -	opts->id = c;
> -	opts->id_allocated = true;
> -	ret = len;
> -end:
> -	mutex_unlock(&opts->lock);
> -	return ret;
> -}
> -
> -CONFIGFS_ATTR(f_midi_opts_, id);
> +F_MIDI_OPT_STRING(id);
> +F_MIDI_OPT_STRING(interface_string);
>  
>  static struct configfs_attribute *midi_attrs[] = {
>  	&f_midi_opts_attr_index,
> @@ -1239,6 +1247,7 @@ static struct configfs_attribute *midi_attrs[] = {
>  	&f_midi_opts_attr_in_ports,
>  	&f_midi_opts_attr_out_ports,
>  	&f_midi_opts_attr_id,
> +	&f_midi_opts_attr_interface_string,
>  	NULL,
>  };
>  
> @@ -1264,6 +1273,8 @@ static void f_midi_free_inst(struct usb_function_instance *f)
>  	if (free) {
>  		if (opts->id_allocated)
>  			kfree(opts->id);
> +		if (opts->interface_string_allocated)
> +			kfree(opts->interface_string);
>  		kfree(opts);
>  	}
>  }
> @@ -1280,6 +1291,7 @@ static struct usb_function_instance *f_midi_alloc_inst(void)
>  	opts->func_inst.free_func_inst = f_midi_free_inst;
>  	opts->index = SNDRV_DEFAULT_IDX1;
>  	opts->id = SNDRV_DEFAULT_STR1;
> +	opts->interface_string = SNDRV_DEFAULT_STR1;

SNDRV_DEFAULT_STR1 is NULL, and this fact can simplify things a lot.

So, it's better to initialize explicitly with NULL instead of
SNDRV_DEFAULT_STR1, then you can just do NULL-check of the pointer
without the use of *_allocated field (that can be dropped).

Other than that, the code change looks good.


thanks,

Takashi