[PATCH v2 0/4] usb: gadget: udc: Rework when the gadget connects

[PATCH v2 0/4] usb: gadget: udc: Rework when the gadget connects

[Alexey Charkov]

The UDC core decides whether to enable the gadget's pullup in several
places: bind and unbind, VBUS events, usb_gadget_{de,}activate() and the
soft_connect attribute. Each applies its own rules, and
gadget->connected records both the pullup state and a request to connect
later. Since commit 62911bc82b03 ("usb: gadget: udc: skip pullup() if
already connected"), a VBUS event before bind leaves the gadget
invisible to the host until it is rebound. That happens on every boot
of an RK3576 board whose Type-C port controller sets the device role
before userspace binds a configfs gadget.

Patch 1 is the v1 fix, small enough for stable. Patches 2-4 implement
Alan's scheme: the gadget connects only while a driver is bound, VBUS is
present, the driver hasn't deactivated it and userspace hasn't
disconnected it through soft_connect. One helper records that in
udc->allow_connect under connect_lock and switches the pullup to match.
The UDC starts and stops only at bind and unbind, and the deactivation
and soft-disconnect requests are forgotten at unbind.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
Changes in v2:
- Add patches 2-4, reworking the connection control as Alan suggested.
- Link to v1: https://patch.msgid.link/20261112-udc-connect-v1-1-03c73259642b@flipper.net

---
Alexey Charkov (4):
      usb: gadget: udc: Clear a pending connection when starting the UDC
      usb: gadget: udc: Forget a deactivation when the gadget driver unbinds
      usb: gadget: configfs: Leave resetting the deactivation to the UDC core
      usb: gadget: udc: Decide on connecting in one place

 drivers/usb/gadget/configfs.c |   1 -
 drivers/usb/gadget/udc/core.c | 197 +++++++++++++++++++++++-------------------
 include/linux/usb/gadget.h    |   4 +-
 3 files changed, 111 insertions(+), 91 deletions(-)
---
base-commit: 6d582d78fad9d828e575ca0cdcff8ea47a77220b
change-id: 20261112-udc-connect-61e0f11cf93b

Best regards,
--  
Alexey Charkov <alchark@flipper.net>

[PATCH v2 1/4] usb: gadget: udc: Clear a pending connection when starting the UDC

[Alexey Charkov]

Since commit 62911bc82b03 ("usb: gadget: udc: skip pullup() if already
connected"), usb_gadget_connect_locked() returns early when
gadget->connected is set. A connection requested while the UDC is
stopped sets that flag too, without enabling the pullup. A later
gadget_bind_driver() or soft_connect "connect" then finds it set and
never enables the pullup, so the host doesn't see the gadget until it is
unbound and bound again.

This happens whenever a UDC driver reports VBUS through
usb_udc_vbus_handler() while no gadget driver is bound, e.g. when a
Type-C port controller switches the controller to the device role at
boot, seconds before userspace binds a configfs gadget.

The pullup is never enabled while the UDC is stopped, so clear
gadget->connected once it starts. The bind connects according to the
VBUS state that the core tracks anyway, and a function that deactivates
the gadget during bind still has the connection recorded for
usb_gadget_activate().

Fixes: 62911bc82b03 ("usb: gadget: udc: skip pullup() if already connected")
Cc: stable@vger.kernel.org
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/usb/gadget/udc/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d5844384c368..577bc15f61a1 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1236,8 +1236,16 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
 	}
 
 	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
-	if (!ret)
+	if (!ret) {
 		udc->started = true;
+		/*
+		 * The pullup is never enabled while the UDC is stopped, so any
+		 * connection recorded meanwhile was only a request. Forget it,
+		 * so that the next usb_gadget_connect_locked() enables the
+		 * pullup rather than returning early.
+		 */
+		udc->gadget->connected = false;
+	}
 
 	return ret;
 }

-- 
2.56.0

