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

Message ID 20230529233700.3706661-3-badhri@google.com
State New
Headers
Series [v3,1/3] Revert "Revert "usb: gadget: udc: core: Invoke usb_gadget_connect only when started"" |

Commit Message

Badhri Jagan Sridharan May 29, 2023, 11:37 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.
---
 drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------
 1 file changed, 124 insertions(+), 145 deletions(-)
  

Comments

Badhri Jagan Sridharan May 29, 2023, 11:39 p.m. UTC | #1
Please disregard v3 as I missed a minor cleanup. Sending v4.

Thanks,
Badhri

On Mon, May 29, 2023 at 4:37 PM Badhri Jagan Sridharan
<badhri@google.com> 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.
>
> ============================================
> 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.
> ---
>  drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------
>  1 file changed, 124 insertions(+), 145 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 4641153e9706..2aa914a0a195 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,26 @@ 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_lock(&udc_lock);
> +       if (!gadget || !gadget->udc) {
> +               //mutex_unlock(&udc_lock);
> +               return;
>         }
> -       mutex_unlock(&udc->connect_lock);
> +       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 +1145,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 +1156,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 +1166,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 +1185,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 +1335,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 +1349,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 +1451,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 +1564,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 +1599,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 +1691,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;
> --
> 2.41.0.rc0.172.g3f132b7071-goog
>
  

Patch

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 4641153e9706..2aa914a0a195 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,26 @@  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_lock(&udc_lock);
+	if (!gadget || !gadget->udc) {
+		//mutex_unlock(&udc_lock);
+		return;
 	}
-	mutex_unlock(&udc->connect_lock);
+	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 +1145,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 +1156,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 +1166,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 +1185,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 +1335,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 +1349,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 +1451,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 +1564,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 +1599,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 +1691,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;