Message-ID: <CAPTae5+iPW2Cr-+yodZ26wtSTo=GAwAZn9q39JyAcUBJ1CzmKw@mail.gmail.com>
Date:   Tue, 30 May 2023 21:00:00 -0700
From:   Badhri Jagan Sridharan <badhri@...gle.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     gregkh@...uxfoundation.org, colin.i.king@...il.com,
        xuetao09@...wei.com, quic_eserrao@...cinc.com,
        water.zhangjiantao@...wei.com, peter.chen@...escale.com,
        balbi@...com, francesco@...cini.it, alistair@...stair23.me,
        stephan@...hold.net, bagasdotme@...il.com, luca@...tu.xyz,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org,
        Francesco Dolcini <francesco.dolcini@...adex.com>
Subject: Re: [PATCH v4 3/3] usb: gadget: udc: core: Offload
 usb_udc_vbus_handler processing

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

Powered by blists - more mailing lists