| Message ID | 20231124164435.74727-1-hgajjar@de.adit-jv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: f_fs: Add the missing get_alt callback | expand |
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 >
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
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;
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(-)