Re: [PATCH v2] usb: gadget: f_fs: Add the missing get_alt callback

[Hardik Gajjar]

On Sat, Dec 02, 2023 at 02:51:15PM +0000, John Keeping wrote:
> 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@xxxxxxxxxxxxxx>
> > ---
> > changes since version 1:
> > 	- improve commit message to indicate why the get_alt callback
> > 	  is necessary
> > 	- Link to v1 - https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_20231124164435.74727-2D1-2Dhgajjar-40de.adit-2Djv.com_&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=HVW16RXWO_lhdf8dGUC8lUHmcMmonwJgRYsnIBtUkIvOKiQCwdrs5RXMJKo9Qhh9&s=tbwPiQ25PgliLyyD75FDRJqiFxR-1ExoOMIXOD4WEZI&e=
> 
> 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.

Indeed, we require a filter for the alt value, and my suggestion is to set the filter to allow alt values up to two.

The set_alt callback function in the f_fs driver is currently resetting the associated endpoints by disabling and enabling them again for positive alt values.
Additionally, it sets ffs_event FUNCTIONFS_ENABLE, which provide userspaces notifications about some change.
It appears that the same event mechanism is used when there is a request to set the alt setting to -1, notifying by FUNCTIONFS_DISABLE.
For example, you can find the link to the Android USB daemon utilizing this FFS event in userspace.
https://gerrit.pixelexperience.org/plugins/gitiles/system_core/+/b36b54cf796d722653ad9bea11284815a3f5eda5/adb/daemon/usb.cpp#331

It's evident that the f_fs driver significantly differs from other gadget drivers (e.g., ncm, uac), where major control resides in userspace.

I believe there's no need for a mechanism to notify userspace for the f_fs driver.
Userspace triggers the data read, eventually called by usb_ep_queue() in f_fs.
As soon as set_alt resets the associated endpoint, the UDC driver should call the endpoint callback with an error, smoothly terminating the userspace system call with the appropriate error.

I couldn't find any userspace notification implementation in tcm_set_alt.

Please let me correct if sounds inappropriate.

Thanks,
Hardik
 
> 
> > ---
> >  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
> >