Re: [PATCH 1/2] usb: gadget: add usb_gadget_activate/deactivate functions

From: Felipe Balbi
Date: Wed Apr 29 2015 - 11:31:45 EST

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@xxxxxx>
> > 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@xxxxxx>
> >
> > 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@xxxxxx>
> > 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@xxxxxx>
> >
> > 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: signature.asc
Description: Digital signature