Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Feedback-ID: ic4e149f5:Fastmail
Date:   Sat, 2 Dec 2023 14:51:15 +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 v2] usb: gadget: f_fs: Add the missing get_alt callback
Message-ID: <ZWtEY0oKS-PcaJ0c@keeping.me.uk>
References: <20231201145234.97452-1-hgajjar@de.adit-jv.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231201145234.97452-1-hgajjar@de.adit-jv.com>
Precedence: bulk

On Fri, Dec 01, 2023 at 03:52:34PM +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.
> 
> 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/

This doesn't address my questions about v1 - I understand what the
get_alt callback does, but I don't see how this is sufficient to make it
work in all circumstances.

To use your example of having settings 0 and 1, what happens if the host
requests setting 2?

Without this patch, when .get_alt is not set, composite_setup() will
reject all settings except 0 so there is no need for ffs_func_set_alt()
to filter its input.  But if .get_alt is set, don't we need to filter
for valid input here?

I also do not see how it is acceptable to change alt setting without
notifying userspace in the general case.  If it works for your specific
use case, that is one thing, but nothing requires the endpoint setup to
be the same across alt settings and the userspace component likely needs
to know if the setup changes.

For examples, look at afunc_set_alt() or tcm_set_alt() in other gadget
functions.  If either of these were to be implemented in userspace then
simply accepting the alt setting is not enough - there are changes that
must be made to the functionality.

> ---
>  drivers/usb/gadget/function/f_fs.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index efe3e3b85769..37c47c11f57a 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -75,6 +75,7 @@ struct ffs_function {
>  	short				*interfaces_nums;
>  
>  	struct usb_function		function;
> +	int				cur_alt[MAX_CONFIG_INTERFACES];
>  };
>  
>  
> @@ -98,6 +99,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 +3234,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)
>  {
> @@ -3266,8 +3277,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 +3587,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
>