Re: [PATCH v2 1/4] usb: gadget: udc: Clear a pending connection when starting the UDC

[sashiko-bot]

> Since commit 62911bc82b03 ("usb: gadget: udc: skip pullup() if already
> connected"), usb_gadget_connect_locked() returns early when
> gadget->connected is set. A connection requested while the UDC is
> stopped sets that flag too, without enabling the pullup. A later
> gadget_bind_driver() or soft_connect "connect" then finds it set and
> never enables the pullup, so the host doesn't see the gadget until it is
> unbound and bound again.
> 
> This happens whenever a UDC driver reports VBUS through
> usb_udc_vbus_handler() while no gadget driver is bound, e.g. when a
> Type-C port controller switches the controller to the device role at
> boot, seconds before userspace binds a configfs gadget.
> 
> The pullup is never enabled while the UDC is stopped, so clear
> gadget->connected once it starts. The bind connects according to the
> VBUS state that the core tracks anyway, and a function that deactivates
> the gadget during bind still has the connection recorded for
> usb_gadget_activate().
> 
> Fixes: 62911bc82b03 ("usb: gadget: udc: skip pullup() if already connected")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexey Charkov <alchark@flipper.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-udc-connect-v2-0-f51b11f07eb8@flipper.net?part=1

[PATCH v2 2/4] usb: gadget: udc: Forget a deactivation when the gadget driver unbinds

[Alexey Charkov]

A gadget driver deactivates the gadget through usb_gadget_deactivate()
until its functions are ready, so the deactivation means nothing to the
next driver. Yet the UDC core keeps gadget->deactivated set across
unbind. Only configfs resets it, from its own unbind callback and
without connect_lock, so a legacy gadget driver that unbinds while
deactivated leaves the next driver unable to connect.

Clear the flag under connect_lock once the driver has unbound, including
when the bind fails after the driver has deactivated the gadget.

Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/usb/gadget/udc/core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 577bc15f61a1..84dc0820c2cb 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1647,6 +1647,13 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
 	return 1;
 }
 
