* [PATCH 0/2] usb: gadget: Fix gadget deactivaton feature @ 2015-04-07 8:31 Robert Baldyga 2015-04-07 8:31 ` [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions Robert Baldyga 2015-04-07 8:31 ` [PATCH 2/2] usb: composite: fix usb_function_activate/deactivate functions Robert Baldyga 0 siblings, 2 replies; 8+ messages in thread From: Robert Baldyga @ 2015-04-07 8:31 UTC (permalink / raw) To: balbi Cc: gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p, Robert Baldyga Hi, This patch set introduces two functions usb_gadget_deactivate() and usb_gadget_activate(), designed to prevent udc-core from showing binded gadget to host until it will be ready to work. It also makes usb_function_deactivate()/activate() using these functions. So far gadget deactivation was made by calling usb_gadget_disconnect(), but since we have usb_gadget_connect() called after gadget->bind() (in udc_bind_to_driver()) this method doesn't provide expected result. Calling function usb_gadget_disconnect() before gadget connection doesn't prevent udc-core from connecting gadget to driver - usb_gadget_disconnect() call is ignored and gadget is connected regardless to it. This usually results with errors, for example because we binded gadget with 0 configurations. This patch set fixes this problem adding functions allowing to perform effective gadget deactivation from gadget->bind(). Best regards, Robert Baldyga Robert Baldyga (2): usb: gadget: add usb_gadget_activate/deactivate functions usb: composite: fix usb_function_activate/deactivate functions drivers/usb/gadget/composite.c | 4 +- include/linux/usb/gadget.h | 100 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 8 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions 2015-04-07 8:31 [PATCH 0/2] usb: gadget: Fix gadget deactivaton feature Robert Baldyga @ 2015-04-07 8:31 ` Robert Baldyga 2015-04-28 16:40 ` Felipe Balbi 2015-04-07 8:31 ` [PATCH 2/2] usb: composite: fix usb_function_activate/deactivate functions Robert Baldyga 1 sibling, 1 reply; 8+ messages in thread From: Robert Baldyga @ 2015-04-07 8:31 UTC (permalink / raw) To: balbi Cc: gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p, Robert Baldyga 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. Signed-off-by: Robert Baldyga <r.baldyga@samsung.com> --- include/linux/usb/gadget.h | 100 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 94 insertions(+), 6 deletions(-) diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 4f3dfb7..15604bb 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -526,6 +526,9 @@ struct usb_gadget_ops { * @quirk_ep_out_aligned_size: epout requires buffer size to be aligned to * MaxPacketSize. * @is_selfpowered: if the gadget is self-powered. + * @deactivated: True if gadget is deactivated - in deactivated state it cannot + * be connected. + * @connected: True if gadget is connected. * * Gadgets have a mostly-portable "gadget driver" implementing device * functions, handling all usb configurations and interfaces. Gadget @@ -568,6 +571,8 @@ struct usb_gadget { unsigned a_alt_hnp_support:1; unsigned quirk_ep_out_aligned_size:1; unsigned is_selfpowered:1; + unsigned deactivated:1; + unsigned connected:1; }; #define work_to_gadget(w) (container_of((w), struct usb_gadget, work)) @@ -771,9 +776,24 @@ 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); + + if (gadget->deactivated) { + /* + * If gadget is deactivated we only save new state. + * Gadget will be connected automatically after activation. + */ + gadget->connected = true; + return 0; + } + + ret = gadget->ops->pullup(gadget, 1); + if (!ret) + gadget->connected = 1; + return ret; } /** @@ -784,20 +804,88 @@ static inline int usb_gadget_connect(struct usb_gadget *gadget) * as a disconnect (when a VBUS session is active). Not all systems * support software pullup controls. * + * Returns zero on success, else negative errno. + */ +static inline int usb_gadget_disconnect(struct usb_gadget *gadget) +{ + int ret; + + if (!gadget->ops->pullup) + return -EOPNOTSUPP; + + if (gadget->deactivated) { + /* + * If gadget is deactivated we only save new state. + * Gadget will stay disconnected after activation. + */ + gadget->connected = false; + return 0; + } + + ret = gadget->ops->pullup(gadget, 0); + if (!ret) + gadget->connected = 0; + return ret; +} + +/** + * usb_gadget_deactivate - deactivate function which is not ready to work + * @gadget: the peripheral being deactivated + * * This routine may be used during the gadget driver bind() call to prevent * the peripheral from ever being visible to the USB host, unless later - * usb_gadget_connect() is called. For example, user mode components may + * usb_gadget_activate() is called. For example, user mode components may * need to be activated before the system can talk to hosts. * * Returns zero on success, else negative errno. */ -static inline int usb_gadget_disconnect(struct usb_gadget *gadget) +static inline int usb_gadget_deactivate(struct usb_gadget *gadget) { - if (!gadget->ops->pullup) - return -EOPNOTSUPP; - return gadget->ops->pullup(gadget, 0); + int ret; + + if (gadget->deactivated) + return 0; + + if (gadget->connected) { + ret = usb_gadget_disconnect(gadget); + if (ret) + return ret; + /* + * If gadget was being connected before deactivation, we want + * to reconnect it in usb_gadget_activate(). + */ + gadget->connected = true; + } + gadget->deactivated = true; + + return 0; } +/** + * usb_gadget_activate - activate function which is not ready to work + * @gadget: the peripheral being activated + * + * This routine activates gadget which was previously deactivated with + * usb_gadget_deactivate() call. It calls usb_gadget_connect() if needed. + * + * Returns zero on success, else negative errno. + */ +static inline int usb_gadget_activate(struct usb_gadget *gadget) +{ + if (!gadget->deactivated) + return 0; + + gadget->deactivated = false; + + /* + * If gadget has been connected before deactivation, or became connected + * while it was being deactivated, we call usb_gadget_connect(). + */ + if (gadget->connected) + return usb_gadget_connect(gadget); + + return 0; +} /*-------------------------------------------------------------------------*/ -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions 2015-04-07 8:31 ` [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions Robert Baldyga @ 2015-04-28 16:40 ` Felipe Balbi 2015-04-29 9:08 ` Robert Baldyga 0 siblings, 1 reply; 8+ messages in thread From: Felipe Balbi @ 2015-04-28 16:40 UTC (permalink / raw) To: Robert Baldyga Cc: balbi, gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p [-- Attachment #1: Type: text/plain, Size: 6431 bytes --] 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 <balbi@ti.com> 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 <balbi@ti.com> 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 <balbi@ti.com> 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 <balbi@ti.com> 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. cheers -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions 2015-04-28 16:40 ` Felipe Balbi @ 2015-04-29 9:08 ` Robert Baldyga 2015-04-29 15:30 ` Felipe Balbi 0 siblings, 1 reply; 8+ messages in thread From: Robert Baldyga @ 2015-04-29 9:08 UTC (permalink / raw) To: balbi; +Cc: gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p 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 <balbi@ti.com> > 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 <balbi@ti.com> > > 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 <balbi@ti.com> > 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 <balbi@ti.com> > > 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. Thanks, Robert Baldyga ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions 2015-04-29 9:08 ` Robert Baldyga @ 2015-04-29 15:30 ` Felipe Balbi 2015-04-30 9:08 ` Robert Baldyga 0 siblings, 1 reply; 8+ messages in thread From: Felipe Balbi @ 2015-04-29 15:30 UTC (permalink / raw) To: Robert Baldyga Cc: balbi, gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p [-- Attachment #1: Type: text/plain, Size: 8317 bytes --] 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 <balbi@ti.com> > > 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 <balbi@ti.com> > > > > 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 <balbi@ti.com> > > 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 <balbi@ti.com> > > > > 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 ? -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions 2015-04-29 15:30 ` Felipe Balbi @ 2015-04-30 9:08 ` Robert Baldyga 2015-04-30 15:12 ` Felipe Balbi 0 siblings, 1 reply; 8+ messages in thread From: Robert Baldyga @ 2015-04-30 9:08 UTC (permalink / raw) To: balbi; +Cc: gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p 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 <balbi@ti.com> >>> 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 <balbi@ti.com> >>> >>> 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 <balbi@ti.com> >>> 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 <balbi@ti.com> >>> >>> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions 2015-04-30 9:08 ` Robert Baldyga @ 2015-04-30 15:12 ` Felipe Balbi 0 siblings, 0 replies; 8+ messages in thread From: Felipe Balbi @ 2015-04-30 15:12 UTC (permalink / raw) To: Robert Baldyga Cc: balbi, gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p [-- Attachment #1: Type: text/plain, Size: 9092 bytes --] 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_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 <balbi@ti.com> > >>> 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 <balbi@ti.com> > >>> > >>> 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 <balbi@ti.com> > >>> 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 <balbi@ti.com> > >>> > >>> 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. 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. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] usb: composite: fix usb_function_activate/deactivate functions 2015-04-07 8:31 [PATCH 0/2] usb: gadget: Fix gadget deactivaton feature Robert Baldyga 2015-04-07 8:31 ` [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions Robert Baldyga @ 2015-04-07 8:31 ` Robert Baldyga 1 sibling, 0 replies; 8+ messages in thread From: Robert Baldyga @ 2015-04-07 8:31 UTC (permalink / raw) To: balbi Cc: gregkh, linux-usb, linux-kernel, m.szyprowski, andrzej.p, Robert Baldyga Using usb_gadget_disconnect to make gadget temporarily invisible to host doesn't provide desired result, because gadget is connected immediately after binding regardless to previous usb_gadget_disconnect() calls. For this reason we use usb_gadget_deactivate() instead of usb_gadget_disconnect() to make it working as expected. Signed-off-by: Robert Baldyga <r.baldyga@samsung.com> --- drivers/usb/gadget/composite.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 4e3447b..9ed77f4 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -279,7 +279,7 @@ int usb_function_deactivate(struct usb_function *function) spin_lock_irqsave(&cdev->lock, flags); if (cdev->deactivations == 0) - status = usb_gadget_disconnect(cdev->gadget); + status = usb_gadget_deactivate(cdev->gadget); if (status == 0) cdev->deactivations++; @@ -311,7 +311,7 @@ int usb_function_activate(struct usb_function *function) else { cdev->deactivations--; if (cdev->deactivations == 0) - status = usb_gadget_connect(cdev->gadget); + status = usb_gadget_activate(cdev->gadget); } spin_unlock_irqrestore(&cdev->lock, flags); -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-04-30 15:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-07 8:31 [PATCH 0/2] usb: gadget: Fix gadget deactivaton feature Robert Baldyga 2015-04-07 8:31 ` [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions Robert Baldyga 2015-04-28 16:40 ` Felipe Balbi 2015-04-29 9:08 ` Robert Baldyga 2015-04-29 15:30 ` Felipe Balbi 2015-04-30 9:08 ` Robert Baldyga 2015-04-30 15:12 ` Felipe Balbi 2015-04-07 8:31 ` [PATCH 2/2] usb: composite: fix usb_function_activate/deactivate functions Robert Baldyga
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).