Hi, > -----Original Message----- > From: Felipe Balbi [mailto:balbi@xxxxxx] > Sent: Thursday, July 26, 2012 4:38 PM > To: Bhupesh SHARMA > Cc: balbi@xxxxxx; Laurent Pinchart; linux-usb@xxxxxxxxxxxxxxx > Subject: Re: [RFC] usb: gadget: Ensure correct functionality for > gadgets that wish to 'connect' only when directed by user-space > > Hi, > > On Thu, Jul 26, 2012 at 07:00:03PM +0800, Bhupesh SHARMA wrote: > > > > Yes. Felipe, plz go through the problem description as mentioned > > > > in the patch log message which can be seen here: > > > > http://permalink.gmane.org/gmane.linux.usb.general/66585 > > > > > > sure, please see above patch. The difference is that this has no > > > impact on the controller drivers. > > > > Yes. I had sent a modified patch (on top of my original one) that did > > not affect the controller driver and instead modified the way > 'pullup' > > is called from the udc-core, based on the simple 'is_pulleddown' > flag. Plz see it here: > > > > http://www.spinics.net/lists/linux-usb/msg66338.html > > > > This patch has an added advantage of not even modifying the > > webcam_driver like composite drivers and has been tested to work well > at-least with the webcam gadget. > > That patch doesn't make a lot of sense. I'll paste it here for > reference: > > | 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; > | } > > My problem is with this last hunk, for a number of reasons. > > a. you never clear that flag which means that if you load uvc (which > controls pullup), then unload it and load g_mass_storage (which > doesn't control pullup) g_mass_storage will never enumerate. Good point. So, if I add the following in addition to the 2nd RFC version, this can be handled correctly: diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index b1d997a..f4ff1e1 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -787,9 +787,16 @@ static inline int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) */ static inline int usb_gadget_connect(struct usb_gadget *gadget) { + int ret; + if (!gadget->ops->pullup) return -EOPNOTSUPP; - return gadget->ops->pullup(gadget, 1); + ret = gadget->ops->pullup(gadget, 1); + + if (!ret) + gadget->is_pulleddown = false; + + return ret; } /** > b. gadget driver and udc are not talking to each other, so there's no > explicit agreement on what to do with pullup and who will > control it. Yes, that's a limitation of the gadget framework. > c. I want all gadget drivers to control their own pullups in the near > future, so that exposing that to userland becomes simpler, this > patch doesn't get close to that. > > d. you tested only with the gadget you care about. You need a much more > extensive test to be able to show me that this will cause no > regressions. Ok. With the above fix added to 2nd RFC patch, I quickly tested the insertion and removal of a few USB gadgets on my board (underlying UDC: DWC3, Host : Windows XP/High-Speed): Note: I just added a few comments to separate the various gadget loading/unloading /* UVC */ # insmod /lib/modules/g_webcam.ko # start uvc-v4l2-daemon # kill -9 uvc-v4l2 daemon # rmmod g_webcam /* Mass Storage */ # insmod /lib/modules/g_mass_storage.ko gadget: Mass Storage Function, version: 2009/09/11 gadget: Number of LUNs=1 lun0: LUN: removable file: (no medium) gadget: Mass Storage Gadget, version: 2009/09/11 gadget: userspace failed to provide iSerialNumber gadget: g_mass_storage ready # g_mass_storage gadget: high-speed config #1: Linux File-Backed Storage g_mass_storage gadget: high-speed config #1: Linux File-Backed Storage g_mass_storage gadget: high-speed config #1: Linux File-Backed Storage g_mass_storage gadget: high-speed config #1: Linux File-Backed Storage g_mass_storage gadget: high-speed config #1: Linux File-Backed Storage g_mass_storage gadget: high-speed config #1: Linux File-Backed Storage g_mass_storage gadget: high-speed config #1: Linux File-Backed Storage ... # rmmod g_mass_storage /* Serial */ # insmod /lib/modules/g_serial.ko gadget: Gadget Serial v2.4 gadget: g_serial ready # rmmod g_serial /* Zero */ # insmod /lib/modules/g_zero.ko gadget: Gadget Zero, version: Cinco de Mayo 2008 gadget: zero ready # The enumeration of these 4 gadgets worked absolutely fine with these changes. Please let me know your comments. Regards, Bhupesh > e. This changes the behavior for all gadgets after the first gadget is > loaded (because you never clear is_pulleddown). > In summary, this patch is wrong and can't be applied. > -- 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
