[v6,2/2] usb: gadget: udc: core: Prevent soft_connect_store() race

Message ID 20230601031028.544244-2-badhri@google.com
State New
Headers
Series [v6,1/2] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing |

Commit Message

Badhri Jagan Sridharan June 1, 2023, 3:10 a.m. UTC
  usb_udc_connect_control(), soft_connect_store() and
usb_gadget_deactivate() can potentialy race against each other to invoke
usb_gadget_connect()/usb_gadget_disconnect(). To prevent this, guarding
udc->vbus, udc->started, gadget->allow_connect, gadget->deactivate with
connect_lock so that ->pullup() is only invoked when gadget driver is
bound, not deactivated and started. 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.

This commit was reverted due to the crash reported in
https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/.
commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing")
addresses the crash reported.

Cc: stable@vger.kernel.org
Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
v5 is the first version in this series.
Changes since v5:
** Fixed up commit message
** Wrapped comments at 76.
---
 drivers/usb/gadget/udc/core.c | 151 ++++++++++++++++++++++++----------
 1 file changed, 109 insertions(+), 42 deletions(-)
  

Comments

Alan Stern June 7, 2023, 6:26 p.m. UTC | #1
This patch looks basically okay.  Most of the comments below are 
concerned only with style or presentation.

On Thu, Jun 01, 2023 at 03:10:28AM +0000, Badhri Jagan Sridharan wrote:
> usb_udc_connect_control(), soft_connect_store() and
> usb_gadget_deactivate() can potentialy race against each other to invoke

"potentially" has two 'l's.

> usb_gadget_connect()/usb_gadget_disconnect(). To prevent this, guarding

s/guarding/guard/

> udc->vbus, udc->started, gadget->allow_connect, gadget->deactivate with

Insert "and" before "gadget->deactivate".

Also, I don't think the patch guards udc->vbus.  After all, that flag 
can be changed in usb_udc_vbus_handler() without the protection of the 
mutex.

> connect_lock so that ->pullup() is only invoked when gadget driver is
> bound, not deactivated and started.

"when the gadget", not "when gadget driver".  The driver isn't what gets 
deactivated or started; the gadget is.

This would be easier to understand if the last two items were switched:

	bound, started, and not deactivated.

Also, it would help readers if there were some additional text to help 
separate the end of this sentence from the start of the next one.  For 
example, you might insert "The routines" at the beginning of the next 
sentence.

>  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.
> 
> This commit was reverted due to the crash reported in

An earlier version of this commit...

> https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/.
> commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing")
> addresses the crash reported.
> 
> Cc: stable@vger.kernel.org
> Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
> v5 is the first version in this series.
> Changes since v5:
> ** Fixed up commit message
> ** Wrapped comments at 76.
> ---
>  drivers/usb/gadget/udc/core.c | 151 ++++++++++++++++++++++++----------
>  1 file changed, 109 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index d2e4f78c53e3..b53f74fcad60 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -40,6 +40,11 @@ static const struct bus_type gadget_bus_type;
>   * @allow_connect: Indicates whether UDC is allowed to be pulled up.
>   * 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. usb_gadget_connect_locked,

Again, separate the two sentences with some extra text.  Otherwise the 
period looks too much like a comma for people to see it easily.