+/* Forget the state that a gadget driver left behind, so the next starts afresh */
+static void usb_udc_forget_driver_locked(struct usb_udc *udc)
+	__must_hold(&udc->connect_lock)
+{
+	udc->gadget->deactivated = false;
+}
+
 static int gadget_bind_driver(struct device *dev)
 {
 	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
@@ -1705,6 +1712,11 @@ static int gadget_bind_driver(struct device *dev)
 		dev_err(&udc->dev, "failed to start %s: %d\n",
 			driver->function, ret);
 
+	/* The driver may have deactivated the gadget before failing */
+	mutex_lock(&udc->connect_lock);
+	usb_udc_forget_driver_locked(udc);
+	mutex_unlock(&udc->connect_lock);
+
 	mutex_lock(&udc_lock);
 	udc->driver = NULL;
 	driver->is_bound = false;
@@ -1734,6 +1746,7 @@ static void gadget_unbind_driver(struct device *dev)
 
 	mutex_lock(&udc->connect_lock);
 	usb_gadget_udc_stop_locked(udc);
+	usb_udc_forget_driver_locked(udc);
 	mutex_unlock(&udc->connect_lock);
 
 	mutex_lock(&udc_lock);

-- 
2.56.0

Re: [PATCH v2 2/4] usb: gadget: udc: Forget a deactivation when the gadget driver unbinds

[sashiko-bot]

> A gadget driver deactivates the gadget through usb_gadget_deactivate()
> until its functions are ready, so the deactivation means nothing to the
> next driver. Yet the UDC core keeps gadget->deactivated set across
> unbind. Only configfs resets it, from its own unbind callback and
> without connect_lock, so a legacy gadget driver that unbinds while
> deactivated leaves the next driver unable to connect.
> 
> Clear the flag under connect_lock once the driver has unbound, including
> when the bind fails after the driver has deactivated the gadget.
> 
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Alexey Charkov <alchark@flipper.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-udc-connect-v2-0-f51b11f07eb8@flipper.net?part=2

[PATCH v2 3/4] usb: gadget: configfs: Leave resetting the deactivation to the UDC core

[Alexey Charkov]

Commit bf95c4d4630c ("usb: gadget: configfs: clear deactivation flag in
configfs_composite_unbind()") made configfs reset gadget->deactivated on
unbind, so that a rebound gadget could connect again. The UDC core now
does that for every gadget driver, under connect_lock, which guards the
flag everywhere else.

Drop the unlocked write from configfs_composite_unbind().

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/usb/gadget/configfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 51df6d1d1487..c0f9e6625b64 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1849,7 +1849,6 @@ static void configfs_composite_unbind(struct usb_gadget *gadget)
 	spin_lock_irqsave(&gi->spinlock, flags);
 	cdev->gadget = NULL;
 	cdev->deactivations = 0;
-	gadget->deactivated = false;
 	set_gadget_data(gadget, NULL);
 	spin_unlock_irqrestore(&gi->spinlock, flags);
 }

-- 
2.56.0

Re: [PATCH v2 3/4] usb: gadget: configfs: Leave resetting the deactivation to the UDC core

[sashiko-bot]

> Commit bf95c4d4630c ("usb: gadget: configfs: clear deactivation flag in
> configfs_composite_unbind()") made configfs reset gadget->deactivated on
> unbind, so that a rebound gadget could connect again. The UDC core now
> does that for every gadget driver, under connect_lock, which guards the
> flag everywhere else.
> 
> Drop the unlocked write from configfs_composite_unbind().
> 
> Signed-off-by: Alexey Charkov <alchark@flipper.net>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-udc-connect-v2-0-f51b11f07eb8@flipper.net?part=3

[PATCH v2 4/4] usb: gadget: udc: Decide on connecting in one place

[Alexey Charkov]

The UDC core connects or disconnects the gadget on bind and unbind, on
VBUS events, in usb_gadget_{de,}activate() and through the soft_connect
attribute, and each path applies its own rules. gadget->connected
records both whether the pullup is enabled and whether to enable it once
that becomes possible, so the paths misread each other's state: a VBUS
event before bind kept the pullup off until the previous patch cleared
the flag on UDC start. soft_connect also starts and stops the UDC behind
the gadget driver's back, and its "connect" overrides both VBUS and
deactivation.

Settle the rules in one place. The gadget may connect only while a
gadget driver is bound, VBUS is present, the driver hasn't deactivated
the gadget and userspace hasn't disconnected it through soft_connect.
Have usb_udc_update_connection_locked() record that in
udc->allow_connect under connect_lock and connect or disconnect the
gadget to match, and call it wherever one of those conditions changes.
gadget->connected then only tracks the pullup, as the UDC drivers that
read it already assume, so the clearing on UDC start can go.

Reduce soft_connect to setting or clearing a disconnect request under
connect_lock, rather than the gadget's device lock, and forget the
request at unbind like the deactivation. If the pullup can't follow,
keep the old request and report the error, so that the request and the
pullup still agree. Leave starting and stopping the UDC to bind and
unbind alone: the attribute is documented to control the pullup, not the
controller.

Let the helper succeed on a UDC without a pullup control, as there is
nothing to switch there. Activation and deactivation keep working on
such UDCs, and binding them works again: it has most likely failed with
-EOPNOTSUPP since commit 0ea39e030a80 ("usb: gadget: udc: Handle
gadget_connect failure during bind operation"), but I don't have any
affected hardware to confirm it.

Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/usb/gadget/udc/core.c | 194 +++++++++++++++++++++---------------------
 include/linux/usb/gadget.h    |   4 +-
 2 files changed, 99 insertions(+), 99 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 84dc0820c2cb..e592705de55e 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -38,14 +38,18 @@ 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_connect: Indicates whether UDC is allowed to be pulled up.
- * Set/cleared by gadget_(un)bind_driver() after gadget driver is bound or
- * unbound.
+ * @bound: True while a gadget driver is bound and the UDC is started for it.
+ * @soft_disconnected: True if userspace has disconnected the gadget through
+ * the soft_connect attribute. Forgotten when the gadget driver unbinds.
+ * @allow_connect: True if the gadget may connect to the host: a gadget driver
+ * is bound, VBUS is present, the driver hasn't deactivated the gadget, and
+ * userspace hasn't disconnected it. Only usb_udc_update_connection_locked()
+ * sets it, and it connects or disconnects the gadget to match.
  * @vbus_work: work routine to handle VBUS status change notifications.
- * @connect_lock: protects udc->started, gadget->connect,
- * gadget->allow_connect and gadget->deactivate. The routines
+ * @connect_lock: protects udc->started, udc->bound, udc->soft_disconnected,
+ * udc->allow_connect, gadget->connected and gadget->deactivated. The routines
  * usb_gadget_connect_locked(), usb_gadget_disconnect_locked(),
- * usb_udc_connect_control_locked(), usb_gadget_udc_start_locked() and
+ * usb_udc_update_connection_locked(), usb_gadget_udc_start_locked() and
  * usb_gadget_udc_stop_locked() are called with this lock held.
  *
  * This represents the internal data structure which is used by the UDC-class
@@ -59,6 +63,8 @@ struct usb_udc {
 	struct list_head		list;
 	bool				vbus;
 	bool				started;
+	bool				bound;
+	bool				soft_disconnected;
 	bool				allow_connect;
 	struct work_struct		vbus_work;
 	struct mutex			connect_lock;
@@ -715,20 +721,13 @@ static int usb_gadget_connect_locked(struct usb_gadget *gadget)
 		goto out;
 	}
 
-	if (gadget->connected)
+	/*
+	 * If the gadget may not connect yet, usb_udc_update_connection_locked()
+	 * will connect it once it may.
+	 */
+	if (gadget->connected || !gadget->udc->allow_connect)
 		goto out;
 
-	if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) {
-		/*
-		 * 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.
-		 */
-		gadget->connected = true;
-		goto out;
-	}
-
 	ret = gadget->ops->pullup(gadget, 1);
 	if (!ret)
 		gadget->connected = 1;
@@ -747,6 +746,10 @@ static int usb_gadget_connect_locked(struct usb_gadget *gadget)
  * enumerating this gadget when the pullup is active and a VBUS session
  * is active (the link is powered).
  *
+ * Does nothing while the gadget may not connect: no gadget driver is bound,
+ * VBUS is absent, the gadget is deactivated or userspace has disconnected it.
+ * The UDC core connects the gadget by itself once all of that changes.
+ *
  * Returns zero on success, else negative errno.
  */
 int usb_gadget_connect(struct usb_gadget *gadget)
@@ -774,15 +777,6 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
 	if (!gadget->connected)
 		goto out;
 
-	if (gadget->deactivated || !gadget->udc->started) {
-		/*
-		 * If gadget is deactivated we only save new state.
-		 * Gadget will stay disconnected after activation.
-		 */
-		gadget->connected = false;
-		goto out;
-	}
-
 	ret = gadget->ops->pullup(gadget, 0);
 	if (!ret)
 		gadget->connected = 0;
@@ -809,6 +803,10 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
  * Following a successful disconnect, invoke the ->disconnect() callback
  * for the current gadget driver so that UDC drivers don't need to.
  *
+ * The gadget stays disconnected until usb_gadget_connect() is called or the
+ * UDC core connects it again on the next change of VBUS, binding, activation
+ * or soft_connect.
+ *
  * Returns zero on success, else negative errno.
  */
 int usb_gadget_disconnect(struct usb_gadget *gadget)
@@ -823,6 +821,32 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
 }
 EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
 
+static bool usb_udc_may_connect(struct usb_udc *udc)
+	__must_hold(&udc->connect_lock)
+{
+	return udc->bound && udc->vbus && !udc->gadget->deactivated &&
+	       !udc->soft_disconnected;
+}
+
+/*
+ * Decide whether the gadget may connect and connect or disconnect it to match.
+ * Call this after changing any of the state that usb_udc_may_connect() checks.
+ */
+static int usb_udc_update_connection_locked(struct usb_udc *udc)
+	__must_hold(&udc->connect_lock)
+{
+	udc->allow_connect = usb_udc_may_connect(udc);
+
+	/* Without a pullup control there is nothing to switch */
+	if (!udc->gadget->ops->pullup)
+		return 0;
+
+	if (udc->allow_connect)
+		return usb_gadget_connect_locked(udc->gadget);
+
+	return usb_gadget_disconnect_locked(udc->gadget);
+}
+
 /**
  * usb_gadget_deactivate - deactivate function which is not ready to work
  * @gadget: the peripheral being deactivated
@@ -845,18 +869,13 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
 	if (gadget->deactivated)
 		goto unlock;
 
-	if (gadget->connected) {
-		ret = usb_gadget_disconnect_locked(gadget);
-		if (ret)
-			goto unlock;
-
-		/*
-		 * If gadget was being connected before deactivation, we want
-		 * to reconnect it in usb_gadget_activate().
-		 */
-		gadget->connected = true;
-	}
 	gadget->deactivated = true;
