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. > > 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. -- balbi
signature.asc
Description: Digital signature
