[PATCH v5 1/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing

[PATCH v5 1/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing

[Badhri Jagan Sridharan]

usb_udc_vbus_handler() can be invoked from interrupt context by irq
handlers of the gadget drivers, however, usb_udc_connect_control() has
to run in non-atomic context due to the following:
a. Some of the gadget driver implementations expect the ->pullup
   callback to be invoked in non-atomic context.
b. usb_gadget_disconnect() acquires udc_lock which is a mutex.

Hence offload invocation of usb_udc_connect_control()
to workqueue.

Cc: stable@vger.kernel.org
Fixes: 1016fc0c096c ("USB: gadget: Fix obscure lockdep violation for udc_mutex")
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
Changes since v1:
- Address Alan Stern's comment on usb_udc_vbus_handler invocation from
  atomic context:
* vbus_events_lock is now a spinlock and allocations in
* usb_udc_vbus_handler are atomic now.

Changes since v2:
- Addressing Alan Stern's comments:
** connect_lock is now held by callers of
* usb_gadget_pullup_update_locked() and gadget_(un)bind_driver() does
* notdirectly hold the lock.

** Both usb_gadget_(dis)connect() and usb_udc_vbus_handler() would
* set/clear udc->vbus and invoke usb_gadget_pullup_update_locked.

** Add "unbinding" to prevent new connections after the gadget is being
* unbound.

Changes since v3:
** Made a minor cleanup which I missed to do in v3 in
* usb_udc_vbus_handler().

Changes since v4:
- Addressing Alan Stern's comments:
** usb_udc_vbus_handler() now offloads invocation of usb_udc_connect_control()
* from workqueue.

** Dropped vbus_events list as this was redundant. Updating to the
* latest value is suffice
---
 drivers/usb/gadget/udc/core.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 52e6d2e84e35..44a9f32679b5 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -48,6 +48,7 @@ struct usb_udc {
 	struct list_head		list;
 	bool				vbus;
 	bool				started;
+	struct work_struct		vbus_work;
 };
 
 static struct class *udc_class;
@@ -1086,6 +1087,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
 		usb_gadget_disconnect(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);
+}
+
 /**
  * usb_udc_vbus_handler - updates the udc core vbus status, and try to
  * connect or disconnect gadget
@@ -1094,6 +1102,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
  *
  * The udc driver calls it when it wants to connect or disconnect gadget
  * according to vbus status.
+ *
+ * This function can be invoked from interrupt context by irq handlers of the gadget drivers,
+ * however, usb_udc_connect_control() has to run in non-atomic context due to the following:
+ * a. Some of the gadget driver implementations expect the ->pullup callback to be invoked in
+ * non-atomic context.
+ * b. usb_gadget_disconnect() acquires udc_lock which is a mutex.
+ * Hence offload invocation of usb_udc_connect_control() to workqueue.
  */
 void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
 {
@@ -1101,7 +1116,7 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
 
 	if (udc) {
 		udc->vbus = status;
-		usb_udc_connect_control(udc);
+		schedule_work(&udc->vbus_work);
 	}
 }
 EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
@@ -1328,6 +1343,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
 	mutex_lock(&udc_lock);
 	list_add_tail(&udc->list, &udc_list);
 	mutex_unlock(&udc_lock);
+	INIT_WORK(&udc->vbus_work, vbus_event_work);
 
 	ret = device_add(&udc->dev);
 	if (ret)
@@ -1558,6 +1574,7 @@ static void gadget_unbind_driver(struct device *dev)
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
+	cancel_work_sync(&udc->vbus_work);
 	usb_gadget_disconnect(gadget);
 	usb_gadget_disable_async_callbacks(udc);
 	if (gadget->irq)

base-commit: 046895105d9666ab56e86ce8dd9786f8003125c6
-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

[Badhri Jagan Sridharan]

usb_udc_connect_control() does not check to see if the udc has already
been started. This causes gadget->ops->pullup to be called through
usb_gadget_connect() when invoked from usb_udc_vbus_handler() even
before usb_gadget_udc_start() is called. Guard this by checking for
udc->started in usb_udc_connect_control() before invoking
usb_gadget_connect().

Guarding udc->vbus, udc->started, gadget->connect, gadget->deactivate
related functions with connect_lock. 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 as they can be simulataneously invoked from
different code paths.

Adding an additional check to make sure udc is started(udc->started)
before pullup callback is invoked.

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.
---
 drivers/usb/gadget/udc/core.c | 148 ++++++++++++++++++++++++----------
 1 file changed, 104 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 44a9f32679b5..6ffe5fda8bb7 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -37,6 +37,10 @@ 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.
+ * @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.
  *
  * This represents the internal data structure which is used by the UDC-class
  * to hold information about udc driver and gadget together.
@@ -49,6 +53,7 @@ struct usb_udc {
 	bool				vbus;
 	bool				started;
 	struct work_struct		vbus_work;
+	struct mutex			connect_lock;
 };
 
 static struct class *udc_class;
@@ -688,17 +693,9 @@ 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.
- */
-int usb_gadget_connect(struct usb_gadget *gadget)
+/* Internal version of usb_gadget_connect needs to be called with connect_lock held. */
+static int usb_gadget_connect_locked(struct usb_gadget *gadget)
+	__must_hold(&gadget->udc->connect_lock)
 {
 	int ret = 0;
 
@@ -707,10 +704,12 @@ int usb_gadget_connect(struct usb_gadget *gadget)
 		goto out;
 	}
 
-	if (gadget->deactivated) {
+	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;
@@ -725,22 +724,32 @@ 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;
 
@@ -752,10 +761,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;
@@ -775,6 +786,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);
 
 /**
@@ -795,10 +830,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
@@ -808,6 +844,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);
 
@@ -831,6 +869,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
 	if (!gadget->deactivated)
 		goto out;
 
+	mutex_lock(&gadget->udc->connect_lock);
 	gadget->deactivated = false;
 
 	/*
@@ -838,7 +877,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);
@@ -1079,19 +1119,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);
+	if (udc->vbus && udc->started)
+		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);
 }
 
 /**
@@ -1139,7 +1182,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
@@ -1150,8 +1193,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;
 
@@ -1168,7 +1214,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
@@ -1177,8 +1223,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");
@@ -1337,6 +1386,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
 
 	udc->gadget = gadget;
 	gadget->udc = udc;
+	mutex_init(&udc->connect_lock);
 
 	udc->started = false;
 
@@ -1539,11 +1589,15 @@ 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);
-	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;
@@ -1575,12 +1629,14 @@ static void gadget_unbind_driver(struct device *dev)
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
 	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;
@@ -1666,11 +1722,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;
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

[Badhri Jagan Sridharan]

On Wed, May 31, 2023 at 10:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, May 31, 2023 at 04:02:02AM +0000, Badhri Jagan Sridharan wrote:
> > usb_udc_connect_control() does not check to see if the udc has already
> > been started. This causes gadget->ops->pullup to be called through
> > usb_gadget_connect() when invoked from usb_udc_vbus_handler() even
> > before usb_gadget_udc_start() is called. Guard this by checking for
> > udc->started in usb_udc_connect_control() before invoking
> > usb_gadget_connect().
>
> After a merged version of patches 1/3 and 3/3 have been applied, it
> seems like most of this will not be needed any more.  Maybe not any of
> it.

Without the connect_lock introduced in this patch, wouldn't the
usb_gadget_connect()/
usb_gadget_disconnect() through soft_connect_store() race against
usb_gadget_connect()/ usb_gadget_disconnect() through
usb_udc_connect_control() ?

On a side note, I am working on merging  patches 1/3 and 3/3.

Thanks,
Badhri

>
> usb_udc_connect_control() gets called from only two places.  One of them
> is in gadget_bind_driver(), where we know that the UDC has been started
> and connecting is allowed.  The other place is the vbus work routine
> queued by usb_udc_vbus_handler().  If that place checks the new
> allow_connect flag before calling usb_gadget_connect(), nothing more
> will be needed.  You just have to make sure that the allow_connect flag
> is set in gadget_bind_driver between the start and connect_control
> calls, and it is cleared in gadget_unbind_driver before the
> cancel_work_sync call.
>
> It's possible that a new mutex will be needed to synchronize accesses to
> the allow_connect flag.  That's something you will have to study and
> decide on.  But if you can avoid adding one, that would be best.
>
> > Guarding udc->vbus, udc->started, gadget->connect, gadget->deactivate
> > related functions with connect_lock. 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 as they can be simulataneously invoked from
> > different code paths.
>
> It's a general principle of kernel programming that locks protect data,
> not code.  So if this patch were to be accepted, you would have to
> change this description to say that connect_lock guards various flags,
> not various function calls.
>
> Alan Stern

Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

[Alan Stern]

On Wed, May 31, 2023 at 03:40:26PM -0700, Badhri Jagan Sridharan wrote:
> On Wed, May 31, 2023 at 10:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, May 31, 2023 at 04:02:02AM +0000, Badhri Jagan Sridharan wrote:
> > > usb_udc_connect_control() does not check to see if the udc has already
> > > been started. This causes gadget->ops->pullup to be called through
> > > usb_gadget_connect() when invoked from usb_udc_vbus_handler() even
> > > before usb_gadget_udc_start() is called. Guard this by checking for
> > > udc->started in usb_udc_connect_control() before invoking
> > > usb_gadget_connect().
> >
> > After a merged version of patches 1/3 and 3/3 have been applied, it
> > seems like most of this will not be needed any more.  Maybe not any of
> > it.
> 
> Without the connect_lock introduced in this patch, wouldn't the
> usb_gadget_connect()/
> usb_gadget_disconnect() through soft_connect_store() race against
> usb_gadget_connect()/ usb_gadget_disconnect() through
> usb_udc_connect_control() ?

Okay, yes, that's a good point.  It needs to be mentioned in the patch 
description so that people will understand it is the real reason for 
this change.

Alan Stern

Re: [PATCH v5 2/3] usb: gadget: udc: core: Invoke usb_gadget_connect only when started

[Badhri Jagan Sridharan]

On Wed, May 31, 2023 at 3:55 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, May 31, 2023 at 03:40:26PM -0700, Badhri Jagan Sridharan wrote:
> > On Wed, May 31, 2023 at 10:55 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, May 31, 2023 at 04:02:02AM +0000, Badhri Jagan Sridharan wrote:
> > > > usb_udc_connect_control() does not check to see if the udc has already
> > > > been started. This causes gadget->ops->pullup to be called through
> > > > usb_gadget_connect() when invoked from usb_udc_vbus_handler() even
> > > > before usb_gadget_udc_start() is called. Guard this by checking for
> > > > udc->started in usb_udc_connect_control() before invoking
> > > > usb_gadget_connect().
> > >
> > > After a merged version of patches 1/3 and 3/3 have been applied, it
> > > seems like most of this will not be needed any more.  Maybe not any of
> > > it.
> >
> > Without the connect_lock introduced in this patch, wouldn't the
> > usb_gadget_connect()/
> > usb_gadget_disconnect() through soft_connect_store() race against
> > usb_gadget_connect()/ usb_gadget_disconnect() through
> > usb_udc_connect_control() ?
>
> Okay, yes, that's a good point.  It needs to be mentioned in the patch
> description so that people will understand it is the real reason for
> this change.

Thanks Alan !
I had posted the v6 version of the series with 1/3 and 3/3 of v5
squashed together. I have made changes to address your concerns in v5.
Instead of adding a new lock, I used the connect_lock in this patch to
protect the allow_connect flag.
Eager to know your thoughts !

Regards,
Badhri

>
> Alan Stern

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

[Badhri Jagan Sridharan]

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(+)

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
@@ -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;
 	}
 
 	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)
-- 
2.41.0.rc0.172.g3f132b7071-goog

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

[Alan Stern]

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

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

Re: [PATCH v5 1/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing

[Alan Stern]

On Wed, May 31, 2023 at 04:02:01AM +0000, Badhri Jagan Sridharan wrote:
> usb_udc_vbus_handler() can be invoked from interrupt context by irq
> handlers of the gadget drivers, however, usb_udc_connect_control() has
> to run in non-atomic context due to the following:
> a. Some of the gadget driver implementations expect the ->pullup
>    callback to be invoked in non-atomic context.
> b. usb_gadget_disconnect() acquires udc_lock which is a mutex.
> 
> Hence offload invocation of usb_udc_connect_control()
> to workqueue.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1016fc0c096c ("USB: gadget: Fix obscure lockdep violation for udc_mutex")
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
> Changes since v1:
> - Address Alan Stern's comment on usb_udc_vbus_handler invocation from
>   atomic context:
> * vbus_events_lock is now a spinlock and allocations in
> * usb_udc_vbus_handler are atomic now.
> 
> Changes since v2:
> - Addressing Alan Stern's comments:
> ** connect_lock is now held by callers of
> * usb_gadget_pullup_update_locked() and gadget_(un)bind_driver() does
> * notdirectly hold the lock.
> 
> ** Both usb_gadget_(dis)connect() and usb_udc_vbus_handler() would
> * set/clear udc->vbus and invoke usb_gadget_pullup_update_locked.
> 
> ** Add "unbinding" to prevent new connections after the gadget is being
> * unbound.
> 
> Changes since v3:
> ** Made a minor cleanup which I missed to do in v3 in
> * usb_udc_vbus_handler().
> 
> Changes since v4:
> - Addressing Alan Stern's comments:
> ** usb_udc_vbus_handler() now offloads invocation of usb_udc_connect_control()
> * from workqueue.
> 
> ** Dropped vbus_events list as this was redundant. Updating to the
> * latest value is suffice
> ---
>  drivers/usb/gadget/udc/core.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 52e6d2e84e35..44a9f32679b5 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -48,6 +48,7 @@ struct usb_udc {
>  	struct list_head		list;
>  	bool				vbus;
>  	bool				started;
> +	struct work_struct		vbus_work;
>  };
>  
>  static struct class *udc_class;
> @@ -1086,6 +1087,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
>  		usb_gadget_disconnect(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);
> +}
> +
>  /**
>   * usb_udc_vbus_handler - updates the udc core vbus status, and try to
>   * connect or disconnect gadget
> @@ -1094,6 +1102,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
>   *
>   * The udc driver calls it when it wants to connect or disconnect gadget
>   * according to vbus status.
> + *
> + * This function can be invoked from interrupt context by irq handlers of the gadget drivers,
> + * however, usb_udc_connect_control() has to run in non-atomic context due to the following:
> + * a. Some of the gadget driver implementations expect the ->pullup callback to be invoked in
> + * non-atomic context.
> + * b. usb_gadget_disconnect() acquires udc_lock which is a mutex.
> + * Hence offload invocation of usb_udc_connect_control() to workqueue.

Comments should be wrapped after about 76 columns (unless there is some 
very good reason not to).

>   */
>  void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
>  {
> @@ -1101,7 +1116,7 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
>  
>  	if (udc) {
>  		udc->vbus = status;
> -		usb_udc_connect_control(udc);
> +		schedule_work(&udc->vbus_work);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
> @@ -1328,6 +1343,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
>  	mutex_lock(&udc_lock);
>  	list_add_tail(&udc->list, &udc_list);
>  	mutex_unlock(&udc_lock);
> +	INIT_WORK(&udc->vbus_work, vbus_event_work);
>  
>  	ret = device_add(&udc->dev);
>  	if (ret)
> @@ -1558,6 +1574,7 @@ static void gadget_unbind_driver(struct device *dev)
>  
>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>  
> +	cancel_work_sync(&udc->vbus_work);
>  	usb_gadget_disconnect(gadget);
>  	usb_gadget_disable_async_callbacks(udc);
>  	if (gadget->irq)

I'm not in love with this, because there's nothing here to prevent the 
work item from being queued again right after it is cancelled.  Patch 
3/3 in the series will fix this, but in the meantime this window will 
exist.

Maybe it would be better to merge the 3/3 patch with this one.  They are 
very closely related, after all, since the other patch addresses the 
matter of not allowing the work item to do anything bad at the wrong 
time.

Alan Stern

Re: [PATCH v5 1/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing

[Badhri Jagan Sridharan]

On Wed, May 31, 2023 at 10:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, May 31, 2023 at 04:02:01AM +0000, Badhri Jagan Sridharan wrote:
> > usb_udc_vbus_handler() can be invoked from interrupt context by irq
> > handlers of the gadget drivers, however, usb_udc_connect_control() has
> > to run in non-atomic context due to the following:
> > a. Some of the gadget driver implementations expect the ->pullup
> >    callback to be invoked in non-atomic context.
> > b. usb_gadget_disconnect() acquires udc_lock which is a mutex.
> >
> > Hence offload invocation of usb_udc_connect_control()
> > to workqueue.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 1016fc0c096c ("USB: gadget: Fix obscure lockdep violation for udc_mutex")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> > Changes since v1:
> > - Address Alan Stern's comment on usb_udc_vbus_handler invocation from
> >   atomic context:
> > * vbus_events_lock is now a spinlock and allocations in
> > * usb_udc_vbus_handler are atomic now.
> >
> > Changes since v2:
> > - Addressing Alan Stern's comments:
> > ** connect_lock is now held by callers of
> > * usb_gadget_pullup_update_locked() and gadget_(un)bind_driver() does
> > * notdirectly hold the lock.
> >
> > ** Both usb_gadget_(dis)connect() and usb_udc_vbus_handler() would
> > * set/clear udc->vbus and invoke usb_gadget_pullup_update_locked.
> >
> > ** Add "unbinding" to prevent new connections after the gadget is being
> > * unbound.
> >
> > Changes since v3:
> > ** Made a minor cleanup which I missed to do in v3 in
> > * usb_udc_vbus_handler().
> >
> > Changes since v4:
> > - Addressing Alan Stern's comments:
> > ** usb_udc_vbus_handler() now offloads invocation of usb_udc_connect_control()
> > * from workqueue.
> >
> > ** Dropped vbus_events list as this was redundant. Updating to the
> > * latest value is suffice
> > ---
> >  drivers/usb/gadget/udc/core.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index 52e6d2e84e35..44a9f32679b5 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -48,6 +48,7 @@ struct usb_udc {
> >       struct list_head                list;
> >       bool                            vbus;
> >       bool                            started;
> > +     struct work_struct              vbus_work;
> >  };
> >
> >  static struct class *udc_class;
> > @@ -1086,6 +1087,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
> >               usb_gadget_disconnect(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);
> > +}
> > +
> >  /**
> >   * usb_udc_vbus_handler - updates the udc core vbus status, and try to
> >   * connect or disconnect gadget
> > @@ -1094,6 +1102,13 @@ static void usb_udc_connect_control(struct usb_udc *udc)
> >   *
> >   * The udc driver calls it when it wants to connect or disconnect gadget
> >   * according to vbus status.
> > + *
> > + * This function can be invoked from interrupt context by irq handlers of the gadget drivers,
> > + * however, usb_udc_connect_control() has to run in non-atomic context due to the following:
> > + * a. Some of the gadget driver implementations expect the ->pullup callback to be invoked in
> > + * non-atomic context.
> > + * b. usb_gadget_disconnect() acquires udc_lock which is a mutex.
> > + * Hence offload invocation of usb_udc_connect_control() to workqueue.
>
> Comments should be wrapped after about 76 columns (unless there is some
> very good reason not to).

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

>
> >   */
> >  void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
> >  {
> > @@ -1101,7 +1116,7 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
> >
> >       if (udc) {
> >               udc->vbus = status;
> > -             usb_udc_connect_control(udc);
> > +             schedule_work(&udc->vbus_work);
> >       }
> >  }
> >  EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
> > @@ -1328,6 +1343,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
> >       mutex_lock(&udc_lock);
> >       list_add_tail(&udc->list, &udc_list);
> >       mutex_unlock(&udc_lock);
> > +     INIT_WORK(&udc->vbus_work, vbus_event_work);
> >
> >       ret = device_add(&udc->dev);
> >       if (ret)
> > @@ -1558,6 +1574,7 @@ static void gadget_unbind_driver(struct device *dev)
> >
> >       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> >
> > +     cancel_work_sync(&udc->vbus_work);
> >       usb_gadget_disconnect(gadget);
> >       usb_gadget_disable_async_callbacks(udc);
> >       if (gadget->irq)
>
> I'm not in love with this, because there's nothing here to prevent the
> work item from being queued again right after it is cancelled.  Patch
> 3/3 in the series will fix this, but in the meantime this window will
> exist.
>
> Maybe it would be better to merge the 3/3 patch with this one.  They are
> very closely related, after all, since the other patch addresses the
> matter of not allowing the work item to do anything bad at the wrong
> time.

Done ! v6. now squashes 1/3 with 3/3.

Thanks a lot,
Badhri

>
> Alan Stern