diff mbox series

[v4,3/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing

Message ID 20230529234816.3720623-3-badhri@google.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/3] Revert "Revert "usb: gadget: udc: core: Invoke usb_gadget_connect only when started"" | expand

Commit Message

Badhri Jagan Sridharan May 29, 2023, 11:48 p.m. UTC 
chipidea udc calls usb_udc_vbus_handler from udc_start gadget
ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler
processing.

============================================
WARNING: possible recursive locking detected
640-rc1-000-devel-00005-gcda3c69ebc14 #1 Not tainted
-------------------------------------------

CPU0
----
lock(&udc->connect_lock);
lock(&udc->connect_lock);

 DEADLOCK

stack backtrace:
  CPU: 1 PID: 566 Comm: echo Not tainted 640-rc1-000-devel-00005-gcda3c69ebc14 #1
  Hardware name: Freescale iMX7 Dual (Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x70/0xb0
  dump_stack_lvl from __lock_acquire+0x924/0x22c4
  __lock_acquire from lock_acquire+0x100/0x370
  lock_acquire from __mutex_lock+0xa8/0xfb4
  __mutex_lock from mutex_lock_nested+0x1c/0x24
  mutex_lock_nested from usb_udc_vbus_handler+0x1c/0x60
  usb_udc_vbus_handler from ci_udc_start+0x74/0x9c
  ci_udc_start from gadget_bind_driver+0x130/0x230
  gadget_bind_driver from really_probe+0xd8/0x3fc
  really_probe from __driver_probe_device+0x94/0x1f0
  __driver_probe_device from driver_probe_device+0x2c/0xc4
  driver_probe_device from __driver_attach+0x114/0x1cc
  __driver_attach from bus_for_each_dev+0x7c/0xcc
  bus_for_each_dev from bus_add_driver+0xd4/0x200
  bus_add_driver from driver_register+0x7c/0x114
  driver_register from usb_gadget_register_driver_owner+0x40/0xe0
  usb_gadget_register_driver_owner from gadget_dev_desc_UDC_store+0xd4/0x110
  gadget_dev_desc_UDC_store from configfs_write_iter+0xac/0x118
  configfs_write_iter from vfs_write+0x1b4/0x40c
  vfs_write from ksys_write+0x70/0xf8
  ksys_write from ret_fast_syscall+0x0/0x1c

Fixes: 0db213ea8eed ("Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup""")
Cc: stable@vger.kernel.org
Reported-by: Stephan Gerhold <stephan@gerhold.net>
Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/
Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/
Reported-by: Alistair <alistair@alistair23.me>
Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@alistair23.me/
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
Changes since v1:
- Address Alan Stern's comment on usb_udc_vbus_handler invocation from
  atomic context:
* vbus_events_lock is now a spinlock and allocations in
* usb_udc_vbus_handler are atomic now.

Changes since v2:
- Addressing Alan Stern's comments:
** connect_lock is now held by callers of
* usb_gadget_pullup_update_locked() and gadget_(un)bind_driver() does
* notdirectly hold the lock.

** Both usb_gadget_(dis)connect() and usb_udc_vbus_handler() would
* set/clear udc->vbus and invoke usb_gadget_pullup_update_locked.

** Add "unbinding" to prevent new connections after the gadget is being
* unbound.

Changes since v3:
** Made a minor cleanup which I missed to do in v3 in
* usb_udc_vbus_handler().
---
 drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------
 1 file changed, 123 insertions(+), 146 deletions(-)

Comments

Alan Stern May 30, 2023, 1:08 a.m. UTC | #1 
On Mon, May 29, 2023 at 11:48:16PM +0000, Badhri Jagan Sridharan wrote:
> chipidea udc calls usb_udc_vbus_handler from udc_start gadget
> ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler
> processing.

This is not a good explanation.  In particular, it doesn't explain why 
moving the processing to a workqueue is the proper solution.

You should describe the issue I raised earlier, namely, that 
usb_udc_vbus_handler() has to run in interrupt context but it calls 
usb_udc_connect_control(), which has to run in process context.  And 
explain _why_ these routines have to run in those contexts.

> ---
>  drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------
>  1 file changed, 123 insertions(+), 146 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 4641153e9706..26380e621e9f 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -38,9 +38,10 @@ static const struct bus_type gadget_bus_type;
>   * for udcs who do not care about vbus status, this value is always true
>   * @started: the UDC's started state. True if the UDC had started.
>   * @connect_lock: protects udc->vbus, udc->started, gadget->connect, gadget->deactivate related
> - * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked,
> - * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are
> - * called with this lock held.
> + * functions. usb_gadget_pullup_update_locked called with this lock held.
> + * @vbus_events: list head for processing vbus updates on usb_udc_vbus_handler.
> + * @vbus_events_lock: protects vbus_events list
> + * @vbus_work: work item that invokes usb_gadget_pullup_update_locked.
>   *
>   * This represents the internal data structure which is used by the UDC-class
>   * to hold information about udc driver and gadget together.
> @@ -53,6 +54,20 @@ struct usb_udc {
>  	bool				vbus;
>  	bool				started;
>  	struct mutex			connect_lock;
> +	struct list_head		vbus_events;
> +	spinlock_t			vbus_events_lock;
> +	struct work_struct		vbus_work;

Do you really need three new fields here?  Isn't vbus_work sufficient?

> +	bool				unbinding;

Do not include this in the same patch.  The unbinding flag does 
something different from the vbus_work structure, so it belongs in a 
different patch.

> +};
> +
> +/**
> + * struct vbus_event - used to notify vbus updates posted through usb_udc_vbus_handler.
> + * @vbus_on: true when vbus is on. false other wise.
> + * @node: list node for maintaining a list of pending updates to be processed.
> + */
> +struct vbus_event {
> +	bool vbus_on;
> +	struct list_head node;
>  };

Why do we need this?  Why can't the work routine simply set the pullup 
according to the current setting of vbus and the other flags?  That's 
what usb_udc_vbus_handler() does now.

>  
>  static struct class *udc_class;
> @@ -693,40 +708,46 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
>  EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
>  
>  /* Internal version of usb_gadget_connect needs to be called with connect_lock held. */
> -static int usb_gadget_connect_locked(struct usb_gadget *gadget)
> +static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
>  	__must_hold(&gadget->udc->connect_lock)
>  {
>  	int ret = 0;
> +	bool connect = !gadget->deactivated && gadget->udc->started && gadget->udc->vbus &&
> +		       !gadget->udc->unbinding;

Since you are wrapping this line anyway, you might as well wrap it 
before column 76.

>  
>  	if (!gadget->ops->pullup) {
>  		ret = -EOPNOTSUPP;
>  		goto out;
>  	}
>  
> -	if (gadget->connected)
> -		goto out;
> -
> -	if (gadget->deactivated || !gadget->udc->started) {
> -		/*
> -		 * If gadget is deactivated we only save new state.
> -		 * Gadget will be connected automatically after activation.
> -		 *
> -		 * udc first needs to be started before gadget can be pulled up.
> -		 */
> -		gadget->connected = true;
> -		goto out;
> +	if (connect != gadget->connected) {
> +		ret = gadget->ops->pullup(gadget, connect);
> +		if (!ret)
> +			gadget->connected = connect;
> +		mutex_lock(&udc_lock);
> +		if (!connect)
> +			gadget->udc->driver->disconnect(gadget);
> +		mutex_unlock(&udc_lock);
>  	}

What will happen if the gadget isn't deactivated, but it is started and 
VBUS power is prevent and the driver isn't unbinding, but the gadget 
driver decides to call usb_gadget_disconnect()?

>  
> -	ret = gadget->ops->pullup(gadget, 1);
> -	if (!ret)
> -		gadget->connected = 1;
> -
>  out:
>  	trace_usb_gadget_connect(gadget, ret);
>  
>  	return ret;
>  }
>  
> +static int usb_gadget_set_vbus(struct usb_gadget *gadget, bool vbus)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->udc->connect_lock);
> +	gadget->udc->vbus = vbus;

Why does this have to be here?  What's wrong with setting vbus in 
interrupt context?

> +	ret = usb_gadget_pullup_update_locked(gadget);
> +	mutex_unlock(&gadget->udc->connect_lock);

Sorry, but at this point I'm getting tired of pointing out all the 
problems in this patch, so I'm going to stop here.

How about instead doing something really simple, like just make 
usb_udc_vbus_handler() queue up a work routine that calls 
usb_udc_connect_control()?  Just a minimal change that will be really 
easy to review.

Alan Stern
Badhri Jagan Sridharan May 31, 2023, 4 a.m. UTC | #2 
On Mon, May 29, 2023 at 6:08 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, May 29, 2023 at 11:48:16PM +0000, Badhri Jagan Sridharan wrote:
> > chipidea udc calls usb_udc_vbus_handler from udc_start gadget
> > ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler
> > processing.
>
> This is not a good explanation.  In particular, it doesn't explain why
> moving the processing to a workqueue is the proper solution.
>
> You should describe the issue I raised earlier, namely, that
> usb_udc_vbus_handler() has to run in interrupt context but it calls
> usb_udc_connect_control(), which has to run in process context.  And
> explain _why_ these routines have to run in those contexts.
>
> > ---
> >  drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------
> >  1 file changed, 123 insertions(+), 146 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index 4641153e9706..26380e621e9f 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -38,9 +38,10 @@ static const struct bus_type gadget_bus_type;
> >   * for udcs who do not care about vbus status, this value is always true
> >   * @started: the UDC's started state. True if the UDC had started.
> >   * @connect_lock: protects udc->vbus, udc->started, gadget->connect, gadget->deactivate related
> > - * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked,
> > - * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are
> > - * called with this lock held.
> > + * functions. usb_gadget_pullup_update_locked called with this lock held.
> > + * @vbus_events: list head for processing vbus updates on usb_udc_vbus_handler.
> > + * @vbus_events_lock: protects vbus_events list
> > + * @vbus_work: work item that invokes usb_gadget_pullup_update_locked.
> >   *
> >   * This represents the internal data structure which is used by the UDC-class
> >   * to hold information about udc driver and gadget together.
> > @@ -53,6 +54,20 @@ struct usb_udc {
> >       bool                            vbus;
> >       bool                            started;
> >       struct mutex                    connect_lock;
> > +     struct list_head                vbus_events;
> > +     spinlock_t                      vbus_events_lock;
> > +     struct work_struct              vbus_work;
>
> Do you really need three new fields here?  Isn't vbus_work sufficient?

Ack. Just the vbus_work is sufficient as vbus can be updated to the
latest value.
Addressing in v5.

>
> > +     bool                            unbinding;
>
> Do not include this in the same patch.  The unbinding flag does
> something different from the vbus_work structure, so it belongs in a
> different patch.

Sure, uploaded as a separate patch in v5.
However, I named it allow_start instead as I believe that UDC should
neither be started nor pulled up when unbound.
Let me know your thoughts in v5 !

>
> > +};
> > +
> > +/**
> > + * struct vbus_event - used to notify vbus updates posted through usb_udc_vbus_handler.
> > + * @vbus_on: true when vbus is on. false other wise.
> > + * @node: list node for maintaining a list of pending updates to be processed.
> > + */
> > +struct vbus_event {
> > +     bool vbus_on;
> > +     struct list_head node;
> >  };
>
> Why do we need this?  Why can't the work routine simply set the pullup
> according to the current setting of vbus and the other flags?  That's
> what usb_udc_vbus_handler() does now.

Ack. Dropping vbus_event and related fields.
>
> >
> >  static struct class *udc_class;
> > @@ -693,40 +708,46 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
> >  EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
> >
> >  /* Internal version of usb_gadget_connect needs to be called with connect_lock held. */
> > -static int usb_gadget_connect_locked(struct usb_gadget *gadget)
> > +static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
> >       __must_hold(&gadget->udc->connect_lock)
> >  {
> >       int ret = 0;
> > +     bool connect = !gadget->deactivated && gadget->udc->started && gadget->udc->vbus &&
> > +                    !gadget->udc->unbinding;
>
> Since you are wrapping this line anyway, you might as well wrap it
> before column 76.
>
> >
> >       if (!gadget->ops->pullup) {
> >               ret = -EOPNOTSUPP;
> >               goto out;
> >       }
> >
> > -     if (gadget->connected)
> > -             goto out;
> > -
> > -     if (gadget->deactivated || !gadget->udc->started) {
> > -             /*
> > -              * If gadget is deactivated we only save new state.
> > -              * Gadget will be connected automatically after activation.
> > -              *
> > -              * udc first needs to be started before gadget can be pulled up.
> > -              */
> > -             gadget->connected = true;
> > -             goto out;
> > +     if (connect != gadget->connected) {
> > +             ret = gadget->ops->pullup(gadget, connect);
> > +             if (!ret)
> > +                     gadget->connected = connect;
> > +             mutex_lock(&udc_lock);
> > +             if (!connect)
> > +                     gadget->udc->driver->disconnect(gadget);
> > +             mutex_unlock(&udc_lock);
> >       }
>
> What will happen if the gadget isn't deactivated, but it is started and
> VBUS power is prevent and the driver isn't unbinding, but the gadget
> driver decides to call usb_gadget_disconnect()?

Simplified as you recommended to directly call
usb_udc_connect_control() from the work item. So, this is no longer an
issue.

>
> >
> > -     ret = gadget->ops->pullup(gadget, 1);
> > -     if (!ret)
> > -             gadget->connected = 1;
> > -
> >  out:
> >       trace_usb_gadget_connect(gadget, ret);
> >
> >       return ret;
> >  }
> >
> > +static int usb_gadget_set_vbus(struct usb_gadget *gadget, bool vbus)
> > +{
> > +     int ret;
> > +
> > +     mutex_lock(&gadget->udc->connect_lock);
> > +     gadget->udc->vbus = vbus;
>
> Why does this have to be here?  What's wrong with setting vbus in
> interrupt context?
>
> > +     ret = usb_gadget_pullup_update_locked(gadget);
> > +     mutex_unlock(&gadget->udc->connect_lock);
>
> Sorry, but at this point I'm getting tired of pointing out all the
> problems in this patch, so I'm going to stop here.
>
> How about instead doing something really simple, like just make
> usb_udc_vbus_handler() queue up a work routine that calls
> usb_udc_connect_control()?  Just a minimal change that will be really
> easy to review.

Ack. v5 now does this.

Thanks for all the feedback,
Badhri
>
> Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 4641153e9706..26380e621e9f 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -38,9 +38,10 @@  static const struct bus_type gadget_bus_type;
  * for udcs who do not care about vbus status, this value is always true
  * @started: the UDC's started state. True if the UDC had started.
  * @connect_lock: protects udc->vbus, udc->started, gadget->connect, gadget->deactivate related
- * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked,
- * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are
- * called with this lock held.
+ * functions. usb_gadget_pullup_update_locked called with this lock held.
+ * @vbus_events: list head for processing vbus updates on usb_udc_vbus_handler.
+ * @vbus_events_lock: protects vbus_events list
+ * @vbus_work: work item that invokes usb_gadget_pullup_update_locked.
  *
  * This represents the internal data structure which is used by the UDC-class
  * to hold information about udc driver and gadget together.
@@ -53,6 +54,20 @@  struct usb_udc {
 	bool				vbus;
 	bool				started;
 	struct mutex			connect_lock;
+	struct list_head		vbus_events;
+	spinlock_t			vbus_events_lock;
+	struct work_struct		vbus_work;
+	bool				unbinding;
+};
+
+/**
+ * struct vbus_event - used to notify vbus updates posted through usb_udc_vbus_handler.
+ * @vbus_on: true when vbus is on. false other wise.
+ * @node: list node for maintaining a list of pending updates to be processed.
+ */
+struct vbus_event {
+	bool vbus_on;
+	struct list_head node;
 };
 
 static struct class *udc_class;
@@ -693,40 +708,46 @@  int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
 EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
 
 /* Internal version of usb_gadget_connect needs to be called with connect_lock held. */
-static int usb_gadget_connect_locked(struct usb_gadget *gadget)
+static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
 	__must_hold(&gadget->udc->connect_lock)
 {
 	int ret = 0;
+	bool connect = !gadget->deactivated && gadget->udc->started && gadget->udc->vbus &&
+		       !gadget->udc->unbinding;
 
 	if (!gadget->ops->pullup) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
 
-	if (gadget->connected)
-		goto out;
-
-	if (gadget->deactivated || !gadget->udc->started) {
-		/*
-		 * If gadget is deactivated we only save new state.
-		 * Gadget will be connected automatically after activation.
-		 *
-		 * udc first needs to be started before gadget can be pulled up.
-		 */
-		gadget->connected = true;
-		goto out;
+	if (connect != gadget->connected) {
+		ret = gadget->ops->pullup(gadget, connect);
+		if (!ret)
+			gadget->connected = connect;
+		mutex_lock(&udc_lock);
+		if (!connect)
+			gadget->udc->driver->disconnect(gadget);
+		mutex_unlock(&udc_lock);
 	}
 
-	ret = gadget->ops->pullup(gadget, 1);
-	if (!ret)
-		gadget->connected = 1;
-
 out:
 	trace_usb_gadget_connect(gadget, ret);
 
 	return ret;
 }
 
+static int usb_gadget_set_vbus(struct usb_gadget *gadget, bool vbus)
+{
+	int ret;
+
+	mutex_lock(&gadget->udc->connect_lock);
+	gadget->udc->vbus = vbus;
+	ret = usb_gadget_pullup_update_locked(gadget);
+	mutex_unlock(&gadget->udc->connect_lock);
+
+	return ret;
+}
+
 /**
  * usb_gadget_connect - software-controlled connect to USB host
  * @gadget:the peripheral being connected
@@ -739,56 +760,10 @@  static int usb_gadget_connect_locked(struct usb_gadget *gadget)
  */
 int usb_gadget_connect(struct usb_gadget *gadget)
 {
-	int ret;
-
-	mutex_lock(&gadget->udc->connect_lock);
-	ret = usb_gadget_connect_locked(gadget);
-	mutex_unlock(&gadget->udc->connect_lock);
-
-	return ret;
+	return usb_gadget_set_vbus(gadget, true);
 }
 EXPORT_SYMBOL_GPL(usb_gadget_connect);
 
-/* Internal version of usb_gadget_disconnect needs to be called with connect_lock held. */
-static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
-	__must_hold(&gadget->udc->connect_lock)
-{
-	int ret = 0;
-
-	if (!gadget->ops->pullup) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
-
-	if (!gadget->connected)
-		goto out;
-
-	if (gadget->deactivated || !gadget->udc->started) {
-		/*
-		 * If gadget is deactivated we only save new state.
-		 * Gadget will stay disconnected after activation.
-		 *
-		 * udc should have been started before gadget being pulled down.
-		 */
-		gadget->connected = false;
-		goto out;
-	}
-
-	ret = gadget->ops->pullup(gadget, 0);
-	if (!ret)
-		gadget->connected = 0;
-
-	mutex_lock(&udc_lock);
-	if (gadget->udc->driver)
-		gadget->udc->driver->disconnect(gadget);
-	mutex_unlock(&udc_lock);
-
-out:
-	trace_usb_gadget_disconnect(gadget, ret);
-
-	return ret;
-}
-
 /**
  * usb_gadget_disconnect - software-controlled disconnect from USB host
  * @gadget:the peripheral being disconnected
@@ -803,16 +778,22 @@  static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
  * Returns zero on success, else negative errno.
  */
 int usb_gadget_disconnect(struct usb_gadget *gadget)
+{
+	return usb_gadget_set_vbus(gadget, false);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
+
+static int usb_gadget_set_deactivate(struct usb_gadget *gadget, bool deactivate)
 {
 	int ret;
 
 	mutex_lock(&gadget->udc->connect_lock);
-	ret = usb_gadget_disconnect_locked(gadget);
+	gadget->deactivated = deactivate;
+	ret = usb_gadget_pullup_update_locked(gadget);
 	mutex_unlock(&gadget->udc->connect_lock);
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
 
 /**
  * usb_gadget_deactivate - deactivate function which is not ready to work
@@ -829,26 +810,7 @@  int usb_gadget_deactivate(struct usb_gadget *gadget)
 {
 	int ret = 0;
 
-	if (gadget->deactivated)
-		goto out;
-
-	mutex_lock(&gadget->udc->connect_lock);
-	if (gadget->connected) {
-		ret = usb_gadget_disconnect_locked(gadget);
-		if (ret)
-			goto unlock;
-
-		/*
-		 * If gadget was being connected before deactivation, we want
-		 * to reconnect it in usb_gadget_activate().
-		 */
-		gadget->connected = true;
-	}
-	gadget->deactivated = true;
-
-unlock:
-	mutex_unlock(&gadget->udc->connect_lock);
-out:
+	ret = usb_gadget_set_deactivate(gadget, true);
 	trace_usb_gadget_deactivate(gadget, ret);
 
 	return ret;
@@ -868,21 +830,7 @@  int usb_gadget_activate(struct usb_gadget *gadget)
 {
 	int ret = 0;
 
-	if (!gadget->deactivated)
-		goto out;
-
-	mutex_lock(&gadget->udc->connect_lock);
-	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)
-		ret = usb_gadget_connect_locked(gadget);
-	mutex_unlock(&gadget->udc->connect_lock);
-
-out:
+	ret = usb_gadget_set_deactivate(gadget, false);
 	trace_usb_gadget_activate(gadget, ret);
 
 	return ret;
@@ -1121,13 +1069,28 @@  EXPORT_SYMBOL_GPL(usb_gadget_set_state);
 
 /* ------------------------------------------------------------------------- */
 
-/* Acquire connect_lock before calling this function. */
-static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
+static void vbus_event_work(struct work_struct *work)
 {
-	if (udc->vbus && udc->started)
-		usb_gadget_connect_locked(udc->gadget);
-	else
-		usb_gadget_disconnect_locked(udc->gadget);
+	struct vbus_event *event, *n;
+	struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work);
+	unsigned long flags;
+
+	spin_lock_irqsave(&udc->vbus_events_lock, flags);
+	list_for_each_entry_safe(event, n, &udc->vbus_events, node) {
+		list_del(&event->node);
+		/* OK to drop the lock here as it suffice to syncrhronize udc->vbus_events node
+		 * retrieval and deletion against usb_udc_vbus_handler. usb_udc_vbus_handler does
+		 * list_add_tail so n would be the same even if the lock is dropped.
+		 */
+		spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
+		mutex_lock(&udc->connect_lock);
+		udc->vbus = event->vbus_on;
+		usb_gadget_pullup_update_locked(udc->gadget);
+		kfree(event);
+		mutex_unlock(&udc->connect_lock);
+		spin_lock_irqsave(&udc->vbus_events_lock, flags);
+	}
+	spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
 }
 
 /**
@@ -1141,14 +1104,24 @@  static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc
  */
 void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
 {
-	struct usb_udc *udc = gadget->udc;
+	struct usb_udc *udc;
+	struct vbus_event *vbus_event;
+	unsigned long flags;
 
-	mutex_lock(&udc->connect_lock);
-	if (udc) {
-		udc->vbus = status;
-		usb_udc_connect_control_locked(udc);
-	}
-	mutex_unlock(&udc->connect_lock);
+	if (!gadget || !gadget->udc)
+		return;
+
+	udc = gadget->udc;
+
+	vbus_event = kzalloc(sizeof(*vbus_event), GFP_ATOMIC);
+	if (!vbus_event)
+		return;
+
+	spin_lock_irqsave(&udc->vbus_events_lock, flags);
+	vbus_event->vbus_on = status;
+	list_add_tail(&vbus_event->node, &udc->vbus_events);
+	spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
+	schedule_work(&udc->vbus_work);
 }
 EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
 
@@ -1170,7 +1143,7 @@  void usb_gadget_udc_reset(struct usb_gadget *gadget,
 EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
 
 /**
- * usb_gadget_udc_start_locked - tells usb device controller to start up
+ * usb_gadget_udc_start - tells usb device controller to start up
  * @udc: The UDC to be started
  *
  * This call is issued by the UDC Class driver when it's about
@@ -1181,11 +1154,8 @@  EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
  * necessary to have it powered on.
  *
  * Returns zero on success, else negative errno.
- *
- * Caller should acquire connect_lock before invoking this function.
  */
-static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
-	__must_hold(&udc->connect_lock)
+static inline int usb_gadget_udc_start(struct usb_udc *udc)
 {
 	int ret;
 
@@ -1194,15 +1164,17 @@  static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
 		return -EBUSY;
 	}
 
+	mutex_lock(&udc->connect_lock);
 	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
 	if (!ret)
 		udc->started = true;
+	mutex_unlock(&udc->connect_lock);
 
 	return ret;
 }
 
 /**
- * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore
+ * usb_gadget_udc_stop - tells usb device controller we don't need it anymore
  * @udc: The UDC to be stopped
  *
  * This call is issued by the UDC Class driver after calling
@@ -1211,19 +1183,18 @@  static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
  * The details are implementation specific, but it can go as
  * far as powering off UDC completely and disable its data
  * line pullups.
- *
- * Caller should acquire connect lock before invoking this function.
  */
-static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc)
-	__must_hold(&udc->connect_lock)
+static inline void usb_gadget_udc_stop(struct usb_udc *udc)
 {
 	if (!udc->started) {
 		dev_err(&udc->dev, "UDC had already stopped\n");
 		return;
 	}
 
+	mutex_lock(&udc->connect_lock);
 	udc->gadget->ops->udc_stop(udc->gadget);
 	udc->started = false;
+	mutex_unlock(&udc->connect_lock);
 }
 
 /**
@@ -1362,6 +1333,7 @@  int usb_add_gadget(struct usb_gadget *gadget)
 	if (!udc)
 		goto error;
 
+	udc->unbinding = true;
 	device_initialize(&udc->dev);
 	udc->dev.release = usb_udc_release;
 	udc->dev.class = udc_class;
@@ -1375,6 +1347,9 @@  int usb_add_gadget(struct usb_gadget *gadget)
 	udc->gadget = gadget;
 	gadget->udc = udc;
 	mutex_init(&udc->connect_lock);
+	INIT_LIST_HEAD(&udc->vbus_events);
+	spin_lock_init(&udc->vbus_events_lock);
+	INIT_WORK(&udc->vbus_work, vbus_event_work);
 
 	udc->started = false;
 
@@ -1474,6 +1449,17 @@  char *usb_get_gadget_udc_name(void)
 }
 EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name);
 
+static int usb_gadget_set_unbinding(struct usb_udc *udc, bool status)
+{
+	int ret;
+
+	mutex_lock(&udc->connect_lock);
+	udc->unbinding = status;
+	ret = usb_gadget_pullup_update_locked(udc->gadget);
+	mutex_unlock(&udc->connect_lock);
+
+	return ret;
+}
 /**
  * usb_add_gadget_udc - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller
@@ -1576,15 +1562,11 @@  static int gadget_bind_driver(struct device *dev)
 	if (ret)
 		goto err_bind;
 
-	mutex_lock(&udc->connect_lock);
-	ret = usb_gadget_udc_start_locked(udc);
-	if (ret) {
-		mutex_unlock(&udc->connect_lock);
+	ret = usb_gadget_udc_start(udc);
+	if (ret)
 		goto err_start;
-	}
 	usb_gadget_enable_async_callbacks(udc);
-	usb_udc_connect_control_locked(udc);
-	mutex_unlock(&udc->connect_lock);
+	usb_gadget_set_unbinding(udc, false);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
@@ -1615,14 +1597,13 @@  static void gadget_unbind_driver(struct device *dev)
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
-	mutex_lock(&udc->connect_lock);
-	usb_gadget_disconnect_locked(gadget);
+	usb_gadget_set_unbinding(udc, true);
+	cancel_work_sync(&udc->vbus_work);
 	usb_gadget_disable_async_callbacks(udc);
 	if (gadget->irq)
 		synchronize_irq(gadget->irq);
 	udc->driver->unbind(gadget);
-	usb_gadget_udc_stop_locked(udc);
-	mutex_unlock(&udc->connect_lock);
+	usb_gadget_udc_stop(udc);
 
 	mutex_lock(&udc_lock);
 	driver->is_bound = false;
@@ -1708,15 +1689,11 @@  static ssize_t soft_connect_store(struct device *dev,
 	}
 
 	if (sysfs_streq(buf, "connect")) {
-		mutex_lock(&udc->connect_lock);
-		usb_gadget_udc_start_locked(udc);
-		usb_gadget_connect_locked(udc->gadget);
-		mutex_unlock(&udc->connect_lock);
+		usb_gadget_udc_start(udc);
+		usb_udc_vbus_handler(udc->gadget, true);
 	} else if (sysfs_streq(buf, "disconnect")) {
-		mutex_lock(&udc->connect_lock);
-		usb_gadget_disconnect_locked(udc->gadget);
-		usb_gadget_udc_stop_locked(udc);
-		mutex_unlock(&udc->connect_lock);
+		usb_udc_vbus_handler(udc->gadget, false);
+		usb_gadget_udc_stop(udc);
 	} else {
 		dev_err(dev, "unsupported command '%s'\n", buf);
 		ret = -EINVAL;