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

usb_udc_vbus_handler() can be invoked from interrupt context by irq
handlers of the gadget drivers, however, usb_udc_connect_control() has
to run in non-atomic context due to the following:
a. Some of the gadget driver implementations expect the ->pullup
   callback to be invoked in non-atomic context.
b. usb_gadget_disconnect() acquires udc_lock which is a mutex.

Hence offload invocation of usb_udc_connect_control()
to workqueue.

Cc: stable@vger.kernel.org
Fixes: 1016fc0c096c ("USB: gadget: Fix obscure lockdep violation for udc_mutex")
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().

Changes since v4:
- Addressing Alan Stern's comments:
** usb_udc_vbus_handler() now offloads invocation of usb_udc_connect_control()
* from workqueue.

** Dropped vbus_events list as this was redundant. Updating to the
* latest value is suffice
---
 drivers/usb/gadget/udc/core.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)


base-commit: 046895105d9666ab56e86ce8dd9786f8003125c6

On Wed, May 31, 2023 at 03:40:26PM -0700, Badhri Jagan Sridharan wrote:
> On Wed, May 31, 2023 at 10:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, May 31, 2023 at 04:02:02AM +0000, Badhri Jagan Sridharan wrote:
> > > usb_udc_connect_control() does not check to see if the udc has already
> > > been started. This causes gadget->ops->pullup to be called through
> > > usb_gadget_connect() when invoked from usb_udc_vbus_handler() even
> > > before usb_gadget_udc_start() is called. Guard this by checking for
> > > udc->started in usb_udc_connect_control() before invoking
> > > usb_gadget_connect().
> >
> > After a merged version of patches 1/3 and 3/3 have been applied, it
> > seems like most of this will not be needed any more.  Maybe not any of
> > it.
> 
> Without the connect_lock introduced in this patch, wouldn't the
> usb_gadget_connect()/
> usb_gadget_disconnect() through soft_connect_store() race against
> usb_gadget_connect()/ usb_gadget_disconnect() through
> usb_udc_connect_control() ?

Okay, yes, that's a good point.  It needs to be mentioned in the patch 
description so that people will understand it is the real reason for 
this change.

Alan Stern