+	ret = usb_udc_update_connection_locked(gadget->udc);
+	if (ret) {
+		/* Still connected, so stay active */
+		gadget->deactivated = false;
+		gadget->udc->allow_connect = usb_udc_may_connect(gadget->udc);
+	}
 
 unlock:
 	mutex_unlock(&gadget->udc->connect_lock);
@@ -871,7 +890,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_deactivate);
  * @gadget: the peripheral being activated
  *
  * This routine activates gadget which was previously deactivated with
- * usb_gadget_deactivate() call. It calls usb_gadget_connect() if needed.
+ * usb_gadget_deactivate() call, and connects it if it may connect.
  *
  * This routine may sleep; it must not be called in interrupt context.
  *
@@ -886,15 +905,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
 		goto unlock;
 
 	gadget->deactivated = false;
-
-	/*
-	 * If gadget has been connected before deactivation, or became connected
-	 * while it was being deactivated, we call usb_gadget_connect().
-	 */
-	if (gadget->connected) {
-		gadget->connected = false;
-		ret = usb_gadget_connect_locked(gadget);
-	}
+	ret = usb_udc_update_connection_locked(gadget->udc);
 
 unlock:
 	mutex_unlock(&gadget->udc->connect_lock);
@@ -1147,21 +1158,12 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
 
 /* ------------------------------------------------------------------------- */
 
