Hi Bhupesh, Thanks for the patch. On Wednesday 27 June 2012 17:54:49 Bhupesh SHARMA wrote: > On Wednesday, June 27, 2012 9:08 AM Bhupesh SHARMA wrote: > > > > 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). This has been a longstanding issue, even if the pre-UDC era. Even the MUSB driver, which I've used to develop the UVC gadget, doesn't handle the softconnect request properly. See http://marc.info/?l=linux- usb&m=124650198916887. > > 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). I agree with your analysis, this needs be fixed. > > 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 [snip] > A much-better approach would be to modify the udc-core instead of > modifying each UDC controller driver. A sample patch which does the > same is given below. > > Please share your ideas/views whether this seems ok. The overall approach looks good. I'll let Felipe comment on the details, he's much more knownledgeable than me about UDC. The idea is to mark gadgets as being connectable before we call the bind() function. A call to usb_gadget_disconnect() during bind() will mark the gadget as non-connectable, and the call to usb_gadget_connect() after bind() should then be skipped. Reference-counting the disconnected state might also be an implementation option, it could bring the additional benefit of properly handling multiple functions that require a userspace application. > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx> > Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx> > --- > drivers/usb/gadget/udc-core.c | 3 ++- > include/linux/usb/gadget.h | 10 +++++++++- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c > index a6ebeec..4851d99 100644 --- a/drivers/usb/gadget/udc-core.c > +++ b/drivers/usb/gadget/udc-core.c > @@ -355,7 +355,8 @@ found: > driver->unbind(udc->gadget); > goto err1; > } > - usb_gadget_connect(udc->gadget); > + if (!udc->gadget->is_pulleddown) > + 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..b1d997a 100644 --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -583,6 +583,7 @@ struct usb_gadget { > unsigned b_hnp_enable:1; > unsigned a_hnp_support:1; > unsigned a_alt_hnp_support:1; > + unsigned is_pulleddown:1; > const char *name; > struct device dev; > }; > @@ -808,9 +809,16 @@ static inline int usb_gadget_connect(struct usb_gadget > *gadget) */ > static inline int usb_gadget_disconnect(struct usb_gadget *gadget) { > + int ret; > + > if (!gadget->ops->pullup) > return -EOPNOTSUPP; > - return gadget->ops->pullup(gadget, 0); > + ret = gadget->ops->pullup(gadget, 0); > + > + if (!ret) > + gadget->is_pulleddown = true; > + > + return ret; > } -- Regards, Laurent Pinchart -- 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
