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