Hi, > -----Original Message----- > From: Bhupesh SHARMA > Sent: Thursday, July 26, 2012 6:02 PM > To: 'balbi@xxxxxx' > Cc: 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, > > > -----Original Message----- > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Thursday, July 26, 2012 5:30 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:51:05PM +0800, Bhupesh SHARMA wrote: > > > > | @@ -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; > > > } > > > > not enough. When you unload a gadget driver usb_gadget_disconnect() > > *MUST* be called, so is_pulleddown will be true again and > > g_mass_storage will continue not to enumerate. > > Please refer to the logs below. The g_mass_storage enumerates perfectly > fine on a Windows XP PC and works fine too (I tried to view some files > kept on the storage media and was able to), more on that below.. > > > > > 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. > > > > that's the kind of work my patch did. It creates a way for gadget > > drivers to tell udc-core that gadget driver wants to manage pullups. > > > > > 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 > > > > this means g_mass_storage is resetting and not enumerating > properly... > > not sure if it was caused by your patch though. > > > > I removed my patch and tested again. I get the same logs. > So, this is definitely not caused by my patch (I even captured a screen > shot of my XP PC's Device Manger and attached with this mail). > > May-be a issue in g_mass_storage :) > Felipe, after a exhaustive test run, I can confirm that the 2nd RFC patch sent by me + this change to clear the 'is_pulleddown' flag works correctly on the following 5 gadgets (various combinations tried to remove/insert gadget modules): - serial - mass storage - webcam - zero - audio All can be enumerated and used correctly (on both Linux and Windows Host). So, we now have two sets of patches: - this one, which requires no change in the gadget driver or composite layer and has been tested extensively. - and the one sent by you which changes the framework to make gadget driver and udc layer talk, which requires changing all gadget drivers which want to request a 'delayed' enumeration (when directed by user-space). Please let me know your view on how to proceed, so that we can close this long-pending issue. 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
