Hi, > -----Original Message----- > From: Bhupesh SHARMA > Sent: Wednesday, June 27, 2012 9:08 AM > To: balbi@xxxxxx; linux-usb@xxxxxxxxxxxxxxx > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; laurent.pinchart@xxxxxxxxxxxxxxxx > Subject: [RFC] usb: gadget: Ensure correct functionality for gadgets > that wish to 'connect' only when directed by user-space > > Hi Felipe, All, > > While working with the webcam gadget in recent past I came to realize > that if the webcam gadget > is compiled as a static part of the kernel, the USB webcam device when > connected to the USB > host will not successfully enumerate (as the USB host will send a > GET_DEF class specific UVC > request), which is handled by a user-space application responsible for > ensuring command/data > flow between the UVC gadget and a real V4L2 device. > > I believe similar will be the case for other gadgets like obex while > working with an underlying > UDC during the start-up phase (during the enumeration process). > > Gadgets like webcam/obex explicitly call the 'pullup' routine of the > underlying > UDC (with is_on argument set to 0) to issue soft-disconnect command at > the very start. This is > done to prevent the enumeration process to proceed unless a supporting > user-space application > is first up and running (which can handle the class-specific control > requests on the basis of > certain negotiations with other entities, like for e.g. a real video > device in case of webcam). > (see [1] for reference). > > However with the current framework (newstyle UDC) just after the 'bind' > of the > function (f_uvc/f_obex) is called, the UDC framework calls the 'pullup' > routine of the > UDC again with a parameter is_on set to 1 to initiate connection with > the Host. > (see [2] for reference). > > This seems to be an incorrect implementation as it causes the > enumeration to > start even for gadgets like webcam/obex which have explicitly requested > only a soft-connect (as the user-space application has not been > launched yet). > > Please let me know your ideas on the same. > At the moment I have used a temporary approach like the one given below > (though it is in > no way a very good way out :) ). > > > [1] http://lxr.linux.no/linux+v3.4.4/drivers/usb/gadget/f_uvc.c#L548 > [2] http://lxr.linux.no/linux+v3.4.4/drivers/usb/gadget/udc-core.c#L341 > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx> > --- > drivers/usb/dwc3/core.h | 1 + > drivers/usb/dwc3/gadget.c | 24 ++++++++++++++++++------ > drivers/usb/gadget/udc-core.c | 19 ++++++++++++++++++- > include/linux/usb/gadget.h | 1 + > 4 files changed, 38 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 2d8676c..9125ec9 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -712,6 +712,7 @@ struct dwc3 { > struct dwc3_hwparams hwparams; > struct dentry *root; > + bool deactivated_at_start; > > u8 test_mode; > u8 test_mode_nr; > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index b82fe61..0d6ea17 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1341,6 +1341,12 @@ static const struct usb_ep_ops > dwc3_gadget_ep_ops = { > }; > > /* ------------------------------------------------------------------- > ------- */ > +static bool dwc3_gadget_is_deactivated_at_start(struct usb_gadget *g) > +{ > + struct dwc3 *dwc = gadget_to_dwc(g); > + > + return dwc->deactivated_at_start; > +} > > static int dwc3_gadget_get_frame(struct usb_gadget *g) > { > @@ -1448,6 +1454,7 @@ static void dwc3_gadget_run_stop(struct dwc3 > *dwc, int is_on) > { > u32 reg; > u32 timeout = 500; > + static bool first_deactivation; > > reg = dwc3_readl(dwc->regs, DWC3_DCTL); > if (is_on) { > @@ -1461,6 +1468,10 @@ static void dwc3_gadget_run_stop(struct dwc3 > *dwc, int is_on) > reg |= DWC3_DCTL_RUN_STOP; > } else { > reg &= ~DWC3_DCTL_RUN_STOP; > + if (first_deactivation == false) { > + dwc->deactivated_at_start = true; > + first_deactivation = true; > + } > } > > dwc3_writel(dwc->regs, DWC3_DCTL, reg); > @@ -1600,12 +1611,13 @@ static int dwc3_gadget_stop(struct usb_gadget > *g, > } > > static const struct usb_gadget_ops dwc3_gadget_ops = { > - .get_frame = dwc3_gadget_get_frame, > - .wakeup = dwc3_gadget_wakeup, > - .set_selfpowered = dwc3_gadget_set_selfpowered, > - .pullup = dwc3_gadget_pullup, > - .udc_start = dwc3_gadget_start, > - .udc_stop = dwc3_gadget_stop, > + .is_deactivated_at_start = > dwc3_gadget_is_deactivated_at_start, > + .get_frame = dwc3_gadget_get_frame, > + .wakeup = dwc3_gadget_wakeup, > + .set_selfpowered = dwc3_gadget_set_selfpowered, > + .pullup = dwc3_gadget_pullup, > + .udc_start = dwc3_gadget_start, > + .udc_stop = dwc3_gadget_stop, > }; > > /* ------------------------------------------------------------------- > ------- */ > diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc- > core.c > index a6ebeec..c42ea92 100644 > --- a/drivers/usb/gadget/udc-core.c > +++ b/drivers/usb/gadget/udc-core.c > @@ -261,6 +261,13 @@ static int udc_is_newstyle(struct usb_udc *udc) > return 0; > } > > +static int udc_is_deactivated_at_start(struct usb_udc *udc) > +{ > + if (udc->gadget->ops->is_deactivated_at_start) > + return udc->gadget->ops->is_deactivated_at_start(udc- > >gadget); > + > + return 0; > +} > > static void usb_gadget_remove_driver(struct usb_udc *udc) > { > @@ -355,7 +362,17 @@ found: > driver->unbind(udc->gadget); > goto err1; > } > - usb_gadget_connect(udc->gadget); > + > + /* > + * do not connect if the function has requested an > + * explicit disconnect at the very start and hence > + * does not want to connect unless a user-space application > + * explicitly calls the 'function_activate' routine > + * to connect (this is required in case of certain > + * functions like obex and webcam). > + */ > + if (!udc_is_deactivated_at_start(udc)) > + usb_gadget_connect(udc->gadget); > } else { > > ret = usb_gadget_start(udc->gadget, driver, bind); > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 665b1d1..4c4e665 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -507,6 +507,7 @@ struct usb_gadget_driver; > * which don't involve endpoints (or i/o). > */ > struct usb_gadget_ops { > + bool (*is_deactivated_at_start)(struct usb_gadget *); > int (*get_frame)(struct usb_gadget *); > int (*wakeup)(struct usb_gadget *); > int (*set_selfpowered) (struct usb_gadget *, int > is_selfpowered); > -- > 1.6.0.2 > > It's been almost a month since this RFC was circulated. As no comments have been received so far, can I assume that this approach is fine and send a formal patch for the same and will that be acceptable for inclusion in mainline? Regards, Bhupesh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