> + * 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.
>   *
>   * This represents the internal data structure which is used by the UDC-class
>   * to hold information about udc driver and gadget together.
> @@ -53,6 +58,7 @@ struct usb_udc {
>  	bool				started;
>  	bool				allow_connect;
>  	struct work_struct		vbus_work;
> +	struct mutex			connect_lock;
>  };
>  
>  static struct class *udc_class;
> @@ -692,17 +698,12 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
>  
> -/**
> - * usb_gadget_connect - software-controlled connect to USB host
> - * @gadget:the peripheral being connected
> - *
> - * Enables the D+ (or potentially D-) pullup.  The host will start
> - * enumerating this gadget when the pullup is active and a VBUS session
> - * is active (the link is powered).
> - *
> - * Returns zero on success, else negative errno.
> +/*
> + * Internal version of usb_gadget_connect needs to be called with
> + * connect_lock held.

I'm not sure you need to say this; it's pretty obvious from the 
__must_hold() annotation a few lines later.  Consider removing this 
paragraph and the similar paragraphs in the other new internal routines.

>   */
> -int usb_gadget_connect(struct usb_gadget *gadget)
> +static int usb_gadget_connect_locked(struct usb_gadget *gadget)
> +	__must_hold(&gadget->udc->connect_lock)
>  {
>  	int ret = 0;
>  
> @@ -711,10 +712,12 @@ int usb_gadget_connect(struct usb_gadget *gadget)
>  		goto out;
>  	}
>  
> -	if (gadget->deactivated || !gadget->udc->allow_connect) {
> +	if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) {
>  		/*
>  		 * If gadget is deactivated we only save new state.
>  		 * Gadget will be connected automatically after activation.

This comment is now inaccurate.  Change it to say:

		 * If the gadget isn't usable (because it is deactivated,
		 * unbound, or not yet started), we only save the new state.
		 * The gadget will be connected automatically when it is
		 * activated/bound/started.

> +		 *
> +		 * udc first needs to be started before gadget can be pulled up.

And then this sentence won't be needed.

>  		 */
>  		gadget->connected = true;
>  		goto out;
> @@ -729,22 +732,35 @@ int usb_gadget_connect(struct usb_gadget *gadget)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(usb_gadget_connect);
>  
>  /**
> - * usb_gadget_disconnect - software-controlled disconnect from USB host
> - * @gadget:the peripheral being disconnected
> - *
> - * Disables the D+ (or potentially D-) pullup, which the host may see
> - * as a disconnect (when a VBUS session is active).  Not all systems
> - * support software pullup controls.
> + * usb_gadget_connect - software-controlled connect to USB host
> + * @gadget:the peripheral being connected
>   *
> - * Following a successful disconnect, invoke the ->disconnect() callback
> - * for the current gadget driver so that UDC drivers don't need to.
> + * Enables the D+ (or potentially D-) pullup.  The host will start
> + * enumerating this gadget when the pullup is active and a VBUS session
> + * is active (the link is powered).
>   *
>   * Returns zero on success, else negative errno.
>   */
> -int usb_gadget_disconnect(struct usb_gadget *gadget)
> +int usb_gadget_connect(struct usb_gadget *gadget)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->udc->connect_lock);
> +	ret = usb_gadget_connect_locked(gadget);
> +	mutex_unlock(&gadget->udc->connect_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_connect);
> +
> +/*
> + * Internal version of usb_gadget_disconnect needs to be called with
> + * connect_lock held.
> + */
> +static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
> +	__must_hold(&gadget->udc->connect_lock)
>  {
>  	int ret = 0;
>  
> @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
>  	if (!gadget->connected)
>  		goto out;
>  
> -	if (gadget->deactivated) {
> +	if (gadget->deactivated || !gadget->udc->started) {

Do you really need to add this extra test?  After all, if the gadget 
isn't started then how could the previous test of gadget->connected 
possibly succeed?

In fact, I suspect this entire section of code was always useless, since 
the gadget couldn't be connected now if it was already deactivated.

>  		/*
>  		 * If gadget is deactivated we only save new state.
>  		 * Gadget will stay disconnected after activation.
> +		 *
> +		 * udc should have been started before gadget being pulled down.

No need to mention this.

>  		 */
>  		gadget->connected = false;
>  		goto out;
> @@ -779,6 +797,30 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
>  
>  	return ret;
>  }
> +
> +/**
> + * usb_gadget_disconnect - software-controlled disconnect from USB host
> + * @gadget:the peripheral being disconnected
> + *
> + * Disables the D+ (or potentially D-) pullup, which the host may see
> + * as a disconnect (when a VBUS session is active).  Not all systems
> + * support software pullup controls.
> + *
> + * Following a successful disconnect, invoke the ->disconnect() callback
> + * for the current gadget driver so that UDC drivers don't need to.
> + *
> + * Returns zero on success, else negative errno.
> + */
> +int usb_gadget_disconnect(struct usb_gadget *gadget)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->udc->connect_lock);
> +	ret = usb_gadget_disconnect_locked(gadget);
> +	mutex_unlock(&gadget->udc->connect_lock);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
>  
>  /**
> @@ -799,10 +841,11 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
>  	if (gadget->deactivated)
>  		goto out;
>  
> +	mutex_lock(&gadget->udc->connect_lock);

The mutex should be acquired _before_ the test of gadget->deactivated.  
Otherwise the the state could change between the time of the test and 
the time of the mutex_lock().

>  	if (gadget->connected) {
> -		ret = usb_gadget_disconnect(gadget);
> +		ret = usb_gadget_disconnect_locked(gadget);
>  		if (ret)
> -			goto out;
> +			goto unlock;
>  
>  		/*
>  		 * If gadget was being connected before deactivation, we want
> @@ -812,6 +855,8 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
>  	}
>  	gadget->deactivated = true;
>  
> +unlock:
> +	mutex_unlock(&gadget->udc->connect_lock);
>  out:
>  	trace_usb_gadget_deactivate(gadget, ret);
>  
> @@ -835,6 +880,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
>  	if (!gadget->deactivated)
>  		goto out;
>  
> +	mutex_lock(&gadget->udc->connect_lock);
>  	gadget->deactivated = false;

Again, lock the mutex before testing the flag.

>  
>  	/*
> @@ -842,7 +888,8 @@ int usb_gadget_activate(struct usb_gadget *gadget)
>  	 * while it was being deactivated, we call usb_gadget_connect().
>  	 */
>  	if (gadget->connected)
> -		ret = usb_gadget_connect(gadget);
> +		ret = usb_gadget_connect_locked(gadget);
> +	mutex_unlock(&gadget->udc->connect_lock);
>  
>  out:
>  	trace_usb_gadget_activate(gadget, ret);
> @@ -1083,19 +1130,22 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
>  
>  /* ------------------------------------------------------------------------- */
>  
> -static void usb_udc_connect_control(struct usb_udc *udc)
> +/* Acquire connect_lock before calling this function. */
> +static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
>  {
>  	if (udc->vbus)
> -		usb_gadget_connect(udc->gadget);
> +		usb_gadget_connect_locked(udc->gadget);
>  	else
> -		usb_gadget_disconnect(udc->gadget);
> +		usb_gadget_disconnect_locked(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);
> +	mutex_lock(&udc->connect_lock);
> +	usb_udc_connect_control_locked(udc);
> +	mutex_unlock(&udc->connect_lock);
>  }
>  
>  /**
> @@ -1144,7 +1194,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget,
>  EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
>  
>  /**
> - * usb_gadget_udc_start - tells usb device controller to start up
> + * usb_gadget_udc_start_locked - tells usb device controller to start up
>   * @udc: The UDC to be started
>   *
>   * This call is issued by the UDC Class driver when it's about
> @@ -1155,8 +1205,11 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
>   * necessary to have it powered on.
>   *
>   * Returns zero on success, else negative errno.
> + *
> + * Caller should acquire connect_lock before invoking this function.
>   */
> -static inline int usb_gadget_udc_start(struct usb_udc *udc)
> +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
> +	__must_hold(&udc->connect_lock)
>  {
>  	int ret;
>  
> @@ -1173,7 +1226,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
>  }
>  
>  /**
> - * usb_gadget_udc_stop - tells usb device controller we don't need it anymore
> + * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore
>   * @udc: The UDC to be stopped
>   *
>   * This call is issued by the UDC Class driver after calling
> @@ -1182,8 +1235,11 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
>   * The details are implementation specific, but it can go as
>   * far as powering off UDC completely and disable its data
>   * line pullups.
> + *
> + * Caller should acquire connect lock before invoking this function.
>   */
> -static inline void usb_gadget_udc_stop(struct usb_udc *udc)
> +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc)
> +	__must_hold(&udc->connect_lock)
>  {
>  	if (!udc->started) {
>  		dev_err(&udc->dev, "UDC had already stopped\n");
> @@ -1342,6 +1398,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
>  
>  	udc->gadget = gadget;
>  	gadget->udc = udc;
> +	mutex_init(&udc->connect_lock);
>  
>  	udc->started = false;
>  
> @@ -1545,12 +1602,16 @@ static int gadget_bind_driver(struct device *dev)
>  	if (ret)
>  		goto err_bind;
>  
> -	ret = usb_gadget_udc_start(udc);
> -	if (ret)
> +	mutex_lock(&udc->connect_lock);
> +	ret = usb_gadget_udc_start_locked(udc);
> +	if (ret) {
> +		mutex_unlock(&udc->connect_lock);
>  		goto err_start;
> +	}
>  	usb_gadget_enable_async_callbacks(udc);
>  	udc->allow_connect = true;
> -	usb_udc_connect_control(udc);
> +	usb_udc_connect_control_locked(udc);
> +	mutex_unlock(&udc->connect_lock);
>  
>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>  	return 0;
> @@ -1583,12 +1644,14 @@ static void gadget_unbind_driver(struct device *dev)
>  
>  	udc->allow_connect = false;
>  	cancel_work_sync(&udc->vbus_work);
> -	usb_gadget_disconnect(gadget);
> +	mutex_lock(&udc->connect_lock);
> +	usb_gadget_disconnect_locked(gadget);
>  	usb_gadget_disable_async_callbacks(udc);
>  	if (gadget->irq)
>  		synchronize_irq(gadget->irq);
>  	udc->driver->unbind(gadget);
> -	usb_gadget_udc_stop(udc);
> +	usb_gadget_udc_stop_locked(udc);
> +	mutex_unlock(&udc->connect_lock);
>  
>  	mutex_lock(&udc_lock);
>  	driver->is_bound = false;

In both these routines, you are careful not to hold the 
udc->connect_lock and the udc_lock at the same time.  Good.

> @@ -1674,11 +1737,15 @@ static ssize_t soft_connect_store(struct device *dev,
>  	}
>  
>  	if (sysfs_streq(buf, "connect")) {
> -		usb_gadget_udc_start(udc);
> -		usb_gadget_connect(udc->gadget);
> +		mutex_lock(&udc->connect_lock);
> +		usb_gadget_udc_start_locked(udc);
> +		usb_gadget_connect_locked(udc->gadget);

Interesting change of behavior here.  Before this patch the connect 
operation would succeed, even if no gadget driver was bound.  Now it 
won't, which is how it ought to behave.

> +		mutex_unlock(&udc->connect_lock);
>  	} else if (sysfs_streq(buf, "disconnect")) {
> -		usb_gadget_disconnect(udc->gadget);
> -		usb_gadget_udc_stop(udc);
> +		mutex_lock(&udc->connect_lock);
> +		usb_gadget_disconnect_locked(udc->gadget);
> +		usb_gadget_udc_stop_locked(udc);
> +		mutex_unlock(&udc->connect_lock);
>  	} else {
>  		dev_err(dev, "unsupported command '%s'\n", buf);
>  		ret = -EINVAL;

Alan Stern
  
Badhri Jagan Sridharan June 8, 2023, 5:30 a.m. UTC | #2
On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> This patch looks basically okay.  Most of the comments below are
> concerned only with style or presentation.
>
> On Thu, Jun 01, 2023 at 03:10:28AM +0000, Badhri Jagan Sridharan wrote:
> > usb_udc_connect_control(), soft_connect_store() and
> > usb_gadget_deactivate() can potentialy race against each other to invoke
>
> "potentially" has two 'l's.
>
> > usb_gadget_connect()/usb_gadget_disconnect(). To prevent this, guarding
>
> s/guarding/guard/
>
> > udc->vbus, udc->started, gadget->allow_connect, gadget->deactivate with
>
> Insert "and" before "gadget->deactivate".
>
> Also, I don't think the patch guards udc->vbus.  After all, that flag
> can be changed in usb_udc_vbus_handler() without the protection of the
> mutex.
>
> > connect_lock so that ->pullup() is only invoked when gadget driver is
> > bound, not deactivated and started.
>
> "when the gadget", not "when gadget driver".  The driver isn't what gets
> deactivated or started; the gadget is.
>
> This would be easier to understand if the last two items were switched:
>
>         bound, started, and not deactivated.
>
> Also, it would help readers if there were some additional text to help
> separate the end of this sentence from the start of the next one.  For
> example, you might insert "The routines" at the beginning of the next
> sentence.
>
> >  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.
> >
> > This commit was reverted due to the crash reported in
>
> An earlier version of this commit...
>
> > https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/.
> > commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing")
> > addresses the crash reported.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> > v5 is the first version in this series.
> > Changes since v5:
> > ** Fixed up commit message
> > ** Wrapped comments at 76.
> > ---
> >  drivers/usb/gadget/udc/core.c | 151 ++++++++++++++++++++++++----------
> >  1 file changed, 109 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index d2e4f78c53e3..b53f74fcad60 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -40,6 +40,11 @@ static const struct bus_type gadget_bus_type;
> >   * @allow_connect: Indicates whether UDC is allowed to be pulled up.
> >   * 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. usb_gadget_connect_locked,
>
> Again, separate the two sentences with some extra text.  Otherwise the
> period looks too much like a comma for people to see it easily.
>
> > + * 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.
> >   *
> >   * This represents the internal data structure which is used by the UDC-class
> >   * to hold information about udc driver and gadget together.
> > @@ -53,6 +58,7 @@ struct usb_udc {
> >       bool                            started;
> >       bool                            allow_connect;
> >       struct work_struct              vbus_work;
> > +     struct mutex                    connect_lock;
> >  };
> >
> >  static struct class *udc_class;
> > @@ -692,17 +698,12 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
> >
> > -/**
> > - * usb_gadget_connect - software-controlled connect to USB host
> > - * @gadget:the peripheral being connected
> > - *
> > - * Enables the D+ (or potentially D-) pullup.  The host will start
> > - * enumerating this gadget when the pullup is active and a VBUS session
> > - * is active (the link is powered).
> > - *
> > - * Returns zero on success, else negative errno.
> > +/*
> > + * Internal version of usb_gadget_connect needs to be called with
> > + * connect_lock held.
>
> I'm not sure you need to say this; it's pretty obvious from the
> __must_hold() annotation a few lines later.  Consider removing this
> paragraph and the similar paragraphs in the other new internal routines.
>
> >   */
> > -int usb_gadget_connect(struct usb_gadget *gadget)
> > +static int usb_gadget_connect_locked(struct usb_gadget *gadget)
> > +     __must_hold(&gadget->udc->connect_lock)
> >  {
> >       int ret = 0;
> >
> > @@ -711,10 +712,12 @@ int usb_gadget_connect(struct usb_gadget *gadget)
> >               goto out;
> >       }
> >
> > -     if (gadget->deactivated || !gadget->udc->allow_connect) {
> > +     if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) {
> >               /*
> >                * If gadget is deactivated we only save new state.
> >                * Gadget will be connected automatically after activation.
>
> This comment is now inaccurate.  Change it to say:
>
>                  * If the gadget isn't usable (because it is deactivated,
>                  * unbound, or not yet started), we only save the new state.
>                  * The gadget will be connected automatically when it is
>                  * activated/bound/started.
>
> > +              *
> > +              * udc first needs to be started before gadget can be pulled up.
>
> And then this sentence won't be needed.
>
> >                */
> >               gadget->connected = true;
> >               goto out;
> > @@ -729,22 +732,35 @@ int usb_gadget_connect(struct usb_gadget *gadget)
> >
> >       return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(usb_gadget_connect);
> >
> >  /**
> > - * usb_gadget_disconnect - software-controlled disconnect from USB host
> > - * @gadget:the peripheral being disconnected
> > - *
> > - * Disables the D+ (or potentially D-) pullup, which the host may see
> > - * as a disconnect (when a VBUS session is active).  Not all systems
> > - * support software pullup controls.
> > + * usb_gadget_connect - software-controlled connect to USB host
> > + * @gadget:the peripheral being connected
> >   *
> > - * Following a successful disconnect, invoke the ->disconnect() callback
> > - * for the current gadget driver so that UDC drivers don't need to.
> > + * Enables the D+ (or potentially D-) pullup.  The host will start
> > + * enumerating this gadget when the pullup is active and a VBUS session
> > + * is active (the link is powered).
> >   *
> >   * Returns zero on success, else negative errno.
> >   */
> > -int usb_gadget_disconnect(struct usb_gadget *gadget)
> > +int usb_gadget_connect(struct usb_gadget *gadget)
> > +{
> > +     int ret;
> > +
> > +     mutex_lock(&gadget->udc->connect_lock);
> > +     ret = usb_gadget_connect_locked(gadget);
> > +     mutex_unlock(&gadget->udc->connect_lock);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_gadget_connect);
> > +
> > +/*
> > + * Internal version of usb_gadget_disconnect needs to be called with
> > + * connect_lock held.
> > + */
> > +static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
> > +     __must_hold(&gadget->udc->connect_lock)
> >  {
> >       int ret = 0;
> >
> > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
> >       if (!gadget->connected)
> >               goto out;
> >
> > -     if (gadget->deactivated) {
> > +     if (gadget->deactivated || !gadget->udc->started) {
>
> Do you really need to add this extra test?  After all, if the gadget
> isn't started then how could the previous test of gadget->connected
> possibly succeed?
>
> In fact, I suspect this entire section of code was always useless, since
> the gadget couldn't be connected now if it was already deactivated.
>
Thanks Alan ! Will fix all other comments in v7 but not sure about this one.
Although the ->pullup() function will not be called,
-> connected flag could actually be set when the gadget is not started.

- if (gadget->deactivated || !gadget->udc->allow_connect) {
+ if (gadget->deactivated || !gadget->udc->allow_connect ||
!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;

This could happen, for instance, when  usb_udc_vbus_handler()  is
invoked after soft_connect_store() disconnects the gadget.
Same applies to when usb_gadget_connect() is called after the gadget
has been deactivated through usb_gadget_deactivate().

This implies that the checks should be there, right ?

> >               /*
> >                * If gadget is deactivated we only save new state.
> >                * Gadget will stay disconnected after activation.
> > +              *
> > +              * udc should have been started before gadget being pulled down.
>
> No need to mention this.
>
> >                */
> >               gadget->connected = false;
> >               goto out;
> > @@ -779,6 +797,30 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
> >
> >       return ret;
> >  }
> > +
> > +/**
> > + * usb_gadget_disconnect - software-controlled disconnect from USB host
> > + * @gadget:the peripheral being disconnected
> > + *
> > + * Disables the D+ (or potentially D-) pullup, which the host may see
> > + * as a disconnect (when a VBUS session is active).  Not all systems
> > + * support software pullup controls.
> > + *
> > + * Following a successful disconnect, invoke the ->disconnect() callback
> > + * for the current gadget driver so that UDC drivers don't need to.
> > + *
> > + * Returns zero on success, else negative errno.
> > + */
> > +int usb_gadget_disconnect(struct usb_gadget *gadget)
> > +{
> > +     int ret;
> > +
> > +     mutex_lock(&gadget->udc->connect_lock);
> > +     ret = usb_gadget_disconnect_locked(gadget);
> > +     mutex_unlock(&gadget->udc->connect_lock);
> > +
> > +     return ret;
> > +}
> >  EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
> >
> >  /**
> > @@ -799,10 +841,11 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
> >       if (gadget->deactivated)
> >               goto out;
> >
> > +     mutex_lock(&gadget->udc->connect_lock);
>
> The mutex should be acquired _before_ the test of gadget->deactivated.
> Otherwise the the state could change between the time of the test and
> the time of the mutex_lock().
>
> >       if (gadget->connected) {
> > -             ret = usb_gadget_disconnect(gadget);
> > +             ret = usb_gadget_disconnect_locked(gadget);
> >               if (ret)
> > -                     goto out;
> > +                     goto unlock;
> >
> >               /*
> >                * If gadget was being connected before deactivation, we want
> > @@ -812,6 +855,8 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
> >       }
> >       gadget->deactivated = true;
> >
> > +unlock:
> > +     mutex_unlock(&gadget->udc->connect_lock);
> >  out:
> >       trace_usb_gadget_deactivate(gadget, ret);
> >
> > @@ -835,6 +880,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
> >       if (!gadget->deactivated)
> >               goto out;
> >
> > +     mutex_lock(&gadget->udc->connect_lock);
> >       gadget->deactivated = false;
>
> Again, lock the mutex before testing the flag.
>
> >
> >       /*
> > @@ -842,7 +888,8 @@ int usb_gadget_activate(struct usb_gadget *gadget)
> >        * while it was being deactivated, we call usb_gadget_connect().
> >        */
> >       if (gadget->connected)
> > -             ret = usb_gadget_connect(gadget);
> > +             ret = usb_gadget_connect_locked(gadget);
> > +     mutex_unlock(&gadget->udc->connect_lock);
> >
> >  out:
> >       trace_usb_gadget_activate(gadget, ret);
> > @@ -1083,19 +1130,22 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
> >
> >  /* ------------------------------------------------------------------------- */
> >
> > -static void usb_udc_connect_control(struct usb_udc *udc)
> > +/* Acquire connect_lock before calling this function. */
> > +static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
> >  {
> >       if (udc->vbus)
> > -             usb_gadget_connect(udc->gadget);
> > +             usb_gadget_connect_locked(udc->gadget);
> >       else
> > -             usb_gadget_disconnect(udc->gadget);
> > +             usb_gadget_disconnect_locked(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);
> > +     mutex_lock(&udc->connect_lock);
> > +     usb_udc_connect_control_locked(udc);
> > +     mutex_unlock(&udc->connect_lock);
> >  }
> >
> >  /**
> > @@ -1144,7 +1194,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget,
> >  EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
> >
> >  /**
> > - * usb_gadget_udc_start - tells usb device controller to start up
> > + * usb_gadget_udc_start_locked - tells usb device controller to start up
> >   * @udc: The UDC to be started
> >   *
> >   * This call is issued by the UDC Class driver when it's about
> > @@ -1155,8 +1205,11 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
> >   * necessary to have it powered on.
> >   *
> >   * Returns zero on success, else negative errno.
> > + *
> > + * Caller should acquire connect_lock before invoking this function.
> >   */
> > -static inline int usb_gadget_udc_start(struct usb_udc *udc)
> > +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
> > +     __must_hold(&udc->connect_lock)
> >  {
> >       int ret;
> >
> > @@ -1173,7 +1226,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
> >  }
> >
> >  /**
> > - * usb_gadget_udc_stop - tells usb device controller we don't need it anymore
> > + * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore
> >   * @udc: The UDC to be stopped
> >   *
> >   * This call is issued by the UDC Class driver after calling
> > @@ -1182,8 +1235,11 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
> >   * The details are implementation specific, but it can go as
> >   * far as powering off UDC completely and disable its data
> >   * line pullups.
> > + *
> > + * Caller should acquire connect lock before invoking this function.
> >   */
> > -static inline void usb_gadget_udc_stop(struct usb_udc *udc)
> > +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc)
> > +     __must_hold(&udc->connect_lock)
> >  {
> >       if (!udc->started) {
> >               dev_err(&udc->dev, "UDC had already stopped\n");
> > @@ -1342,6 +1398,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
> >
> >       udc->gadget = gadget;
> >       gadget->udc = udc;
> > +     mutex_init(&udc->connect_lock);
> >
> >       udc->started = false;
> >
> > @@ -1545,12 +1602,16 @@ static int gadget_bind_driver(struct device *dev)
> >       if (ret)
> >               goto err_bind;
> >
> > -     ret = usb_gadget_udc_start(udc);
> > -     if (ret)
> > +     mutex_lock(&udc->connect_lock);
> > +     ret = usb_gadget_udc_start_locked(udc);
> > +     if (ret) {
> > +             mutex_unlock(&udc->connect_lock);
> >               goto err_start;
> > +     }
> >       usb_gadget_enable_async_callbacks(udc);
> >       udc->allow_connect = true;
> > -     usb_udc_connect_control(udc);
> > +     usb_udc_connect_control_locked(udc);
> > +     mutex_unlock(&udc->connect_lock);
> >
> >       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> >       return 0;
> > @@ -1583,12 +1644,14 @@ static void gadget_unbind_driver(struct device *dev)
> >
> >       udc->allow_connect = false;
> >       cancel_work_sync(&udc->vbus_work);
> > -     usb_gadget_disconnect(gadget);
> > +     mutex_lock(&udc->connect_lock);
> > +     usb_gadget_disconnect_locked(gadget);
> >       usb_gadget_disable_async_callbacks(udc);
> >       if (gadget->irq)
> >               synchronize_irq(gadget->irq);
> >       udc->driver->unbind(gadget);
> > -     usb_gadget_udc_stop(udc);
> > +     usb_gadget_udc_stop_locked(udc);
> > +     mutex_unlock(&udc->connect_lock);
> >
> >       mutex_lock(&udc_lock);
> >       driver->is_bound = false;
>
> In both these routines, you are careful not to hold the
> udc->connect_lock and the udc_lock at the same time.  Good.
>
> > @@ -1674,11 +1737,15 @@ static ssize_t soft_connect_store(struct device *dev,
> >       }
> >
> >       if (sysfs_streq(buf, "connect")) {
> > -             usb_gadget_udc_start(udc);
> > -             usb_gadget_connect(udc->gadget);
> > +             mutex_lock(&udc->connect_lock);
> > +             usb_gadget_udc_start_locked(udc);
> > +             usb_gadget_connect_locked(udc->gadget);
>
> Interesting change of behavior here.  Before this patch the connect
> operation would succeed, even if no gadget driver was bound.  Now it
> won't, which is how it ought to behave.
>
> > +             mutex_unlock(&udc->connect_lock);
> >       } else if (sysfs_streq(buf, "disconnect")) {
> > -             usb_gadget_disconnect(udc->gadget);
> > -             usb_gadget_udc_stop(udc);
> > +             mutex_lock(&udc->connect_lock);
> > +             usb_gadget_disconnect_locked(udc->gadget);
> > +             usb_gadget_udc_stop_locked(udc);
> > +             mutex_unlock(&udc->connect_lock);
> >       } else {
> >               dev_err(dev, "unsupported command '%s'\n", buf);
> >               ret = -EINVAL;
>
> Alan Stern
  
Alan Stern June 8, 2023, 3:27 p.m. UTC | #3
On Wed, Jun 07, 2023 at 10:17:04PM -0700, Badhri Jagan Sridharan wrote:
> On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@rowland.harvard.edu>
> wrote:
> > > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget
> > *gadget)
> > >       if (!gadget->connected)
> > >               goto out;
> > >
> > > -     if (gadget->deactivated) {
> > > +     if (gadget->deactivated || !gadget->udc->started) {
> >
> > Do you really need to add this extra test?  After all, if the gadget
> > isn't started then how could the previous test of gadget->connected
> > possibly succeed?
> >
> > In fact, I suspect this entire section of code was always useless, since
> > the gadget couldn't be connected now if it was already deactivated.
> >
> 
> Thanks Alan ! Will fix all other comments in v7 but not sure about this one.
> Although the ->pullup() function will not be called,
> -> connected flag could actually be set when the gadget is not started.
> 
> - if (gadget->deactivated || !gadget->udc->allow_connect) {
> + if (gadget->deactivated || !gadget->udc->allow_connect ||
> !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;
> 
> This could happen, for instance, when  usb_udc_vbus_handler()  is invoked
> after soft_connect_store() disconnects the gadget.
> Same applies to when usb_gadget_connect() is called after the gadget has
> been deactivated through usb_gadget_deactivate().
> 
> This implies that the checks should be there, right ?

Yes, you're right; the checks do need to be there.  I had forgotten
about these possible cases.  Ignore that comment.

Alan Stern
  
Badhri Jagan Sridharan June 8, 2023, 8:47 p.m. UTC | #4
Thanks Alan ! Just sent out the v7 of the series after fixing all
other comments.

Regards,
Badhri

On Thu, Jun 8, 2023 at 8:27 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Jun 07, 2023 at 10:17:04PM -0700, Badhri Jagan Sridharan wrote:
> > On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@rowland.harvard.edu>
> > wrote:
> > > > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget
> > > *gadget)
> > > >       if (!gadget->connected)
> > > >               goto out;
> > > >
> > > > -     if (gadget->deactivated) {
> > > > +     if (gadget->deactivated || !gadget->udc->started) {
> > >
> > > Do you really need to add this extra test?  After all, if the gadget
> > > isn't started then how could the previous test of gadget->connected
> > > possibly succeed?
> > >
> > > In fact, I suspect this entire section of code was always useless, since
> > > the gadget couldn't be connected now if it was already deactivated.
> > >
> >
> > Thanks Alan ! Will fix all other comments in v7 but not sure about this one.
> > Although the ->pullup() function will not be called,
> > -> connected flag could actually be set when the gadget is not started.
> >
> > - if (gadget->deactivated || !gadget->udc->allow_connect) {
> > + if (gadget->deactivated || !gadget->udc->allow_connect ||
> > !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;
> >
> > This could happen, for instance, when  usb_udc_vbus_handler()  is invoked
> > after soft_connect_store() disconnects the gadget.
> > Same applies to when usb_gadget_connect() is called after the gadget has
> > been deactivated through usb_gadget_deactivate().
> >
> > This implies that the checks should be there, right ?
>
> Yes, you're right; the checks do need to be there.  I had forgotten
> about these possible cases.  Ignore that comment.
>
> Alan Stern
  

Patch

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d2e4f78c53e3..b53f74fcad60 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -40,6 +40,11 @@  static const struct bus_type gadget_bus_type;
  * @allow_connect: Indicates whether UDC is allowed to be pulled up.
  * 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. 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.
  *
  * This represents the internal data structure which is used by the UDC-class
  * to hold information about udc driver and gadget together.
@@ -53,6 +58,7 @@  struct usb_udc {
 	bool				started;
 	bool				allow_connect;
 	struct work_struct		vbus_work;
+	struct mutex			connect_lock;
 };
 
 static struct class *udc_class;
@@ -692,17 +698,12 @@  int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
 }
 EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
 
-/**
- * usb_gadget_connect - software-controlled connect to USB host
- * @gadget:the peripheral being connected
- *
- * Enables the D+ (or potentially D-) pullup.  The host will start
- * enumerating this gadget when the pullup is active and a VBUS session
- * is active (the link is powered).
- *
- * Returns zero on success, else negative errno.
+/*
+ * Internal version of usb_gadget_connect needs to be called with
+ * connect_lock held.
  */
-int usb_gadget_connect(struct usb_gadget *gadget)
+static int usb_gadget_connect_locked(struct usb_gadget *gadget)
+	__must_hold(&gadget->udc->connect_lock)
 {
 	int ret = 0;
 
@@ -711,10 +712,12 @@  int usb_gadget_connect(struct usb_gadget *gadget)
 		goto out;
 	}
 
-	if (gadget->deactivated || !gadget->udc->allow_connect) {
+	if (gadget->deactivated || !gadget->udc->allow_connect || !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;
@@ -729,22 +732,35 @@  int usb_gadget_connect(struct usb_gadget *gadget)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(usb_gadget_connect);
 
 /**
- * usb_gadget_disconnect - software-controlled disconnect from USB host
- * @gadget:the peripheral being disconnected
- *
- * Disables the D+ (or potentially D-) pullup, which the host may see
- * as a disconnect (when a VBUS session is active).  Not all systems
- * support software pullup controls.
+ * usb_gadget_connect - software-controlled connect to USB host
+ * @gadget:the peripheral being connected
  *
- * Following a successful disconnect, invoke the ->disconnect() callback
- * for the current gadget driver so that UDC drivers don't need to.
+ * Enables the D+ (or potentially D-) pullup.  The host will start
+ * enumerating this gadget when the pullup is active and a VBUS session
+ * is active (the link is powered).
  *
  * Returns zero on success, else negative errno.
  */
-int usb_gadget_disconnect(struct usb_gadget *gadget)
+int usb_gadget_connect(struct usb_gadget *gadget)
+{
+	int ret;
+
+	mutex_lock(&gadget->udc->connect_lock);
+	ret = usb_gadget_connect_locked(gadget);
+	mutex_unlock(&gadget->udc->connect_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_connect);
+
+/*
+ * Internal version of usb_gadget_disconnect needs to be called with
+ * connect_lock held.
+ */
+static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
+	__must_hold(&gadget->udc->connect_lock)
 {
 	int ret = 0;
 
@@ -756,10 +772,12 @@  int usb_gadget_disconnect(struct usb_gadget *gadget)
 	if (!gadget->connected)
 		goto out;
 
-	if (gadget->deactivated) {
+	if (gadget->deactivated || !gadget->udc->started) {
 		/*
 		 * If gadget is deactivated we only save new state.
 		 * Gadget will stay disconnected after activation.
+		 *
+		 * udc should have been started before gadget being pulled down.
 		 */
 		gadget->connected = false;
 		goto out;
@@ -779,6 +797,30 @@  int usb_gadget_disconnect(struct usb_gadget *gadget)
 
 	return ret;
 }
+
+/**
+ * usb_gadget_disconnect - software-controlled disconnect from USB host
+ * @gadget:the peripheral being disconnected
+ *
+ * Disables the D+ (or potentially D-) pullup, which the host may see
+ * as a disconnect (when a VBUS session is active).  Not all systems
+ * support software pullup controls.
+ *
+ * Following a successful disconnect, invoke the ->disconnect() callback
+ * for the current gadget driver so that UDC drivers don't need to.
+ *
+ * Returns zero on success, else negative errno.
+ */
+int usb_gadget_disconnect(struct usb_gadget *gadget)
+{
+	int ret;
+
+	mutex_lock(&gadget->udc->connect_lock);
+	ret = usb_gadget_disconnect_locked(gadget);
+	mutex_unlock(&gadget->udc->connect_lock);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
 
 /**
@@ -799,10 +841,11 @@  int usb_gadget_deactivate(struct usb_gadget *gadget)
 	if (gadget->deactivated)
 		goto out;
 
+	mutex_lock(&gadget->udc->connect_lock);
 	if (gadget->connected) {
-		ret = usb_gadget_disconnect(gadget);
+		ret = usb_gadget_disconnect_locked(gadget);
 		if (ret)
-			goto out;
+			goto unlock;
 
 		/*
 		 * If gadget was being connected before deactivation, we want
@@ -812,6 +855,8 @@  int usb_gadget_deactivate(struct usb_gadget *gadget)
 	}
 	gadget->deactivated = true;
 
+unlock:
+	mutex_unlock(&gadget->udc->connect_lock);
 out:
 	trace_usb_gadget_deactivate(gadget, ret);
 
@@ -835,6 +880,7 @@  int usb_gadget_activate(struct usb_gadget *gadget)
 	if (!gadget->deactivated)
 		goto out;
 
+	mutex_lock(&gadget->udc->connect_lock);
 	gadget->deactivated = false;
 
 	/*
@@ -842,7 +888,8 @@  int usb_gadget_activate(struct usb_gadget *gadget)
 	 * while it was being deactivated, we call usb_gadget_connect().
 	 */
 	if (gadget->connected)
-		ret = usb_gadget_connect(gadget);
+		ret = usb_gadget_connect_locked(gadget);
+	mutex_unlock(&gadget->udc->connect_lock);
 
 out:
 	trace_usb_gadget_activate(gadget, ret);
@@ -1083,19 +1130,22 @@  EXPORT_SYMBOL_GPL(usb_gadget_set_state);
 
 /* ------------------------------------------------------------------------- */
 
-static void usb_udc_connect_control(struct usb_udc *udc)
+/* Acquire connect_lock before calling this function. */
+static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
 {
 	if (udc->vbus)
-		usb_gadget_connect(udc->gadget);
+		usb_gadget_connect_locked(udc->gadget);
 	else
-		usb_gadget_disconnect(udc->gadget);
+		usb_gadget_disconnect_locked(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);
+	mutex_lock(&udc->connect_lock);
+	usb_udc_connect_control_locked(udc);
+	mutex_unlock(&udc->connect_lock);
 }
 
 /**
@@ -1144,7 +1194,7 @@  void usb_gadget_udc_reset(struct usb_gadget *gadget,
 EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
 
 /**
- * usb_gadget_udc_start - tells usb device controller to start up
+ * usb_gadget_udc_start_locked - tells usb device controller to start up
  * @udc: The UDC to be started
  *
  * This call is issued by the UDC Class driver when it's about
@@ -1155,8 +1205,11 @@  EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
  * necessary to have it powered on.
  *
  * Returns zero on success, else negative errno.
+ *
+ * Caller should acquire connect_lock before invoking this function.
  */
-static inline int usb_gadget_udc_start(struct usb_udc *udc)
+static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
+	__must_hold(&udc->connect_lock)
 {
 	int ret;
 
@@ -1173,7 +1226,7 @@  static inline int usb_gadget_udc_start(struct usb_udc *udc)
 }
 
 /**
- * usb_gadget_udc_stop - tells usb device controller we don't need it anymore
+ * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore
  * @udc: The UDC to be stopped
  *
  * This call is issued by the UDC Class driver after calling
@@ -1182,8 +1235,11 @@  static inline int usb_gadget_udc_start(struct usb_udc *udc)
  * The details are implementation specific, but it can go as
  * far as powering off UDC completely and disable its data
  * line pullups.
+ *
+ * Caller should acquire connect lock before invoking this function.
  */
-static inline void usb_gadget_udc_stop(struct usb_udc *udc)
+static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc)
+	__must_hold(&udc->connect_lock)
 {
 	if (!udc->started) {
 		dev_err(&udc->dev, "UDC had already stopped\n");
@@ -1342,6 +1398,7 @@  int usb_add_gadget(struct usb_gadget *gadget)
 
 	udc->gadget = gadget;
 	gadget->udc = udc;
+	mutex_init(&udc->connect_lock);
 
 	udc->started = false;
 
@@ -1545,12 +1602,16 @@  static int gadget_bind_driver(struct device *dev)
 	if (ret)
 		goto err_bind;
 
-	ret = usb_gadget_udc_start(udc);
-	if (ret)
+	mutex_lock(&udc->connect_lock);
+	ret = usb_gadget_udc_start_locked(udc);
+	if (ret) {
+		mutex_unlock(&udc->connect_lock);
 		goto err_start;
+	}
 	usb_gadget_enable_async_callbacks(udc);
 	udc->allow_connect = true;
-	usb_udc_connect_control(udc);
+	usb_udc_connect_control_locked(udc);
+	mutex_unlock(&udc->connect_lock);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
@@ -1583,12 +1644,14 @@  static void gadget_unbind_driver(struct device *dev)
 
 	udc->allow_connect = false;
 	cancel_work_sync(&udc->vbus_work);
-	usb_gadget_disconnect(gadget);
+	mutex_lock(&udc->connect_lock);
+	usb_gadget_disconnect_locked(gadget);
 	usb_gadget_disable_async_callbacks(udc);
 	if (gadget->irq)
 		synchronize_irq(gadget->irq);
 	udc->driver->unbind(gadget);
-	usb_gadget_udc_stop(udc);
+	usb_gadget_udc_stop_locked(udc);
+	mutex_unlock(&udc->connect_lock);
 
 	mutex_lock(&udc_lock);
 	driver->is_bound = false;
@@ -1674,11 +1737,15 @@  static ssize_t soft_connect_store(struct device *dev,
 	}
 
 	if (sysfs_streq(buf, "connect")) {
-		usb_gadget_udc_start(udc);
-		usb_gadget_connect(udc->gadget);
+		mutex_lock(&udc->connect_lock);
+		usb_gadget_udc_start_locked(udc);
+		usb_gadget_connect_locked(udc->gadget);
+		mutex_unlock(&udc->connect_lock);
 	} else if (sysfs_streq(buf, "disconnect")) {
-		usb_gadget_disconnect(udc->gadget);
-		usb_gadget_udc_stop(udc);
+		mutex_lock(&udc->connect_lock);
+		usb_gadget_disconnect_locked(udc->gadget);
+		usb_gadget_udc_stop_locked(udc);
+		mutex_unlock(&udc->connect_lock);
 	} else {
 		dev_err(dev, "unsupported command '%s'\n", buf);
 		ret = -EINVAL;