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 :) Regards, Bhupesh
Attachment:
usb-mass-storage-enumeration.png
Description: usb-mass-storage-enumeration.png
