Re: [PATCH v5 3/3] usb: gadget: udc: core: Prevent UDC from starting when unbound

[Badhri Jagan Sridharan]

On Wed, May 31, 2023 at 10:40 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, May 31, 2023 at 04:02:03AM +0000, Badhri Jagan Sridharan wrote:
> > UDC should neither be started nor pulled up unless the gadget driver is
> > bound. The new flag "allow_start" is now set by gadget_bind_driver()
> > and cleared by gadget_unbind_driver(). usb_gadget_udc_start_locked()
> > now checks whether allow_start is set before starting the UDC by
> > invoking the ->udc_start() callback.
>
> "allow_start" isn't quite the right name either, because there is a
> short time interval during which the UDC is started but we don't want
> to allow it to connect to the host (this occurs in
> gadget_unbind_driver() between the disconnect call and the
> usb_gadge_udc_stop call).  A more accurate name would be "allow_connect"
> or "allow_pullup".

Sure, Have renamed the flag to "allow_connect".

>
> > Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> > Cc: stable <stable@xxxxxxxxxx>
> > Signed-off-by: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> > ---
> > v5 is the first version in this series.
> > ---
> >  drivers/usb/gadget/udc/core.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index 6ffe5fda8bb7..ac9d6186815d 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -37,6 +37,8 @@ static const struct bus_type gadget_bus_type;
> >   * @vbus: for udcs who care about vbus status, this value is real vbus status;
> >   * 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.
> > + * @allow_start: Indicates whether UDC is allowed to start. Set/cleared by gadget_(un)bind_driver()
> > + * after gadget driver is bound or unbound.
> >   * @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
>
> As before, wrap the comments around column 76.

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

>
> > @@ -52,6 +54,7 @@ struct usb_udc {
> >       struct list_head                list;
> >       bool                            vbus;
> >       bool                            started;
> > +     bool                            allow_start;
> >       struct work_struct              vbus_work;
> >       struct mutex                    connect_lock;
> >  };
> > @@ -1204,6 +1207,9 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
> >       if (udc->started) {
> >               dev_err(&udc->dev, "UDC had already started\n");
> >               return -EBUSY;
> > +     } else if (!udc->allow_start) {
> > +             dev_err(&udc->dev, "UDC not allowed to start. Is gadget driver bound ?\n");
> > +             return -EIO;
> >       }
>
> This isn't the right test or the right place.  We want to prevent the
> UDC from connecting to the host, not prevent it from starting.
>
> >
> >       ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
> > @@ -1590,6 +1596,7 @@ static int gadget_bind_driver(struct device *dev)
> >               goto err_bind;
> >
> >       mutex_lock(&udc->connect_lock);
> > +     udc->allow_start = true;
> >       ret = usb_gadget_udc_start_locked(udc);
> >       if (ret) {
> >               mutex_unlock(&udc->connect_lock);
> > @@ -1630,6 +1637,7 @@ static void gadget_unbind_driver(struct device *dev)
> >
> >       cancel_work_sync(&udc->vbus_work);
> >       mutex_lock(&udc->connect_lock);
> > +     udc->allow_start = false;
> >       usb_gadget_disconnect_locked(gadget);
> >       usb_gadget_disable_async_callbacks(udc);
> >       if (gadget->irq)
>
> Here is where the problem about the vbus work item getting requeued can
> be fixed.  Clear the allow_connect flag before the call to
> cancel_work_sync().  That way, even if the vbus work item gets queued
> again after it is cancelled, when it does run it won't do anything.
>
> Although, come to think of it, there is still the problem of making sure
> that the work item doesn't run after the udc has been deallocated.  It
> looks like you will also need to cancel the work item at the end of
> usb_del_gadget().  At that point the UDC has already been stopped, so it
> won't call usb_hcd_vbus_handler() again.

Sure, Have made these changes in V6.

Thanks a lot,
Badhri

>
> Alan Stern