-/* Acquire connect_lock before calling this function. */
-static int usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
-{
-	if (udc->vbus)
-		return usb_gadget_connect_locked(udc->gadget);
-	else
-		return 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);
 
 	mutex_lock(&udc->connect_lock);
-	usb_udc_connect_control_locked(udc);
+	usb_udc_update_connection_locked(udc);
 	mutex_unlock(&udc->connect_lock);
 }
 
@@ -1175,12 +1177,12 @@ static void vbus_event_work(struct work_struct *work)
  * 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:
+ * the gadget drivers, however, usb_udc_update_connection_locked() 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.
+ * Hence offload invocation of usb_udc_update_connection_locked() to workqueue.
  */
 void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
 {
@@ -1236,16 +1238,8 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
 	}
 
 	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
-	if (!ret) {
+	if (!ret)
 		udc->started = true;
-		/*
-		 * The pullup is never enabled while the UDC is stopped, so any
-		 * connection recorded meanwhile was only a request. Forget it,
-		 * so that the next usb_gadget_connect_locked() enables the
-		 * pullup rather than returning early.
-		 */
-		udc->gadget->connected = false;
-	}
 
 	return ret;
 }
@@ -1652,6 +1646,7 @@ static void usb_udc_forget_driver_locked(struct usb_udc *udc)
 	__must_hold(&udc->connect_lock)
 {
 	udc->gadget->deactivated = false;
+	udc->soft_disconnected = false;
 }
 
 static int gadget_bind_driver(struct device *dev)
