Received-SPF: pass (google.com: domain of linux-kernel+bounces-16570-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: ic4e149f5:Fastmail
Date: Thu, 4 Jan 2024 11:16:39 +0000
From: John Keeping <john@keeping.me.uk>
To: Hardik Gajjar <hgajjar@de.adit-jv.com>
Cc: gregkh@linuxfoundation.org, stern@rowland.harvard.edu,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	erosca@de.adit-jv.com, jlayton@kernel.org, brauner@kernel.org
Subject: Re: [PATCH v3] usb: gadget: f_fs: Add the missing get_alt callback
Message-ID: <ZZaTl3mqeCY5xD_d@keeping.me.uk>
References: <20240102123419.13491-1-hgajjar@de.adit-jv.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240102123419.13491-1-hgajjar@de.adit-jv.com>

On Tue, Jan 02, 2024 at 01:34:19PM +0100, Hardik Gajjar wrote:
> The Apple CarLife iAP gadget has a descriptor with two alternate
> settings. The host sends the set_alt request to configure alt_setting
> 0 or 1, and this is verified by the subsequent get_alt request.
> 
> This patch implements and sets the get_alt callback. Without the
> get_alt callback, composite.c abruptly concludes the
> USB_REQ_GET/SET_INTERFACE request, assuming only one alt setting
> for the endpoint.

I still do not understand what happens if different alternate settings
have different endpoints.

Changing the alternate calls ffs_func_eps_disable() /
ffs_func_eps_enable() but those functions affect _all_ the configured
endpoints.  If f_fs moves to support multiple alternate settings, then
isn't there a problem with that behaviour?  Don't we need
ffs_func_eps_enable() to enable only the endpoints used by the current
alternate setting?

The commit message does not explain why this patch can be as simple as
it is and why there is no need to address any wider issues that there
seem to be from supporting multiple alternate settings.

> Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> ---
> changes since version 1:
> 	- improve commit message to indicate why the get_alt callback
> 	  is necessary
> 	- Link to v1 - https://lore.kernel.org/all/20231124164435.74727-1-hgajjar@de.adit-jv.com/
> 
> changes since version 2:
> 	- Add the limit to allow set up to 2 alt settings.
> 	- Link to v2 - https://lore.kernel.org/all/20231201145234.97452-1-hgajjar@de.adit-jv.com/
> ---
>  drivers/usb/gadget/function/f_fs.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index efe3e3b85769..22200d618184 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -42,6 +42,7 @@
>  #include "configfs.h"
>  
>  #define FUNCTIONFS_MAGIC	0xa647361 /* Chosen by a honest dice roll ;) */
> +#define MAX_ALT_SETTINGS	2		  /* Allow up to 2 alt settings to be set. */
>  
>  /* Reference counter handling */
>  static void ffs_data_get(struct ffs_data *ffs);
> @@ -75,6 +76,7 @@ struct ffs_function {
>  	short				*interfaces_nums;
>  
>  	struct usb_function		function;
> +	int				cur_alt[MAX_CONFIG_INTERFACES];
>  };
>  
>  
> @@ -98,6 +100,7 @@ static int __must_check ffs_func_eps_enable(struct ffs_function *func);
>  static int ffs_func_bind(struct usb_configuration *,
>  			 struct usb_function *);
>  static int ffs_func_set_alt(struct usb_function *, unsigned, unsigned);
> +static int ffs_func_get_alt(struct usb_function *f, unsigned int intf);
>  static void ffs_func_disable(struct usb_function *);
>  static int ffs_func_setup(struct usb_function *,
>  			  const struct usb_ctrlrequest *);
> @@ -3232,6 +3235,15 @@ static void ffs_reset_work(struct work_struct *work)
>  	ffs_data_reset(ffs);
>  }
>  
> +static int ffs_func_get_alt(struct usb_function *f,
> +			    unsigned int interface)
> +{
> +	struct ffs_function *func = ffs_func_from_usb(f);
> +	int intf = ffs_func_revmap_intf(func, interface);
> +
> +	return (intf < 0) ? intf : func->cur_alt[interface];
> +}
> +
>  static int ffs_func_set_alt(struct usb_function *f,
>  			    unsigned interface, unsigned alt)
>  {
> @@ -3239,6 +3251,9 @@ static int ffs_func_set_alt(struct usb_function *f,
>  	struct ffs_data *ffs = func->ffs;
>  	int ret = 0, intf;
>  
> +	if (alt > MAX_ALT_SETTINGS)
> +		return -EINVAL;
> +
>  	if (alt != (unsigned)-1) {
>  		intf = ffs_func_revmap_intf(func, interface);
>  		if (intf < 0)
> @@ -3266,8 +3281,10 @@ static int ffs_func_set_alt(struct usb_function *f,
>  
>  	ffs->func = func;
>  	ret = ffs_func_eps_enable(func);
> -	if (ret >= 0)
> +	if (ret >= 0) {
>  		ffs_event_add(ffs, FUNCTIONFS_ENABLE);
> +		func->cur_alt[interface] = alt;
> +	}
>  	return ret;
>  }
>  
> @@ -3574,6 +3591,7 @@ static struct usb_function *ffs_alloc(struct usb_function_instance *fi)
>  	func->function.bind    = ffs_func_bind;
>  	func->function.unbind  = ffs_func_unbind;
>  	func->function.set_alt = ffs_func_set_alt;
> +	func->function.get_alt = ffs_func_get_alt;
>  	func->function.disable = ffs_func_disable;
>  	func->function.setup   = ffs_func_setup;
>  	func->function.req_match = ffs_func_req_match;
> -- 
> 2.17.1
>