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

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

[Hardik Gajjar]

Some USB OTG hubs have multiple alternate configurations to offer,
such as one for Carplay and another for Carlife.

This patch implements and sets the get_alt callback to retrieve the
currently used alternate setting. The new function allows dynamic
retrieval of the current alternate setting for a specific interface. The
current alternate setting values are stored in the 'cur_alt' array
within the 'ffs_function' structure.

Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
---
 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

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

[John Keeping]

On Fri, Nov 24, 2023 at 05:44:35PM +0100, Hardik Gajjar wrote:
> Some USB OTG hubs have multiple alternate configurations to offer,
> such as one for Carplay and another for Carlife.
> 
> This patch implements and sets the get_alt callback to retrieve the
> currently used alternate setting. The new function allows dynamic
> retrieval of the current alternate setting for a specific interface. The
> current alternate setting values are stored in the 'cur_alt' array
> within the 'ffs_function' structure.

Doesn't the alt setting need to be forwarded to userspace?

What happens if the available endpoints change - doesn't that mean the
available endpoint files change?

It's not sufficient to just blindly accept any alt setting and assume it
will work, that may be the case in one specific constrained scenario,
but it's not true in general.  At the very least we must not accept an
alt setting that is not defined in the descriptors.

> Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> ---
>  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
> 

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

[Hardik Gajjar]

On Sun, Nov 26, 2023 at 12:41:15PM +0000, John Keeping wrote:
> On Fri, Nov 24, 2023 at 05:44:35PM +0100, Hardik Gajjar wrote:
> > Some USB OTG hubs have multiple alternate configurations to offer,
> > such as one for Carplay and another for Carlife.
> > 
> > This patch implements and sets the get_alt callback to retrieve the
> > currently used alternate setting. The new function allows dynamic
> > retrieval of the current alternate setting for a specific interface. The
> > current alternate setting values are stored in the 'cur_alt' array
> > within the 'ffs_function' structure.
> 
> Doesn't the alt setting need to be forwarded to userspace?
> 
> What happens if the available endpoints change - doesn't that mean the
> available endpoint files change?
> 
> It's not sufficient to just blindly accept any alt setting and assume it
> will work, that may be the case in one specific constrained scenario,
> but it's not true in general.  At the very least we must not accept an
> alt setting that is not defined in the descriptors.

The commit message appears a bit confusing.
The user space creates an FFS/iAP (Apple) gadget and writes an ep0 descriptor
with multiple alt settings. The host then sends the set_alt request to configure
alt_setting 0 or 1, verified by the subsequent get_alt request. Without this
patch, the Host/Phone consistently sets alt 0 and retrieves alt 0, even when
the request is for alt 1.

Given that f_fs is a user space-controlled gadget, creating a descriptor seems
quite flexible for this type of gadget.

I intend to submit an upstream patch v2 with a clearer commit message.

> 
> > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> > ---
> >  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
> > 

Thanks,
Hardik

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

[Hardik Gajjar]

The Apple CarLife iAP gadget has a descriptor in userspace 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.

unlike the uvc and ncm, f_fs gadget is fully implemented in userspace,
and driver just reset the eps and generate the event. so no additional
adaptaion associated with this change is not required in set_alt callback

Signed-off-by: Hardik Gajjar <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 6bff6cb93789..a3e8cf7963e7 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 *);
@@ -3231,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)
 {
@@ -3238,6 +3250,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)
@@ -3265,8 +3280,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;
 }
 
@@ -3573,6 +3590,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