@@ -1686,8 +1681,8 @@ static int gadget_bind_driver(struct device *dev)
 		goto err_start;
 	}
 	usb_gadget_enable_async_callbacks(udc);
-	udc->allow_connect = true;
-	ret = usb_udc_connect_control_locked(udc);
+	udc->bound = true;
+	ret = usb_udc_update_connection_locked(udc);
 	if (ret)
 		goto err_connect_control;
 
@@ -1697,6 +1692,7 @@ static int gadget_bind_driver(struct device *dev)
 	return 0;
 
  err_connect_control:
+	udc->bound = false;
 	udc->allow_connect = false;
 	usb_gadget_disable_async_callbacks(udc);
 	if (gadget->irq)
@@ -1733,15 +1729,17 @@ static void gadget_unbind_driver(struct device *dev)
 
 	dev_dbg(&udc->dev, "unbinding gadget driver [%s]\n", driver->function);
 
-	udc->allow_connect = false;
-	cancel_work_sync(&udc->vbus_work);
 	mutex_lock(&udc->connect_lock);
-	usb_gadget_disconnect_locked(gadget);
+	udc->bound = false;
+	usb_udc_update_connection_locked(udc);
 	usb_gadget_disable_async_callbacks(udc);
 	if (gadget->irq)
 		synchronize_irq(gadget->irq);
 	mutex_unlock(&udc->connect_lock);
 
+	/* A VBUS event can no longer connect the gadget, so just wait it out */
+	cancel_work_sync(&udc->vbus_work);
+
 	udc->driver->unbind(gadget);
 
 	mutex_lock(&udc->connect_lock);
