RE: [RFC] usb: gadget: Ensure correct functionality for gadgets that wish to 'connect' only when directed by user-space

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux