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

  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.

Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
Cc: stable <stable@kernel.org>
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
v5 is the first version in this series.
---
 drivers/usb/gadget/udc/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)
  

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".

> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
> 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.

> @@ -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.

Alan Stern
  

On Wed, May 31, 2023 at 10:40 AM Alan Stern <stern@rowland.harvard.edu> 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@kernel.org>
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> > 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