On Wed, May 31, 2023 at 10:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, May 31, 2023 at 04:02:01AM +0000, Badhri Jagan Sridharan wrote:
> > usb_udc_vbus_handler() can be invoked from interrupt context by irq
> > handlers of the gadget drivers, however, usb_udc_connect_control() has
> > to run in non-atomic context due to the following:
> > a. Some of the gadget driver implementations expect the ->pullup
> >    callback to be invoked in non-atomic context.
> > b. usb_gadget_disconnect() acquires udc_lock which is a mutex.
> >
> > Hence offload invocation of usb_udc_connect_control()
> > to workqueue.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 1016fc0c096c ("USB: gadget: Fix obscure lockdep violation for udc_mutex")
> > 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().
> >
> > Changes since v4:
> > - Addressing Alan Stern's comments:
> > ** usb_udc_vbus_handler() now offloads invocation of usb_udc_connect_control()
> > * from workqueue.
> >
> > ** Dropped vbus_events list as this was redundant. Updating to the
> > * latest value is suffice
> > ---
> >  drivers/usb/gadget/udc/core.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index 52e6d2e84e35..44a9f32679b5 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -48,6 +48,7 @@ struct usb_udc {
> >       struct list_head                list;
> >       bool                            vbus;
> >       bool                            started;
> > +     struct work_struct              vbus_work;
> >  };
> >
> >  static struct class *udc_class;
> > @@ -1086,6 +1087,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
> >               usb_gadget_disconnect(udc->gadget);
> >  }
> >
> > +static void vbus_event_work(struct work_struct *work)
> > +{
> > +     struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work);
> > +
> > +     usb_udc_connect_control(udc);
> > +}
> > +
> >  /**
> >   * usb_udc_vbus_handler - updates the udc core vbus status, and try to
> >   * connect or disconnect gadget
> > @@ -1094,6 +1102,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
> >   *
> >   * The udc driver calls it when it wants to connect or disconnect gadget
> >   * according to vbus status.
> > + *
> > + * This function can be invoked from interrupt context by irq handlers of the gadget drivers,
> > + * however, usb_udc_connect_control() has to run in non-atomic context due to the following:
> > + * a. Some of the gadget driver implementations expect the ->pullup callback to be invoked in
> > + * non-atomic context.
> > + * b. usb_gadget_disconnect() acquires udc_lock which is a mutex.
> > + * Hence offload invocation of usb_udc_connect_control() to workqueue.
>
> Comments should be wrapped after about 76 columns (unless there is some
> very good reason not to).

Sounds good ! Have addressed this in v6. Wrapping comments at 76.

>
> >   */
> >  void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
> >  {
> > @@ -1101,7 +1116,7 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
> >
> >       if (udc) {
> >               udc->vbus = status;
> > -             usb_udc_connect_control(udc);
> > +             schedule_work(&udc->vbus_work);
> >       }
> >  }
> >  EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
> > @@ -1328,6 +1343,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
> >       mutex_lock(&udc_lock);
> >       list_add_tail(&udc->list, &udc_list);
> >       mutex_unlock(&udc_lock);
> > +     INIT_WORK(&udc->vbus_work, vbus_event_work);
> >
> >       ret = device_add(&udc->dev);
> >       if (ret)
> > @@ -1558,6 +1574,7 @@ static void gadget_unbind_driver(struct device *dev)
> >
> >       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> >
> > +     cancel_work_sync(&udc->vbus_work);
> >       usb_gadget_disconnect(gadget);
> >       usb_gadget_disable_async_callbacks(udc);
> >       if (gadget->irq)
>
> I'm not in love with this, because there's nothing here to prevent the
> work item from being queued again right after it is cancelled.  Patch
> 3/3 in the series will fix this, but in the meantime this window will
> exist.
>
> Maybe it would be better to merge the 3/3 patch with this one.  They are
> very closely related, after all, since the other patch addresses the
> matter of not allowing the work item to do anything bad at the wrong
> time.

Done ! v6. now squashes 1/3 with 3/3.

Thanks a lot,
Badhri

>
> Alan Stern

On Wed, May 31, 2023 at 3:55 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, May 31, 2023 at 03:40:26PM -0700, Badhri Jagan Sridharan wrote:
> > On Wed, May 31, 2023 at 10:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, May 31, 2023 at 04:02:02AM +0000, Badhri Jagan Sridharan wrote:
> > > > usb_udc_connect_control() does not check to see if the udc has already
> > > > been started. This causes gadget->ops->pullup to be called through
> > > > usb_gadget_connect() when invoked from usb_udc_vbus_handler() even
> > > > before usb_gadget_udc_start() is called. Guard this by checking for
> > > > udc->started in usb_udc_connect_control() before invoking
> > > > usb_gadget_connect().
> > >
> > > After a merged version of patches 1/3 and 3/3 have been applied, it
> > > seems like most of this will not be needed any more.  Maybe not any of
> > > it.
> >
> > Without the connect_lock introduced in this patch, wouldn't the
> > usb_gadget_connect()/
> > usb_gadget_disconnect() through soft_connect_store() race against
> > usb_gadget_connect()/ usb_gadget_disconnect() through
> > usb_udc_connect_control() ?
>
> Okay, yes, that's a good point.  It needs to be mentioned in the patch
> description so that people will understand it is the real reason for
> this change.

Thanks Alan !
I had posted the v6 version of the series with 1/3 and 3/3 of v5
squashed together. I have made changes to address your concerns in v5.
Instead of adding a new lock, I used the connect_lock in this patch to
protect the allow_connect flag.
Eager to know your thoughts !

Regards,
Badhri

>
> Alan Stern