Hi,
> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Thursday, July 26, 2012 4:38 PM
> To: Bhupesh SHARMA
> Cc: [email protected]; Laurent Pinchart; [email protected]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
