From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752169AbbD3PNm (ORCPT ); Thu, 30 Apr 2015 11:13:42 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:44402 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751896AbbD3PNk (ORCPT ); Thu, 30 Apr 2015 11:13:40 -0400 Date: Thu, 30 Apr 2015 10:12:02 -0500 From: Felipe Balbi To: Robert Baldyga CC: , , , , , Subject: Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions Message-ID: <20150430151202.GE1515@saruman.tx.rr.com> Reply-To: References: <1428395513-22229-1-git-send-email-r.baldyga@samsung.com> <1428395513-22229-2-git-send-email-r.baldyga@samsung.com> <20150428164034.GH18263@saruman.tx.rr.com> <55409F76.70103@samsung.com> <20150429153000.GE7262@saruman.tx.rr.com> <5541F10B.4030801@samsung.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="r7U+bLA8boMOj+mD" Content-Disposition: inline In-Reply-To: <5541F10B.4030801@samsung.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --r7U+bLA8boMOj+mD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Apr 30, 2015 at 11:08:27AM +0200, Robert Baldyga wrote: > On 04/29/2015 05:30 PM, Felipe Balbi wrote: > > On Wed, Apr 29, 2015 at 11:08:06AM +0200, Robert Baldyga wrote: > >> Hi Felipe, > >> > >> On 04/28/2015 06:40 PM, Felipe Balbi wrote: > >>> On Tue, Apr 07, 2015 at 10:31:52AM +0200, Robert Baldyga wrote: > >>>> These functions allows to deactivate gadget to make it not visible to > >>>> host and make it active again when gadget driver is finally ready. > >>>> > >>>> They are needed to fix usb_function_activate() and usb_function_deac= tivate() > >>>> functions which currently are not working as usb_gadget_connect() is > >>>> called immediately after function bind regardless to previous calls = of > >>>> usb_gadget_disconnect() function. > >>> > >>> and that's what needs to be fixed, a long time ago I wrote the patch > >>> below which I never got to finishing: > >>> > >>> commit a23800e2463ae1f4eafa7c0a15bb44afee75994f > >>> Author: Felipe Balbi > >>> Date: Thu Jul 26 14:23:44 2012 +0300 > >>> > >>> usb: gadget: let gadgets control pullup on their own > >>> =20 > >>> This is useful on gadgets that depend on userland > >>> daemons to function properly. We can delay connection > >>> to the host until userland is ready. > >>> =20 > >>> Signed-off-by: Felipe Balbi > >>> > >>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/comp= osite.c > >>> index 7c821de8ce3d..790ccf29f2ee 100644 > >>> --- a/drivers/usb/gadget/composite.c > >>> +++ b/drivers/usb/gadget/composite.c > >>> @@ -1784,8 +1784,9 @@ int usb_composite_probe(struct usb_composite_dr= iver *driver) > >>> driver->name =3D "composite"; > >>> =20 > >>> driver->gadget_driver =3D composite_driver_template; > >>> - gadget_driver =3D &driver->gadget_driver; > >>> =20 > >>> + gadget_driver =3D &driver->gadget_driver; > >>> + gadget_driver->controls_pullups =3D driver->controls_pullups; > >>> gadget_driver->function =3D (char *) driver->name; > >>> gadget_driver->driver.name =3D driver->name; > >>> gadget_driver->max_speed =3D driver->max_speed; > >>> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-c= ore.c > >>> index 8a1eeb24ae6a..c0f4fca9384b 100644 > >>> --- a/drivers/usb/gadget/udc-core.c > >>> +++ b/drivers/usb/gadget/udc-core.c > >>> @@ -235,7 +235,18 @@ static void usb_gadget_remove_driver(struct usb_= udc *udc) > >>> =20 > >>> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > >>> =20 > >>> - usb_gadget_disconnect(udc->gadget); > >>> + /* > >>> + * NOTICE: if gadget driver wants to control > >>> + * pullup, it needs to make sure that when > >>> + * user tries to rmmod the gadget driver, it > >>> + * will disconnect the pullups before returning > >>> + * from its ->unbind() method. > >>> + * > >>> + * We are truly trusting the gadget driver here. > >>> + */ > >>> + if (!udc->driver->controls_pullups) > >>> + usb_gadget_disconnect(udc->gadget); > >>> + > >>> udc->driver->disconnect(udc->gadget); > >>> udc->driver->unbind(udc->gadget); > >>> usb_gadget_udc_stop(udc->gadget, udc->driver); > >>> @@ -300,7 +311,18 @@ static int udc_bind_to_driver(struct usb_udc *ud= c, struct usb_gadget_driver *dri > >>> driver->unbind(udc->gadget); > >>> goto err1; > >>> } > >>> - usb_gadget_connect(udc->gadget); > >>> + > >>> + /* > >>> + * NOTICE: if gadget driver wants to control > >>> + * pullups, it needs to make sure its calls > >>> + * to usb_function_activate() and > >>> + * usb_function_deactivate() are balanced, > >>> + * otherwise gadget_driver will never enumerate. > >>> + * > >>> + * We are truly trusting the gadget driver here. > >>> + */ > >>> + if (!driver->controls_pullups) > >>> + usb_gadget_connect(udc->gadget); > >>> =20 > >>> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > >>> return 0; > >>> diff --git a/include/linux/usb/composite.h b/include/linux/usb/compos= ite.h > >>> index 3c671c1b37f6..7ae797c85cb9 100644 > >>> --- a/include/linux/usb/composite.h > >>> +++ b/include/linux/usb/composite.h > >>> @@ -157,6 +157,7 @@ struct usb_function { > >>> int (*get_status)(struct usb_function *); > >>> int (*func_suspend)(struct usb_function *, > >>> u8 suspend_opt); > >>> + > >>> /* private: */ > >>> /* internals */ > >>> struct list_head list; > >>> @@ -279,6 +280,8 @@ enum { > >>> * @max_speed: Highest speed the driver supports. > >>> * @needs_serial: set to 1 if the gadget needs userspace to provide > >>> * a serial number. If one is not provided, warning will be printe= d. > >>> + * @controls_pullups: this driver will control pullup and udc-core s= houldn't > >>> + * enable it by default > >>> * @bind: (REQUIRED) Used to allocate resources that are shared acro= ss the > >>> * whole device, such as string IDs, and add its configurations using > >>> * @usb_add_config(). This may fail by returning a negative errno > >>> @@ -308,6 +311,7 @@ struct usb_composite_driver { > >>> struct usb_gadget_strings **strings; > >>> enum usb_device_speed max_speed; > >>> unsigned needs_serial:1; > >>> + unsigned controls_pullups:1; > >>> =20 > >>> int (*bind)(struct usb_composite_dev *cdev); > >>> int (*unbind)(struct usb_composite_dev *); > >>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > >>> index 32b734d88d6b..87971fa38f08 100644 > >>> --- a/include/linux/usb/gadget.h > >>> +++ b/include/linux/usb/gadget.h > >>> @@ -774,6 +774,7 @@ static inline int usb_gadget_disconnect(struct us= b_gadget *gadget) > >>> * @suspend: Invoked on USB suspend. May be called in_interrupt. > >>> * @resume: Invoked on USB resume. May be called in_interrupt. > >>> * @driver: Driver model state for this driver. > >>> + * @controls_pullups: tells udc-core to not enable pullups by default > >>> * > >>> * Devices are disabled till a gadget driver successfully bind()s, w= hich > >>> * means the driver will handle setup() requests needed to enumerate= (and > >>> @@ -833,6 +834,8 @@ struct usb_gadget_driver { > >>> =20 > >>> /* FIXME support safe rmmod */ > >>> struct device_driver driver; > >>> + > >>> + unsigned controls_pullups:1; > >>> }; > >>> =20 > >>> =20 > >>> > >>> Together with that, I wrote: > >>> > >>> commit 0b733885e276a38cc9fa415c0977f063f9ce4d9d > >>> Author: Felipe Balbi > >>> Date: Wed Feb 6 12:34:33 2013 +0200 > >>> > >>> usb: gadget: webcam: let it control pullups > >>> =20 > >>> this gadget driver needs to make sure that > >>> we will only enumerate after its userland > >>> counterpart is up and running. > >>> =20 > >>> Signed-off-by: Felipe Balbi > >>> > >>> diff --git a/drivers/usb/gadget/webcam.c b/drivers/usb/gadget/webcam.c > >>> index 8cef1e658c29..41a4d03715bc 100644 > >>> --- a/drivers/usb/gadget/webcam.c > >>> +++ b/drivers/usb/gadget/webcam.c > >>> @@ -388,6 +388,7 @@ static __refdata struct usb_composite_driver webc= am_driver =3D { > >>> .max_speed =3D USB_SPEED_SUPER, > >>> .bind =3D webcam_bind, > >>> .unbind =3D webcam_unbind, > >>> + .controls_pullups =3D true, > >>> }; > >>> =20 > >>> static int __init > >>> > >>> This makes it really simple, either a gadget driver wants to control > >>> pullups, or it doesn't. For those who wish to control pullups (for > >>> whatever reason) we rely completely on the gadget telling us it's ok = to > >>> connect pullups by means of usb_function_activate()/deactivate(), for > >>> all others, we just connect straight away. > >>> > >> > >> It looks like your patch fixes problem at gadget level, but in case of > >> gadgets composed using ConfigFS we don't know if any function will want > >> to deactivate gadget. We would need to supply mechanism which functions > >> could use to tell composite that they want to control pullups by > >> themselves. That's why I made it a little more complicated, but in > >> result we have no need to modify function drivers. > >=20 > > I really prefer functions to tell us that they can control pullups; much > > like Host stack requires a "supports_autosuspend" flag if your driver > > supports runtime PM. > >=20 > > I see that your patch is much more granular, but then, we need a > > per-function flag then we could: > >=20 > > for_each_function(config) > > if (fuction->controls_pullup) > > usb_function_deactivate(function); > >=20 > > (well, just illustrating) > >=20 > > Then, usb_function_deactivate() is initially called by the framework so > > that deactivate counter already has the correct initial value and gadget > > will only connect after all functions in a particular configuration have > > called usb_function_activate(). How do you feel about that ? >=20 > Looks good to me, makes things more clear. I will update my patches. Cool, thanks Let's see what you come up with for configuration changes, I didn't think too much about it, but we need to consider that possibility too. --=20 balbi --r7U+bLA8boMOj+mD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVQkZCAAoJEIaOsuA1yqREafwP/2qcftlEaayfheUSJe93QVGX ljsH7RBWQjZ5Mv5mnwpTnE+3Nqis6vw/TOguliCO464KGpHKDUEib4UrfWNBmJZo gKeENYOjhE35dMFYqm1rL8siyjFDGelAhFlq1MvnaZbF0YtD3bNZM1PgerRmMTFQ td7DHVeh54Hrfmuhkk0O46rwBzznSU4lCRARZsyxD7j5SVzKyMMBsGx5Cfvt6xrw 6+BQXJrFg7SAYE4qn2Fs1xLd7JgEk5OwOe/MrB3YUWGgT8043hXT2lGQDu6Ms8Xx 1e+/fky+skb5cYWUTYh8ua8NpMClbSuyE3gO3HuoWSLDNnSBYC7qOG++mgrW39Ib Ev3/u17+S99cYk+LcgYKPtODENA82Evf6zqZcQ1JUGgwBKGnTcnfWalt/nWDX1jp 9dkHVVssiYDO4bG6H4DKqY7lFdAJiBWX1Va8ycM15MahYoarfFEmW8L7N5lA6Hw6 GJkSBQoTVfnWPqdOlXkbC+12rK1si7o7n/I5h9puRWvkjy6Zz+QRTtof74RvZjtc YbKdBOLZsHn/fOY5dQb+fbrPVkm5t/9cyTfPwSAHuD+EyO6kpBeSLthQdQgly+M9 jeiHhvNZi9klI40BW/KZ+OMkOhIGL6QrLcuVttSg1oOSeEaNfJXdzXVhnajYzv2P 1kTZF0McUz7lSRA9emyl =j4un -----END PGP SIGNATURE----- --r7U+bLA8boMOj+mD--