* [PATCH v3 1/3] Revert "Revert "usb: gadget: udc: core: Invoke usb_gadget_connect only when started""
@ 2023-05-29 23:36 Badhri Jagan Sridharan
2023-05-29 23:36 ` [PATCH v3 2/3] Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup"" Badhri Jagan Sridharan
2023-05-29 23:37 ` [PATCH v3 3/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing Badhri Jagan Sridharan
0 siblings, 2 replies; 4+ messages in thread
From: Badhri Jagan Sridharan @ 2023-05-29 23:36 UTC (permalink / raw)
To: gregkh, stern, colin.i.king, xuetao09, quic_eserrao,
water.zhangjiantao, peter.chen, balbi, francesco, alistair,
stephan, bagasdotme, luca
Cc: linux-usb, linux-kernel, stable, Badhri Jagan Sridharan
This reverts commit f22e9b67f19ccc73de1ae04375d4b30684e261f8.
The regression reported in
https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/ is being
fixed in
commit 7d7863db7cc0 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing").
Hence reverting the revert.
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
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 52e6d2e84e35..583c339876ab 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.
@@ -48,6 +52,7 @@ struct usb_udc {
struct list_head list;
bool vbus;
bool started;
+ struct mutex connect_lock;
};
static struct class *udc_class;
@@ -687,17 +692,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;
@@ -706,10 +703,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;
@@ -724,22 +723,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;
@@ -751,10 +760,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;
@@ -774,6 +785,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);
/**
@@ -794,10 +829,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
@@ -807,6 +843,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);
@@ -830,6 +868,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
if (!gadget->deactivated)
goto out;
+ mutex_lock(&gadget->udc->connect_lock);
gadget->deactivated = false;
/*
@@ -837,7 +876,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);
@@ -1078,12 +1118,13 @@ 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);
}
/**
@@ -1099,10 +1140,12 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
{
struct usb_udc *udc = gadget->udc;
+ mutex_lock(&udc->connect_lock);
if (udc) {
udc->vbus = status;
- usb_udc_connect_control(udc);
+ usb_udc_connect_control_locked(udc);
}
+ mutex_unlock(&udc->connect_lock);
}
EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
@@ -1124,7 +1167,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
@@ -1135,8 +1178,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;
@@ -1153,7 +1199,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
@@ -1162,8 +1208,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");
@@ -1322,6 +1371,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
udc->gadget = gadget;
gadget->udc = udc;
+ mutex_init(&udc->connect_lock);
udc->started = false;
@@ -1523,11 +1573,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;
@@ -1558,12 +1612,14 @@ static void gadget_unbind_driver(struct device *dev)
kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
- 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;
@@ -1649,11 +1705,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;
base-commit: b4a4be8471846d96b0ac52a0e9e7d48005cc97e2
--
2.41.0.rc0.172.g3f132b7071-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 2/3] Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup""
2023-05-29 23:36 [PATCH v3 1/3] Revert "Revert "usb: gadget: udc: core: Invoke usb_gadget_connect only when started"" Badhri Jagan Sridharan
@ 2023-05-29 23:36 ` Badhri Jagan Sridharan
2023-05-29 23:37 ` [PATCH v3 3/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing Badhri Jagan Sridharan
1 sibling, 0 replies; 4+ messages in thread
From: Badhri Jagan Sridharan @ 2023-05-29 23:36 UTC (permalink / raw)
To: gregkh, stern, colin.i.king, xuetao09, quic_eserrao,
water.zhangjiantao, peter.chen, balbi, francesco, alistair,
stephan, bagasdotme, luca
Cc: linux-usb, linux-kernel, stable, Badhri Jagan Sridharan
This reverts commit 5e1617210aede9f1b91bb9819c93097b6da481f9.
The regression reported in
https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/ is being
fixed in
commit 7d7863db7cc0 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing").
Hence reverting the revert.
Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
drivers/usb/gadget/udc/core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 583c339876ab..4641153e9706 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -703,6 +703,9 @@ static int usb_gadget_connect_locked(struct usb_gadget *gadget)
goto out;
}
+ if (gadget->connected)
+ goto out;
+
if (gadget->deactivated || !gadget->udc->started) {
/*
* If gadget is deactivated we only save new state.
--
2.41.0.rc0.172.g3f132b7071-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 3/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing
2023-05-29 23:36 [PATCH v3 1/3] Revert "Revert "usb: gadget: udc: core: Invoke usb_gadget_connect only when started"" Badhri Jagan Sridharan
2023-05-29 23:36 ` [PATCH v3 2/3] Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup"" Badhri Jagan Sridharan
@ 2023-05-29 23:37 ` Badhri Jagan Sridharan
2023-05-29 23:39 ` Badhri Jagan Sridharan
1 sibling, 1 reply; 4+ messages in thread
From: Badhri Jagan Sridharan @ 2023-05-29 23:37 UTC (permalink / raw)
To: gregkh, stern, colin.i.king, xuetao09, quic_eserrao,
water.zhangjiantao, peter.chen, balbi, francesco, alistair,
stephan, bagasdotme, luca
Cc: linux-usb, linux-kernel, stable, Badhri Jagan Sridharan,
Francesco Dolcini
chipidea udc calls usb_udc_vbus_handler from udc_start gadget
ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler
processing.
============================================
WARNING: possible recursive locking detected
640-rc1-000-devel-00005-gcda3c69ebc14 #1 Not tainted
-------------------------------------------
CPU0
----
lock(&udc->connect_lock);
lock(&udc->connect_lock);
DEADLOCK
stack backtrace:
CPU: 1 PID: 566 Comm: echo Not tainted 640-rc1-000-devel-00005-gcda3c69ebc14 #1
Hardware name: Freescale iMX7 Dual (Device Tree)
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x70/0xb0
dump_stack_lvl from __lock_acquire+0x924/0x22c4
__lock_acquire from lock_acquire+0x100/0x370
lock_acquire from __mutex_lock+0xa8/0xfb4
__mutex_lock from mutex_lock_nested+0x1c/0x24
mutex_lock_nested from usb_udc_vbus_handler+0x1c/0x60
usb_udc_vbus_handler from ci_udc_start+0x74/0x9c
ci_udc_start from gadget_bind_driver+0x130/0x230
gadget_bind_driver from really_probe+0xd8/0x3fc
really_probe from __driver_probe_device+0x94/0x1f0
__driver_probe_device from driver_probe_device+0x2c/0xc4
driver_probe_device from __driver_attach+0x114/0x1cc
__driver_attach from bus_for_each_dev+0x7c/0xcc
bus_for_each_dev from bus_add_driver+0xd4/0x200
bus_add_driver from driver_register+0x7c/0x114
driver_register from usb_gadget_register_driver_owner+0x40/0xe0
usb_gadget_register_driver_owner from gadget_dev_desc_UDC_store+0xd4/0x110
gadget_dev_desc_UDC_store from configfs_write_iter+0xac/0x118
configfs_write_iter from vfs_write+0x1b4/0x40c
vfs_write from ksys_write+0x70/0xf8
ksys_write from ret_fast_syscall+0x0/0x1c
Fixes: 0db213ea8eed ("Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup""")
Cc: stable@vger.kernel.org
Reported-by: Stephan Gerhold <stephan@gerhold.net>
Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/
Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/
Reported-by: Alistair <alistair@alistair23.me>
Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@alistair23.me/
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.
---
drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------
1 file changed, 124 insertions(+), 145 deletions(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 4641153e9706..2aa914a0a195 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -38,9 +38,10 @@ static const struct bus_type gadget_bus_type;
* 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.
+ * functions. usb_gadget_pullup_update_locked called with this lock held.
+ * @vbus_events: list head for processing vbus updates on usb_udc_vbus_handler.
+ * @vbus_events_lock: protects vbus_events list
+ * @vbus_work: work item that invokes usb_gadget_pullup_update_locked.
*
* This represents the internal data structure which is used by the UDC-class
* to hold information about udc driver and gadget together.
@@ -53,6 +54,20 @@ struct usb_udc {
bool vbus;
bool started;
struct mutex connect_lock;
+ struct list_head vbus_events;
+ spinlock_t vbus_events_lock;
+ struct work_struct vbus_work;
+ bool unbinding;
+};
+
+/**
+ * struct vbus_event - used to notify vbus updates posted through usb_udc_vbus_handler.
+ * @vbus_on: true when vbus is on. false other wise.
+ * @node: list node for maintaining a list of pending updates to be processed.
+ */
+struct vbus_event {
+ bool vbus_on;
+ struct list_head node;
};
static struct class *udc_class;
@@ -693,40 +708,46 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
/* Internal version of usb_gadget_connect needs to be called with connect_lock held. */
-static int usb_gadget_connect_locked(struct usb_gadget *gadget)
+static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
__must_hold(&gadget->udc->connect_lock)
{
int ret = 0;
+ bool connect = !gadget->deactivated && gadget->udc->started && gadget->udc->vbus &&
+ !gadget->udc->unbinding;
if (!gadget->ops->pullup) {
ret = -EOPNOTSUPP;
goto out;
}
- if (gadget->connected)
- goto out;
-
- 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;
+ if (connect != gadget->connected) {
+ ret = gadget->ops->pullup(gadget, connect);
+ if (!ret)
+ gadget->connected = connect;
+ mutex_lock(&udc_lock);
+ if (!connect)
+ gadget->udc->driver->disconnect(gadget);
+ mutex_unlock(&udc_lock);
}
- ret = gadget->ops->pullup(gadget, 1);
- if (!ret)
- gadget->connected = 1;
-
out:
trace_usb_gadget_connect(gadget, ret);
return ret;
}
+static int usb_gadget_set_vbus(struct usb_gadget *gadget, bool vbus)
+{
+ int ret;
+
+ mutex_lock(&gadget->udc->connect_lock);
+ gadget->udc->vbus = vbus;
+ ret = usb_gadget_pullup_update_locked(gadget);
+ mutex_unlock(&gadget->udc->connect_lock);
+
+ return ret;
+}
+
/**
* usb_gadget_connect - software-controlled connect to USB host
* @gadget:the peripheral being connected
@@ -739,56 +760,10 @@ static int usb_gadget_connect_locked(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;
+ return usb_gadget_set_vbus(gadget, true);
}
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;
-
- if (!gadget->ops->pullup) {
- ret = -EOPNOTSUPP;
- goto out;
- }
-
- 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.
- *
- * udc should have been started before gadget being pulled down.
- */
- gadget->connected = false;
- goto out;
- }
-
- ret = gadget->ops->pullup(gadget, 0);
- if (!ret)
- gadget->connected = 0;
-
- mutex_lock(&udc_lock);
- if (gadget->udc->driver)
- gadget->udc->driver->disconnect(gadget);
- mutex_unlock(&udc_lock);
-
-out:
- trace_usb_gadget_disconnect(gadget, ret);
-
- return ret;
-}
-
/**
* usb_gadget_disconnect - software-controlled disconnect from USB host
* @gadget:the peripheral being disconnected
@@ -803,16 +778,22 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
* Returns zero on success, else negative errno.
*/
int usb_gadget_disconnect(struct usb_gadget *gadget)
+{
+ return usb_gadget_set_vbus(gadget, false);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
+
+static int usb_gadget_set_deactivate(struct usb_gadget *gadget, bool deactivate)
{
int ret;
mutex_lock(&gadget->udc->connect_lock);
- ret = usb_gadget_disconnect_locked(gadget);
+ gadget->deactivated = deactivate;
+ ret = usb_gadget_pullup_update_locked(gadget);
mutex_unlock(&gadget->udc->connect_lock);
return ret;
}
-EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
/**
* usb_gadget_deactivate - deactivate function which is not ready to work
@@ -829,26 +810,7 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
{
int ret = 0;
- if (gadget->deactivated)
- goto out;
-
- mutex_lock(&gadget->udc->connect_lock);
- 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;
-
-unlock:
- mutex_unlock(&gadget->udc->connect_lock);
-out:
+ ret = usb_gadget_set_deactivate(gadget, true);
trace_usb_gadget_deactivate(gadget, ret);
return ret;
@@ -868,21 +830,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
{
int ret = 0;
- if (!gadget->deactivated)
- goto out;
-
- mutex_lock(&gadget->udc->connect_lock);
- 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)
- ret = usb_gadget_connect_locked(gadget);
- mutex_unlock(&gadget->udc->connect_lock);
-
-out:
+ ret = usb_gadget_set_deactivate(gadget, false);
trace_usb_gadget_activate(gadget, ret);
return ret;
@@ -1121,13 +1069,28 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
/* ------------------------------------------------------------------------- */
-/* Acquire connect_lock before calling this function. */
-static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
+static void vbus_event_work(struct work_struct *work)
{
- if (udc->vbus && udc->started)
- usb_gadget_connect_locked(udc->gadget);
- else
- usb_gadget_disconnect_locked(udc->gadget);
+ struct vbus_event *event, *n;
+ struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work);
+ unsigned long flags;
+
+ spin_lock_irqsave(&udc->vbus_events_lock, flags);
+ list_for_each_entry_safe(event, n, &udc->vbus_events, node) {
+ list_del(&event->node);
+ /* OK to drop the lock here as it suffice to syncrhronize udc->vbus_events node
+ * retrieval and deletion against usb_udc_vbus_handler. usb_udc_vbus_handler does
+ * list_add_tail so n would be the same even if the lock is dropped.
+ */
+ spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
+ mutex_lock(&udc->connect_lock);
+ udc->vbus = event->vbus_on;
+ usb_gadget_pullup_update_locked(udc->gadget);
+ kfree(event);
+ mutex_unlock(&udc->connect_lock);
+ spin_lock_irqsave(&udc->vbus_events_lock, flags);
+ }
+ spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
}
/**
@@ -1141,14 +1104,26 @@ static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc
*/
void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
{
- struct usb_udc *udc = gadget->udc;
+ struct usb_udc *udc;
+ struct vbus_event *vbus_event;
+ unsigned long flags;
- mutex_lock(&udc->connect_lock);
- if (udc) {
- udc->vbus = status;
- usb_udc_connect_control_locked(udc);
+ //mutex_lock(&udc_lock);
+ if (!gadget || !gadget->udc) {
+ //mutex_unlock(&udc_lock);
+ return;
}
- mutex_unlock(&udc->connect_lock);
+ udc = gadget->udc;
+
+ vbus_event = kzalloc(sizeof(*vbus_event), GFP_ATOMIC);
+ if (!vbus_event)
+ return;
+
+ spin_lock_irqsave(&udc->vbus_events_lock, flags);
+ vbus_event->vbus_on = status;
+ list_add_tail(&vbus_event->node, &udc->vbus_events);
+ spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
+ schedule_work(&udc->vbus_work);
}
EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
@@ -1170,7 +1145,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget,
EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
/**
- * usb_gadget_udc_start_locked - tells usb device controller to start up
+ * usb_gadget_udc_start - 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
@@ -1181,11 +1156,8 @@ 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_locked(struct usb_udc *udc)
- __must_hold(&udc->connect_lock)
+static inline int usb_gadget_udc_start(struct usb_udc *udc)
{
int ret;
@@ -1194,15 +1166,17 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
return -EBUSY;
}
+ mutex_lock(&udc->connect_lock);
ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
if (!ret)
udc->started = true;
+ mutex_unlock(&udc->connect_lock);
return ret;
}
/**
- * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore
+ * usb_gadget_udc_stop - 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
@@ -1211,19 +1185,18 @@ static inline int usb_gadget_udc_start_locked(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_locked(struct usb_udc *udc)
- __must_hold(&udc->connect_lock)
+static inline void usb_gadget_udc_stop(struct usb_udc *udc)
{
if (!udc->started) {
dev_err(&udc->dev, "UDC had already stopped\n");
return;
}
+ mutex_lock(&udc->connect_lock);
udc->gadget->ops->udc_stop(udc->gadget);
udc->started = false;
+ mutex_unlock(&udc->connect_lock);
}
/**
@@ -1362,6 +1335,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
if (!udc)
goto error;
+ udc->unbinding = true;
device_initialize(&udc->dev);
udc->dev.release = usb_udc_release;
udc->dev.class = udc_class;
@@ -1375,6 +1349,9 @@ int usb_add_gadget(struct usb_gadget *gadget)
udc->gadget = gadget;
gadget->udc = udc;
mutex_init(&udc->connect_lock);
+ INIT_LIST_HEAD(&udc->vbus_events);
+ spin_lock_init(&udc->vbus_events_lock);
+ INIT_WORK(&udc->vbus_work, vbus_event_work);
udc->started = false;
@@ -1474,6 +1451,17 @@ char *usb_get_gadget_udc_name(void)
}
EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name);
+static int usb_gadget_set_unbinding(struct usb_udc *udc, bool status)
+{
+ int ret;
+
+ mutex_lock(&udc->connect_lock);
+ udc->unbinding = status;
+ ret = usb_gadget_pullup_update_locked(udc->gadget);
+ mutex_unlock(&udc->connect_lock);
+
+ return ret;
+}
/**
* usb_add_gadget_udc - adds a new gadget to the udc class driver list
* @parent: the parent device to this udc. Usually the controller
@@ -1576,15 +1564,11 @@ static int gadget_bind_driver(struct device *dev)
if (ret)
goto err_bind;
- mutex_lock(&udc->connect_lock);
- ret = usb_gadget_udc_start_locked(udc);
- if (ret) {
- mutex_unlock(&udc->connect_lock);
+ ret = usb_gadget_udc_start(udc);
+ if (ret)
goto err_start;
- }
usb_gadget_enable_async_callbacks(udc);
- usb_udc_connect_control_locked(udc);
- mutex_unlock(&udc->connect_lock);
+ usb_gadget_set_unbinding(udc, false);
kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
return 0;
@@ -1615,14 +1599,13 @@ static void gadget_unbind_driver(struct device *dev)
kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
- mutex_lock(&udc->connect_lock);
- usb_gadget_disconnect_locked(gadget);
+ usb_gadget_set_unbinding(udc, true);
+ cancel_work_sync(&udc->vbus_work);
usb_gadget_disable_async_callbacks(udc);
if (gadget->irq)
synchronize_irq(gadget->irq);
udc->driver->unbind(gadget);
- usb_gadget_udc_stop_locked(udc);
- mutex_unlock(&udc->connect_lock);
+ usb_gadget_udc_stop(udc);
mutex_lock(&udc_lock);
driver->is_bound = false;
@@ -1708,15 +1691,11 @@ static ssize_t soft_connect_store(struct device *dev,
}
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);
+ usb_gadget_udc_start(udc);
+ usb_udc_vbus_handler(udc->gadget, true);
} 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);
+ usb_udc_vbus_handler(udc->gadget, false);
+ usb_gadget_udc_stop(udc);
} else {
dev_err(dev, "unsupported command '%s'\n", buf);
ret = -EINVAL;
--
2.41.0.rc0.172.g3f132b7071-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 3/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing
2023-05-29 23:37 ` [PATCH v3 3/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing Badhri Jagan Sridharan
@ 2023-05-29 23:39 ` Badhri Jagan Sridharan
0 siblings, 0 replies; 4+ messages in thread
From: Badhri Jagan Sridharan @ 2023-05-29 23:39 UTC (permalink / raw)
To: gregkh, stern, colin.i.king, xuetao09, quic_eserrao,
water.zhangjiantao, peter.chen, balbi, francesco, alistair,
stephan, bagasdotme, luca
Cc: linux-usb, linux-kernel, stable, Francesco Dolcini
Please disregard v3 as I missed a minor cleanup. Sending v4.
Thanks,
Badhri
On Mon, May 29, 2023 at 4:37 PM Badhri Jagan Sridharan
<badhri@google.com> wrote:
>
> chipidea udc calls usb_udc_vbus_handler from udc_start gadget
> ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler
> processing.
>
> ============================================
> WARNING: possible recursive locking detected
> 640-rc1-000-devel-00005-gcda3c69ebc14 #1 Not tainted
> -------------------------------------------
>
> CPU0
> ----
> lock(&udc->connect_lock);
> lock(&udc->connect_lock);
>
> DEADLOCK
>
> stack backtrace:
> CPU: 1 PID: 566 Comm: echo Not tainted 640-rc1-000-devel-00005-gcda3c69ebc14 #1
> Hardware name: Freescale iMX7 Dual (Device Tree)
> unwind_backtrace from show_stack+0x10/0x14
> show_stack from dump_stack_lvl+0x70/0xb0
> dump_stack_lvl from __lock_acquire+0x924/0x22c4
> __lock_acquire from lock_acquire+0x100/0x370
> lock_acquire from __mutex_lock+0xa8/0xfb4
> __mutex_lock from mutex_lock_nested+0x1c/0x24
> mutex_lock_nested from usb_udc_vbus_handler+0x1c/0x60
> usb_udc_vbus_handler from ci_udc_start+0x74/0x9c
> ci_udc_start from gadget_bind_driver+0x130/0x230
> gadget_bind_driver from really_probe+0xd8/0x3fc
> really_probe from __driver_probe_device+0x94/0x1f0
> __driver_probe_device from driver_probe_device+0x2c/0xc4
> driver_probe_device from __driver_attach+0x114/0x1cc
> __driver_attach from bus_for_each_dev+0x7c/0xcc
> bus_for_each_dev from bus_add_driver+0xd4/0x200
> bus_add_driver from driver_register+0x7c/0x114
> driver_register from usb_gadget_register_driver_owner+0x40/0xe0
> usb_gadget_register_driver_owner from gadget_dev_desc_UDC_store+0xd4/0x110
> gadget_dev_desc_UDC_store from configfs_write_iter+0xac/0x118
> configfs_write_iter from vfs_write+0x1b4/0x40c
> vfs_write from ksys_write+0x70/0xf8
> ksys_write from ret_fast_syscall+0x0/0x1c
>
> Fixes: 0db213ea8eed ("Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup""")
> Cc: stable@vger.kernel.org
> Reported-by: Stephan Gerhold <stephan@gerhold.net>
> Closes: https://lore.kernel.org/all/ZF4bMptC3Lf2Hnee@gerhold.net/
> Reported-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Closes: https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/
> Reported-by: Alistair <alistair@alistair23.me>
> Closes: https://lore.kernel.org/lkml/0cf8c588b701d7cf25ffe1a9217b81716e6a5c51.camel@alistair23.me/
> 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.
> ---
> drivers/usb/gadget/udc/core.c | 269 ++++++++++++++++------------------
> 1 file changed, 124 insertions(+), 145 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 4641153e9706..2aa914a0a195 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -38,9 +38,10 @@ static const struct bus_type gadget_bus_type;
> * 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.
> + * functions. usb_gadget_pullup_update_locked called with this lock held.
> + * @vbus_events: list head for processing vbus updates on usb_udc_vbus_handler.
> + * @vbus_events_lock: protects vbus_events list
> + * @vbus_work: work item that invokes usb_gadget_pullup_update_locked.
> *
> * This represents the internal data structure which is used by the UDC-class
> * to hold information about udc driver and gadget together.
> @@ -53,6 +54,20 @@ struct usb_udc {
> bool vbus;
> bool started;
> struct mutex connect_lock;
> + struct list_head vbus_events;
> + spinlock_t vbus_events_lock;
> + struct work_struct vbus_work;
> + bool unbinding;
> +};
> +
> +/**
> + * struct vbus_event - used to notify vbus updates posted through usb_udc_vbus_handler.
> + * @vbus_on: true when vbus is on. false other wise.
> + * @node: list node for maintaining a list of pending updates to be processed.
> + */
> +struct vbus_event {
> + bool vbus_on;
> + struct list_head node;
> };
>
> static struct class *udc_class;
> @@ -693,40 +708,46 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
> EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
>
> /* Internal version of usb_gadget_connect needs to be called with connect_lock held. */
> -static int usb_gadget_connect_locked(struct usb_gadget *gadget)
> +static int usb_gadget_pullup_update_locked(struct usb_gadget *gadget)
> __must_hold(&gadget->udc->connect_lock)
> {
> int ret = 0;
> + bool connect = !gadget->deactivated && gadget->udc->started && gadget->udc->vbus &&
> + !gadget->udc->unbinding;
>
> if (!gadget->ops->pullup) {
> ret = -EOPNOTSUPP;
> goto out;
> }
>
> - if (gadget->connected)
> - goto out;
> -
> - 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;
> + if (connect != gadget->connected) {
> + ret = gadget->ops->pullup(gadget, connect);
> + if (!ret)
> + gadget->connected = connect;
> + mutex_lock(&udc_lock);
> + if (!connect)
> + gadget->udc->driver->disconnect(gadget);
> + mutex_unlock(&udc_lock);
> }
>
> - ret = gadget->ops->pullup(gadget, 1);
> - if (!ret)
> - gadget->connected = 1;
> -
> out:
> trace_usb_gadget_connect(gadget, ret);
>
> return ret;
> }
>
> +static int usb_gadget_set_vbus(struct usb_gadget *gadget, bool vbus)
> +{
> + int ret;
> +
> + mutex_lock(&gadget->udc->connect_lock);
> + gadget->udc->vbus = vbus;
> + ret = usb_gadget_pullup_update_locked(gadget);
> + mutex_unlock(&gadget->udc->connect_lock);
> +
> + return ret;
> +}
> +
> /**
> * usb_gadget_connect - software-controlled connect to USB host
> * @gadget:the peripheral being connected
> @@ -739,56 +760,10 @@ static int usb_gadget_connect_locked(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;
> + return usb_gadget_set_vbus(gadget, true);
> }
> 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;
> -
> - if (!gadget->ops->pullup) {
> - ret = -EOPNOTSUPP;
> - goto out;
> - }
> -
> - 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.
> - *
> - * udc should have been started before gadget being pulled down.
> - */
> - gadget->connected = false;
> - goto out;
> - }
> -
> - ret = gadget->ops->pullup(gadget, 0);
> - if (!ret)
> - gadget->connected = 0;
> -
> - mutex_lock(&udc_lock);
> - if (gadget->udc->driver)
> - gadget->udc->driver->disconnect(gadget);
> - mutex_unlock(&udc_lock);
> -
> -out:
> - trace_usb_gadget_disconnect(gadget, ret);
> -
> - return ret;
> -}
> -
> /**
> * usb_gadget_disconnect - software-controlled disconnect from USB host
> * @gadget:the peripheral being disconnected
> @@ -803,16 +778,22 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
> * Returns zero on success, else negative errno.
> */
> int usb_gadget_disconnect(struct usb_gadget *gadget)
> +{
> + return usb_gadget_set_vbus(gadget, false);
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
> +
> +static int usb_gadget_set_deactivate(struct usb_gadget *gadget, bool deactivate)
> {
> int ret;
>
> mutex_lock(&gadget->udc->connect_lock);
> - ret = usb_gadget_disconnect_locked(gadget);
> + gadget->deactivated = deactivate;
> + ret = usb_gadget_pullup_update_locked(gadget);
> mutex_unlock(&gadget->udc->connect_lock);
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
>
> /**
> * usb_gadget_deactivate - deactivate function which is not ready to work
> @@ -829,26 +810,7 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
> {
> int ret = 0;
>
> - if (gadget->deactivated)
> - goto out;
> -
> - mutex_lock(&gadget->udc->connect_lock);
> - 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;
> -
> -unlock:
> - mutex_unlock(&gadget->udc->connect_lock);
> -out:
> + ret = usb_gadget_set_deactivate(gadget, true);
> trace_usb_gadget_deactivate(gadget, ret);
>
> return ret;
> @@ -868,21 +830,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
> {
> int ret = 0;
>
> - if (!gadget->deactivated)
> - goto out;
> -
> - mutex_lock(&gadget->udc->connect_lock);
> - 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)
> - ret = usb_gadget_connect_locked(gadget);
> - mutex_unlock(&gadget->udc->connect_lock);
> -
> -out:
> + ret = usb_gadget_set_deactivate(gadget, false);
> trace_usb_gadget_activate(gadget, ret);
>
> return ret;
> @@ -1121,13 +1069,28 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
>
> /* ------------------------------------------------------------------------- */
>
> -/* Acquire connect_lock before calling this function. */
> -static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
> +static void vbus_event_work(struct work_struct *work)
> {
> - if (udc->vbus && udc->started)
> - usb_gadget_connect_locked(udc->gadget);
> - else
> - usb_gadget_disconnect_locked(udc->gadget);
> + struct vbus_event *event, *n;
> + struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&udc->vbus_events_lock, flags);
> + list_for_each_entry_safe(event, n, &udc->vbus_events, node) {
> + list_del(&event->node);
> + /* OK to drop the lock here as it suffice to syncrhronize udc->vbus_events node
> + * retrieval and deletion against usb_udc_vbus_handler. usb_udc_vbus_handler does
> + * list_add_tail so n would be the same even if the lock is dropped.
> + */
> + spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
> + mutex_lock(&udc->connect_lock);
> + udc->vbus = event->vbus_on;
> + usb_gadget_pullup_update_locked(udc->gadget);
> + kfree(event);
> + mutex_unlock(&udc->connect_lock);
> + spin_lock_irqsave(&udc->vbus_events_lock, flags);
> + }
> + spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
> }
>
> /**
> @@ -1141,14 +1104,26 @@ static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc
> */
> void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status)
> {
> - struct usb_udc *udc = gadget->udc;
> + struct usb_udc *udc;
> + struct vbus_event *vbus_event;
> + unsigned long flags;
>
> - mutex_lock(&udc->connect_lock);
> - if (udc) {
> - udc->vbus = status;
> - usb_udc_connect_control_locked(udc);
> + //mutex_lock(&udc_lock);
> + if (!gadget || !gadget->udc) {
> + //mutex_unlock(&udc_lock);
> + return;
> }
> - mutex_unlock(&udc->connect_lock);
> + udc = gadget->udc;
> +
> + vbus_event = kzalloc(sizeof(*vbus_event), GFP_ATOMIC);
> + if (!vbus_event)
> + return;
> +
> + spin_lock_irqsave(&udc->vbus_events_lock, flags);
> + vbus_event->vbus_on = status;
> + list_add_tail(&vbus_event->node, &udc->vbus_events);
> + spin_unlock_irqrestore(&udc->vbus_events_lock, flags);
> + schedule_work(&udc->vbus_work);
> }
> EXPORT_SYMBOL_GPL(usb_udc_vbus_handler);
>
> @@ -1170,7 +1145,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget,
> EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
>
> /**
> - * usb_gadget_udc_start_locked - tells usb device controller to start up
> + * usb_gadget_udc_start - 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
> @@ -1181,11 +1156,8 @@ 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_locked(struct usb_udc *udc)
> - __must_hold(&udc->connect_lock)
> +static inline int usb_gadget_udc_start(struct usb_udc *udc)
> {
> int ret;
>
> @@ -1194,15 +1166,17 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
> return -EBUSY;
> }
>
> + mutex_lock(&udc->connect_lock);
> ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
> if (!ret)
> udc->started = true;
> + mutex_unlock(&udc->connect_lock);
>
> return ret;
> }
>
> /**
> - * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore
> + * usb_gadget_udc_stop - 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
> @@ -1211,19 +1185,18 @@ static inline int usb_gadget_udc_start_locked(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_locked(struct usb_udc *udc)
> - __must_hold(&udc->connect_lock)
> +static inline void usb_gadget_udc_stop(struct usb_udc *udc)
> {
> if (!udc->started) {
> dev_err(&udc->dev, "UDC had already stopped\n");
> return;
> }
>
> + mutex_lock(&udc->connect_lock);
> udc->gadget->ops->udc_stop(udc->gadget);
> udc->started = false;
> + mutex_unlock(&udc->connect_lock);
> }
>
> /**
> @@ -1362,6 +1335,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
> if (!udc)
> goto error;
>
> + udc->unbinding = true;
> device_initialize(&udc->dev);
> udc->dev.release = usb_udc_release;
> udc->dev.class = udc_class;
> @@ -1375,6 +1349,9 @@ int usb_add_gadget(struct usb_gadget *gadget)
> udc->gadget = gadget;
> gadget->udc = udc;
> mutex_init(&udc->connect_lock);
> + INIT_LIST_HEAD(&udc->vbus_events);
> + spin_lock_init(&udc->vbus_events_lock);
> + INIT_WORK(&udc->vbus_work, vbus_event_work);
>
> udc->started = false;
>
> @@ -1474,6 +1451,17 @@ char *usb_get_gadget_udc_name(void)
> }
> EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name);
>
> +static int usb_gadget_set_unbinding(struct usb_udc *udc, bool status)
> +{
> + int ret;
> +
> + mutex_lock(&udc->connect_lock);
> + udc->unbinding = status;
> + ret = usb_gadget_pullup_update_locked(udc->gadget);
> + mutex_unlock(&udc->connect_lock);
> +
> + return ret;
> +}
> /**
> * usb_add_gadget_udc - adds a new gadget to the udc class driver list
> * @parent: the parent device to this udc. Usually the controller
> @@ -1576,15 +1564,11 @@ static int gadget_bind_driver(struct device *dev)
> if (ret)
> goto err_bind;
>
> - mutex_lock(&udc->connect_lock);
> - ret = usb_gadget_udc_start_locked(udc);
> - if (ret) {
> - mutex_unlock(&udc->connect_lock);
> + ret = usb_gadget_udc_start(udc);
> + if (ret)
> goto err_start;
> - }
> usb_gadget_enable_async_callbacks(udc);
> - usb_udc_connect_control_locked(udc);
> - mutex_unlock(&udc->connect_lock);
> + usb_gadget_set_unbinding(udc, false);
>
> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> return 0;
> @@ -1615,14 +1599,13 @@ static void gadget_unbind_driver(struct device *dev)
>
> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>
> - mutex_lock(&udc->connect_lock);
> - usb_gadget_disconnect_locked(gadget);
> + usb_gadget_set_unbinding(udc, true);
> + cancel_work_sync(&udc->vbus_work);
> usb_gadget_disable_async_callbacks(udc);
> if (gadget->irq)
> synchronize_irq(gadget->irq);
> udc->driver->unbind(gadget);
> - usb_gadget_udc_stop_locked(udc);
> - mutex_unlock(&udc->connect_lock);
> + usb_gadget_udc_stop(udc);
>
> mutex_lock(&udc_lock);
> driver->is_bound = false;
> @@ -1708,15 +1691,11 @@ static ssize_t soft_connect_store(struct device *dev,
> }
>
> 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);
> + usb_gadget_udc_start(udc);
> + usb_udc_vbus_handler(udc->gadget, true);
> } 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);
> + usb_udc_vbus_handler(udc->gadget, false);
> + usb_gadget_udc_stop(udc);
> } else {
> dev_err(dev, "unsupported command '%s'\n", buf);
> ret = -EINVAL;
> --
> 2.41.0.rc0.172.g3f132b7071-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-29 23:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-29 23:36 [PATCH v3 1/3] Revert "Revert "usb: gadget: udc: core: Invoke usb_gadget_connect only when started"" Badhri Jagan Sridharan
2023-05-29 23:36 ` [PATCH v3 2/3] Revert "Revert "usb: gadget: udc: core: Prevent redundant calls to pullup"" Badhri Jagan Sridharan
2023-05-29 23:37 ` [PATCH v3 3/3] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing Badhri Jagan Sridharan
2023-05-29 23:39 ` Badhri Jagan Sridharan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).