On Mon, May 29, 2023 at 6:08 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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
