On Wed, May 31, 2023 at 10:29 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Fixes: 1016fc0c096c ("USB: gadget: Fix obscure lockdep violation for udc_mutex")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> > ---
> > 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
