From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751372AbbD3JIf (ORCPT ); Thu, 30 Apr 2015 05:08:35 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:10588 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750934AbbD3JIb (ORCPT ); Thu, 30 Apr 2015 05:08:31 -0400 X-AuditID: cbfec7f4-f79c56d0000012ee-12-5541f1118032 Message-id: <5541F10B.4030801@samsung.com> Date: Thu, 30 Apr 2015 11:08:27 +0200 From: Robert Baldyga User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-version: 1.0 To: balbi@ti.com Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, m.szyprowski@samsung.com, andrzej.p@samsung.com Subject: Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions 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> In-reply-to: <20150429153000.GE7262@saruman.tx.rr.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrALMWRmVeSWpSXmKPExsVy+t/xa7qCHx1DDU5eZLaY9bKdxeLg/XqL 5sXr2Swu75rDZrFoWSuzxdojd9kd2Dz2z13D7tG3ZRWjx/Eb25k8Pm+SC2CJ4rJJSc3JLEst 0rdL4Mq4s2Uua8Fip4o1348xNjB+NO1i5OSQEDCReL39CjuELSZx4d56ti5GLg4hgaWMEl9P d7FDOM8YJe6+vswKUsUroCXxefVnZhCbRUBV4ubKXUwgNpuAjsSW7xMYuxg5OEQFIiS6T1RC lAtK/Jh8jwXEFhEQkFj/4hLYTGaBLkaJ9V9Pgm0WFgiR2N08C2rzd0aJXZfWg3VwCphJ/GhY xwoylFlAT+L+RS2QMLOAvMTmNW+ZJzAKzEKyYxZC1SwkVQsYmVcxiqaWJhcUJ6XnGuoVJ+YW l+al6yXn525ihITzlx2Mi49ZHWIU4GBU4uFlmOYYKsSaWFZcmXuIUYKDWUmE9/AjoBBvSmJl VWpRfnxRaU5q8SFGaQ4WJXHeubvehwgJpCeWpGanphakFsFkmTg4pRoYNY/eDzI9kF9v9URm t0FuQ/7TYlnNBBkZAecE25Nv4t6+19cSW10R9p1/ZhzL694wuV1py3rPCwms3vM+geuHkaod +8a5F34wNjQLXb0tZCIpqbN9h1/t3GX3AyvaHjz0lGYTmNVWq3pWsoxzT0/KdsdHbq76/fqN Yt//GuziELu7prPu6mclluKMREMt5qLiRAAiPxa1YwIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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_deactivate() >>>> 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 >>> >>> This is useful on gadgets that depend on userland >>> daemons to function properly. We can delay connection >>> to the host until userland is ready. >>> >>> Signed-off-by: Felipe Balbi >>> >>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.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_driver *driver) >>> driver->name = "composite"; >>> >>> driver->gadget_driver = composite_driver_template; >>> - gadget_driver = &driver->gadget_driver; >>> >>> + gadget_driver = &driver->gadget_driver; >>> + gadget_driver->controls_pullups = driver->controls_pullups; >>> gadget_driver->function = (char *) driver->name; >>> gadget_driver->driver.name = driver->name; >>> gadget_driver->max_speed = driver->max_speed; >>> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.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) >>> >>> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); >>> >>> - 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 *udc, 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); >>> >>> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); >>> return 0; >>> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.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 printed. >>> + * @controls_pullups: this driver will control pullup and udc-core shouldn't >>> + * enable it by default >>> * @bind: (REQUIRED) Used to allocate resources that are shared across 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; >>> >>> 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 usb_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, which >>> * means the driver will handle setup() requests needed to enumerate (and >>> @@ -833,6 +834,8 @@ struct usb_gadget_driver { >>> >>> /* FIXME support safe rmmod */ >>> struct device_driver driver; >>> + >>> + unsigned controls_pullups:1; >>> }; >>> >>> >>> >>> 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 >>> >>> this gadget driver needs to make sure that >>> we will only enumerate after its userland >>> counterpart is up and running. >>> >>> 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 webcam_driver = { >>> .max_speed = USB_SPEED_SUPER, >>> .bind = webcam_bind, >>> .unbind = webcam_unbind, >>> + .controls_pullups = true, >>> }; >>> >>> 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. > > 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. > > I see that your patch is much more granular, but then, we need a > per-function flag then we could: > > for_each_function(config) > if (fuction->controls_pullup) > usb_function_deactivate(function); > > (well, just illustrating) > > 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 ? Looks good to me, makes things more clear. I will update my patches. Thanks, Robert Baldyga