@@ -1826,35 +1824,37 @@ static ssize_t soft_connect_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t n)
 {
 	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
-	ssize_t			ret;
-
-	device_lock(&udc->gadget->dev);
-	if (!udc->driver) {
-		dev_err(dev, "soft-connect without a gadget driver\n");
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
+	bool			disconnect, was_disconnected;
+	int			ret;
 
 	if (sysfs_streq(buf, "connect")) {
-		mutex_lock(&udc->connect_lock);
-		usb_gadget_udc_start_locked(udc);
-		usb_gadget_connect_locked(udc->gadget);
-		mutex_unlock(&udc->connect_lock);
+		disconnect = false;
 	} else if (sysfs_streq(buf, "disconnect")) {
-		mutex_lock(&udc->connect_lock);
-		usb_gadget_disconnect_locked(udc->gadget);
-		usb_gadget_udc_stop_locked(udc);
-		mutex_unlock(&udc->connect_lock);
+		disconnect = true;
 	} else {
 		dev_err(dev, "unsupported command '%s'\n", buf);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
-	ret = n;
-out:
-	device_unlock(&udc->gadget->dev);
-	return ret;
+	guard(mutex)(&udc->connect_lock);
+
+	if (!udc->bound) {
+		dev_err(dev, "soft-connect without a gadget driver\n");
+		return -EOPNOTSUPP;
+	}
+
+	/* "connect" only lifts the disconnect, it doesn't override VBUS etc. */
+	was_disconnected = udc->soft_disconnected;
+	udc->soft_disconnected = disconnect;
+	ret = usb_udc_update_connection_locked(udc);
+	if (ret) {
+		/* The pullup didn't change, so neither does the request */
+		udc->soft_disconnected = was_disconnected;
+		udc->allow_connect = usb_udc_may_connect(udc);
+		return ret;
+	}
+
+	return n;
 }
 static DEVICE_ATTR_WO(soft_connect);
 
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 8285b19a25e0..53ace954d189 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -412,8 +412,8 @@ struct usb_gadget_ops {
  *	u_ether.c to improve performance.
  * @is_selfpowered: if the gadget is self-powered.
  * @deactivated: True if gadget is deactivated - in deactivated state it cannot
- *	be connected.
- * @connected: True if gadget is connected.
+ *	be connected. Cleared when the gadget driver unbinds.
+ * @connected: True if the gadget's pullup is enabled.
  * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
  *	indicates that it supports LPM as per the LPM ECN & errata.
  * @wakeup_capable: True if gadget is capable of sending remote wakeup.

-- 
2.56.0

Re: [PATCH v2 4/4] usb: gadget: udc: Decide on connecting in one place

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: udc: stale gadget->connected if pullup disable
  fails on unbind
--

commit 8198ac49b976bc4f15b1f15c4abb55ae61ceb982
Author: Alexey Charkov <alchark@flipper.net>

usb: gadget: udc: Decide on connecting in one place

This commit centralizes the UDC connection rules in
usb_udc_update_connection_locked() so that gadget->connected only tracks
whether the pullup is enabled.

> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 84dc0820c2cb9..e592705de55e4 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
[ ... ]
> @@ -1236,16 +1238,8 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
>  	}
>  
>  	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
> -	if (!ret) {
> +	if (!ret)
>  		udc->started = true;
> -		/*
> -		 * The pullup is never enabled while the UDC is stopped, so any
> -		 * connection recorded meanwhile was only a request. Forget it,
> -		 * so that the next usb_gadget_connect_locked() enables the
> -		 * pullup rather than returning early.
> -		 */
> -		udc->gadget->connected = false;
> -	}
>  
>  	return ret;
>  }
[ ... ]
> @@ -1733,15 +1729,17 @@ static void gadget_unbind_driver(struct device *dev)
>  
>  	dev_dbg(&udc->dev, "unbinding gadget driver [%s]\n", driver->function);
>  
> -	udc->allow_connect = false;
> -	cancel_work_sync(&udc->vbus_work);
>  	mutex_lock(&udc->connect_lock);
> -	usb_gadget_disconnect_locked(gadget);
> +	udc->bound = false;
> +	usb_udc_update_connection_locked(udc);
>  	usb_gadget_disable_async_callbacks(udc);
>  	if (gadget->irq)
>  		synchronize_irq(gadget->irq);
>  	mutex_unlock(&udc->connect_lock);
>  
> +	/* A VBUS event can no longer connect the gadget, so just wait it out */
> +	cancel_work_sync(&udc->vbus_work);

[Severity: Medium]
Can gadget->connected stay set across unbind and prevent the next bound
gadget driver from enabling the pullup?

In usb_gadget_disconnect_locked(), gadget->connected is only cleared when
gadget->ops->pullup(gadget, 0) returns 0, and some UDC drivers fail that
call when VBUS is already inactive:

drivers/usb/gadget/udc/fsl_udc_core.c:fsl_pullup() {
	...
	if (!udc->vbus_active)
		return -EOPNOTSUPP;
	...
}

When that happens during gadget_unbind_driver(), neither
usb_gadget_udc_stop_locked() nor usb_udc_forget_driver_locked() clears
gadget->connected, and usb_gadget_udc_start_locked() no longer resets it
either.  On the next gadget_bind_driver(), usb_gadget_connect_locked() sees
gadget->connected == 1 and returns early without calling
gadget->ops->pullup(gadget, 1).

Also, if vbus_work is pending when cancel_work_sync() runs after
usb_gadget_disable_async_callbacks(), a lingering gadget->connected == 1
causes vbus_event_work() -> usb_gadget_disconnect_locked() to invoke
driver->disconnect() after async callbacks have been disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-udc-connect-v2-0-f51b11f07eb8@flipper.net?part=4