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

  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(-)
  

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
  

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