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@xxxxxxxxxxxxxx>
> > ---
> > 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
