* [PATCH 0/2] CDC-ACM: fix circular dependency
@ 2021-03-11 12:59 Oliver Neukum
2021-03-11 12:59 ` [PATCH 1/2] cdc-acm: untangle a circular dependency between callback and softint Oliver Neukum
2021-03-11 13:00 ` [PATCH 2/2] CDC-ACM: downgrade message to debug Oliver Neukum
0 siblings, 2 replies; 4+ messages in thread
From: Oliver Neukum @ 2021-03-11 12:59 UTC (permalink / raw)
To: bruno.thomsen, gregKHusb, linux-usb
The driver has an issue with an interaction between queued works
and interrupt handlers that needs to be solved.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] cdc-acm: untangle a circular dependency between callback and softint
2021-03-11 12:59 [PATCH 0/2] CDC-ACM: fix circular dependency Oliver Neukum
@ 2021-03-11 12:59 ` Oliver Neukum
2021-03-11 13:00 ` [PATCH 2/2] CDC-ACM: downgrade message to debug Oliver Neukum
1 sibling, 0 replies; 4+ messages in thread
From: Oliver Neukum @ 2021-03-11 12:59 UTC (permalink / raw)
To: bruno.thomsen, gregKHusb, linux-usb; +Cc: Oliver Neukum
We have a cycle of callbacks scheduling works which submit
URBs with thos callbacks. This needs to be blocked, stopped
and unblocked to untangle the circle.
The issue leads to faults like:
[ 55.068392] Unable to handle kernel paging request at virtual address 6b6b6c03
[ 55.075624] pgd = be866494
[ 55.078335] [6b6b6c03] *pgd=00000000
[ 55.081924] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[ 55.087238] Modules linked in: ppp_async crc_ccitt ppp_generic slhc
xt_TCPMSS xt_tcpmss xt_hl nf_log_ipv6 nf_log_ipv4 nf_log_common
xt_policy xt_limit xt_conntrack xt_tcpudp xt_pkttype ip6table_mangle
iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4
iptable_mangle ip6table_filter ip6_tables iptable_filter ip_tables
des_generic md5 sch_fq_codel cdc_mbim cdc_wdm cdc_ncm usbnet mii
cdc_acm usb_storage ip_tunnel xfrm_user xfrm6_tunnel tunnel6
xfrm4_tunnel tunnel4 esp6 esp4 ah6 ah4 xfrm_algo xt_LOG xt_LED
xt_comment x_tables ipv6
[ 55.134954] CPU: 0 PID: 82 Comm: kworker/0:2 Tainted: G
T 5.8.17 #1
[ 55.142526] Hardware name: Freescale i.MX7 Dual (Device Tree)
[ 55.148304] Workqueue: events acm_softint [cdc_acm]
[ 55.153196] PC is at kobject_get+0x10/0xa4
[ 55.157302] LR is at usb_get_dev+0x14/0x1c
[ 55.161402] pc : [<8047c06c>] lr : [<80560448>] psr: 20000193
[ 55.167671] sp : bca39ea8 ip : 00007374 fp : bf6cbd80
[ 55.172899] r10: 00000000 r9 : bdd92284 r8 : bdd92008
[ 55.178128] r7 : 6b6b6b6b r6 : fffffffe r5 : 60000113 r4 : 6b6b6be3
[ 55.184658] r3 : 6b6b6b6b r2 : 00000111 r1 : 00000000 r0 : 6b6b6be3
[ 55.191191] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
[ 55.198417] Control: 10c5387d Table: bcf0c06a DAC: 00000051
[ 55.204168] Process kworker/0:2 (pid: 82, stack limit = 0x9bdd2a89)
[ 55.210439] Stack: (0xbca39ea8 to 0xbca3a000)
[ 55.214805] 9ea0: bf6cbd80 80769a50 6b6b6b6b 80560448 bdeb0500 8056bfe8
[ 55.222991] 9ec0: 00000002 b76da000 00000000 bdeb0500 bdd92448 bca38000 bdeb0510 8056d69c
[ 55.231177] 9ee0: bca38000 00000000 80c050fc 00000000 bca39f44 09d42015 00000000 00000001
[ 55.239363] 9f00: bdd92448 bdd92438 bdd92000 7f1158c4 bdd92448 bca2ee00 bf6cbd80 bf6cef00
[ 55.247549] 9f20: 00000000 00000000 00000000 801412d8 bf6cbd98 80c03d00 bca2ee00 bf6cbd80
[ 55.255735] 9f40: bca2ee14 bf6cbd98 80c03d00 00000008 bca38000 80141568 00000000 80c446ae
[ 55.263921] 9f60: 00000000 bc9ed880 bc9f0700 bca38000 bc117eb4 80141524 bca2ee00 bc9ed8a4
[ 55.272107] 9f80: 00000000 80147cc8 00000000 bc9f0700 80147b84 00000000 00000000 00000000
[ 55.280292] 9fa0: 00000000 00000000 00000000 80100148 00000000 00000000 00000000 00000000
[ 55.288477] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 55.296662] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[ 55.304860] [<8047c06c>] (kobject_get) from [<80560448>] (usb_get_dev+0x14/0x1c)
[ 55.312271] [<80560448>] (usb_get_dev) from [<8056bfe8>] (usb_hcd_unlink_urb+0x50/0xd8)
[ 55.320286] [<8056bfe8>] (usb_hcd_unlink_urb) from [<8056d69c>] (usb_kill_urb.part.0+0x44/0xd0)
[ 55.329004] [<8056d69c>] (usb_kill_urb.part.0) from [<7f1158c4>] (acm_softint+0x4c/0x10c [cdc_acm])
[ 55.338082] [<7f1158c4>] (acm_softint [cdc_acm]) from [<801412d8>] (process_one_work+0x19c/0x3e8)
[ 55.346969] [<801412d8>] (process_one_work) from [<80141568>] (worker_thread+0x44/0x4dc)
[ 55.355072] [<80141568>] (worker_thread) from [<80147cc8>] (kthread+0x144/0x180)
[ 55.362481] [<80147cc8>] (kthread) from [<80100148>] (ret_from_fork+0x14/0x2c)
[ 55.369706] Exception stack(0xbca39fb0 to 0xbca39ff8)
Tested-by: Bruno Thomsen <bruno.thomsen@gmail.com>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/class/cdc-acm.c | 48 ++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index b013671261a2..d684cf94b1c0 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -147,17 +147,29 @@ static inline int acm_set_control(struct acm *acm, int control)
#define acm_send_break(acm, ms) \
acm_ctrl_msg(acm, USB_CDC_REQ_SEND_BREAK, ms, NULL, 0)
-static void acm_kill_urbs(struct acm *acm)
+static void acm_poison_urbs(struct acm *acm)
{
int i;
- usb_kill_urb(acm->ctrlurb);
+ usb_poison_urb(acm->ctrlurb);
for (i = 0; i < ACM_NW; i++)
- usb_kill_urb(acm->wb[i].urb);
+ usb_poison_urb(acm->wb[i].urb);
for (i = 0; i < acm->rx_buflimit; i++)
- usb_kill_urb(acm->read_urbs[i]);
+ usb_poison_urb(acm->read_urbs[i]);
+}
+
+static void acm_unpoison_urbs(struct acm *acm)
+{
+ int i;
+
+ for (i = 0; i < acm->rx_buflimit; i++)
+ usb_unpoison_urb(acm->read_urbs[i]);
+ for (i = 0; i < ACM_NW; i++)
+ usb_unpoison_urb(acm->wb[i].urb);
+ usb_unpoison_urb(acm->ctrlurb);
}
+
/*
* Write buffer management.
* All of these assume proper locks taken by the caller.
@@ -226,9 +238,10 @@ static int acm_start_wb(struct acm *acm, struct acm_wb *wb)
rc = usb_submit_urb(wb->urb, GFP_ATOMIC);
if (rc < 0) {
- dev_err(&acm->data->dev,
- "%s - usb_submit_urb(write bulk) failed: %d\n",
- __func__, rc);
+ if (rc != -EPERM)
+ dev_err(&acm->data->dev,
+ "%s - usb_submit_urb(write bulk) failed: %d\n",
+ __func__, rc);
acm_write_done(acm, wb);
}
return rc;
@@ -482,11 +495,6 @@ static void acm_read_bulk_callback(struct urb *urb)
dev_vdbg(&acm->data->dev, "got urb %d, len %d, status %d\n",
rb->index, urb->actual_length, status);
- if (!acm->dev) {
- dev_dbg(&acm->data->dev, "%s - disconnected\n", __func__);
- return;
- }
-
switch (status) {
case 0:
usb_mark_last_busy(acm->dev);
@@ -733,6 +741,7 @@ static void acm_port_shutdown(struct tty_port *port)
* Need to grab write_lock to prevent race with resume, but no need to
* hold it due to the tty-port initialised flag.
*/
+ acm_poison_urbs(acm);
spin_lock_irq(&acm->write_lock);
spin_unlock_irq(&acm->write_lock);
@@ -749,7 +758,8 @@ static void acm_port_shutdown(struct tty_port *port)
usb_autopm_put_interface_async(acm->control);
}
- acm_kill_urbs(acm);
+ acm_unpoison_urbs(acm);
+
}
static void acm_tty_cleanup(struct tty_struct *tty)
@@ -1542,8 +1552,14 @@ static void acm_disconnect(struct usb_interface *intf)
if (!acm)
return;
- mutex_lock(&acm->mutex);
acm->disconnected = true;
+ /*
+ * there is a circular dependency. acm_softint() can resubmit
+ * the URBs in error handling so we need to block any
+ * submission right away
+ */
+ acm_poison_urbs(acm);
+ mutex_lock(&acm->mutex);
if (acm->country_codes) {
device_remove_file(&acm->control->dev,
&dev_attr_wCountryCodes);
@@ -1562,7 +1578,6 @@ static void acm_disconnect(struct usb_interface *intf)
tty_kref_put(tty);
}
- acm_kill_urbs(acm);
cancel_delayed_work_sync(&acm->dwork);
tty_unregister_device(acm_tty_driver, acm->minor);
@@ -1604,7 +1619,7 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
if (cnt)
return 0;
- acm_kill_urbs(acm);
+ acm_poison_urbs(acm);
cancel_delayed_work_sync(&acm->dwork);
acm->urbs_in_error_delay = 0;
@@ -1617,6 +1632,7 @@ static int acm_resume(struct usb_interface *intf)
struct urb *urb;
int rv = 0;
+ acm_unpoison_urbs(acm);
spin_lock_irq(&acm->write_lock);
if (--acm->susp_count)
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] CDC-ACM: downgrade message to debug
2021-03-11 12:59 [PATCH 0/2] CDC-ACM: fix circular dependency Oliver Neukum
2021-03-11 12:59 ` [PATCH 1/2] cdc-acm: untangle a circular dependency between callback and softint Oliver Neukum
@ 2021-03-11 13:00 ` Oliver Neukum
1 sibling, 0 replies; 4+ messages in thread
From: Oliver Neukum @ 2021-03-11 13:00 UTC (permalink / raw)
To: bruno.thomsen, gregKHusb, linux-usb; +Cc: Oliver Neukum
This failure is so common that logging an error here amounts
to spamming log files.
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/class/cdc-acm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index d684cf94b1c0..fd2fce072985 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -659,7 +659,8 @@ static void acm_port_dtr_rts(struct tty_port *port, int raise)
res = acm_set_control(acm, val);
if (res && (acm->ctrl_caps & USB_CDC_CAP_LINE))
- dev_err(&acm->control->dev, "failed to set dtr/rts\n");
+ /* This is broken in too many devices to spam the logs */
+ dev_dbg(&acm->control->dev, "failed to set dtr/rts\n");
}
static int acm_port_activate(struct tty_port *port, struct tty_struct *tty)
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 1/2] cdc-acm: untangle a circular dependency between callback and softint
@ 2021-03-11 13:01 Oliver Neukum
0 siblings, 0 replies; 4+ messages in thread
From: Oliver Neukum @ 2021-03-11 13:01 UTC (permalink / raw)
To: bruno.thomsen, gregKH, linux-usb; +Cc: Oliver Neukum
We have a cycle of callbacks scheduling works which submit
URBs with thos callbacks. This needs to be blocked, stopped
and unblocked to untangle the circle.
The issue leads to faults like:
[ 55.068392] Unable to handle kernel paging request at virtual address 6b6b6c03
[ 55.075624] pgd = be866494
[ 55.078335] [6b6b6c03] *pgd=00000000
[ 55.081924] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[ 55.087238] Modules linked in: ppp_async crc_ccitt ppp_generic slhc
xt_TCPMSS xt_tcpmss xt_hl nf_log_ipv6 nf_log_ipv4 nf_log_common
xt_policy xt_limit xt_conntrack xt_tcpudp xt_pkttype ip6table_mangle
iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4
iptable_mangle ip6table_filter ip6_tables iptable_filter ip_tables
des_generic md5 sch_fq_codel cdc_mbim cdc_wdm cdc_ncm usbnet mii
cdc_acm usb_storage ip_tunnel xfrm_user xfrm6_tunnel tunnel6
xfrm4_tunnel tunnel4 esp6 esp4 ah6 ah4 xfrm_algo xt_LOG xt_LED
xt_comment x_tables ipv6
[ 55.134954] CPU: 0 PID: 82 Comm: kworker/0:2 Tainted: G
T 5.8.17 #1
[ 55.142526] Hardware name: Freescale i.MX7 Dual (Device Tree)
[ 55.148304] Workqueue: events acm_softint [cdc_acm]
[ 55.153196] PC is at kobject_get+0x10/0xa4
[ 55.157302] LR is at usb_get_dev+0x14/0x1c
[ 55.161402] pc : [<8047c06c>] lr : [<80560448>] psr: 20000193
[ 55.167671] sp : bca39ea8 ip : 00007374 fp : bf6cbd80
[ 55.172899] r10: 00000000 r9 : bdd92284 r8 : bdd92008
[ 55.178128] r7 : 6b6b6b6b r6 : fffffffe r5 : 60000113 r4 : 6b6b6be3
[ 55.184658] r3 : 6b6b6b6b r2 : 00000111 r1 : 00000000 r0 : 6b6b6be3
[ 55.191191] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
[ 55.198417] Control: 10c5387d Table: bcf0c06a DAC: 00000051
[ 55.204168] Process kworker/0:2 (pid: 82, stack limit = 0x9bdd2a89)
[ 55.210439] Stack: (0xbca39ea8 to 0xbca3a000)
[ 55.214805] 9ea0: bf6cbd80 80769a50 6b6b6b6b 80560448 bdeb0500 8056bfe8
[ 55.222991] 9ec0: 00000002 b76da000 00000000 bdeb0500 bdd92448 bca38000 bdeb0510 8056d69c
[ 55.231177] 9ee0: bca38000 00000000 80c050fc 00000000 bca39f44 09d42015 00000000 00000001
[ 55.239363] 9f00: bdd92448 bdd92438 bdd92000 7f1158c4 bdd92448 bca2ee00 bf6cbd80 bf6cef00
[ 55.247549] 9f20: 00000000 00000000 00000000 801412d8 bf6cbd98 80c03d00 bca2ee00 bf6cbd80
[ 55.255735] 9f40: bca2ee14 bf6cbd98 80c03d00 00000008 bca38000 80141568 00000000 80c446ae
[ 55.263921] 9f60: 00000000 bc9ed880 bc9f0700 bca38000 bc117eb4 80141524 bca2ee00 bc9ed8a4
[ 55.272107] 9f80: 00000000 80147cc8 00000000 bc9f0700 80147b84 00000000 00000000 00000000
[ 55.280292] 9fa0: 00000000 00000000 00000000 80100148 00000000 00000000 00000000 00000000
[ 55.288477] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 55.296662] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[ 55.304860] [<8047c06c>] (kobject_get) from [<80560448>] (usb_get_dev+0x14/0x1c)
[ 55.312271] [<80560448>] (usb_get_dev) from [<8056bfe8>] (usb_hcd_unlink_urb+0x50/0xd8)
[ 55.320286] [<8056bfe8>] (usb_hcd_unlink_urb) from [<8056d69c>] (usb_kill_urb.part.0+0x44/0xd0)
[ 55.329004] [<8056d69c>] (usb_kill_urb.part.0) from [<7f1158c4>] (acm_softint+0x4c/0x10c [cdc_acm])
[ 55.338082] [<7f1158c4>] (acm_softint [cdc_acm]) from [<801412d8>] (process_one_work+0x19c/0x3e8)
[ 55.346969] [<801412d8>] (process_one_work) from [<80141568>] (worker_thread+0x44/0x4dc)
[ 55.355072] [<80141568>] (worker_thread) from [<80147cc8>] (kthread+0x144/0x180)
[ 55.362481] [<80147cc8>] (kthread) from [<80100148>] (ret_from_fork+0x14/0x2c)
[ 55.369706] Exception stack(0xbca39fb0 to 0xbca39ff8)
Tested-by: Bruno Thomsen <bruno.thomsen@gmail.com>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/class/cdc-acm.c | 48 ++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index b013671261a2..d684cf94b1c0 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -147,17 +147,29 @@ static inline int acm_set_control(struct acm *acm, int control)
#define acm_send_break(acm, ms) \
acm_ctrl_msg(acm, USB_CDC_REQ_SEND_BREAK, ms, NULL, 0)
-static void acm_kill_urbs(struct acm *acm)
+static void acm_poison_urbs(struct acm *acm)
{
int i;
- usb_kill_urb(acm->ctrlurb);
+ usb_poison_urb(acm->ctrlurb);
for (i = 0; i < ACM_NW; i++)
- usb_kill_urb(acm->wb[i].urb);
+ usb_poison_urb(acm->wb[i].urb);
for (i = 0; i < acm->rx_buflimit; i++)
- usb_kill_urb(acm->read_urbs[i]);
+ usb_poison_urb(acm->read_urbs[i]);
+}
+
+static void acm_unpoison_urbs(struct acm *acm)
+{
+ int i;
+
+ for (i = 0; i < acm->rx_buflimit; i++)
+ usb_unpoison_urb(acm->read_urbs[i]);
+ for (i = 0; i < ACM_NW; i++)
+ usb_unpoison_urb(acm->wb[i].urb);
+ usb_unpoison_urb(acm->ctrlurb);
}
+
/*
* Write buffer management.
* All of these assume proper locks taken by the caller.
@@ -226,9 +238,10 @@ static int acm_start_wb(struct acm *acm, struct acm_wb *wb)
rc = usb_submit_urb(wb->urb, GFP_ATOMIC);
if (rc < 0) {
- dev_err(&acm->data->dev,
- "%s - usb_submit_urb(write bulk) failed: %d\n",
- __func__, rc);
+ if (rc != -EPERM)
+ dev_err(&acm->data->dev,
+ "%s - usb_submit_urb(write bulk) failed: %d\n",
+ __func__, rc);
acm_write_done(acm, wb);
}
return rc;
@@ -482,11 +495,6 @@ static void acm_read_bulk_callback(struct urb *urb)
dev_vdbg(&acm->data->dev, "got urb %d, len %d, status %d\n",
rb->index, urb->actual_length, status);
- if (!acm->dev) {
- dev_dbg(&acm->data->dev, "%s - disconnected\n", __func__);
- return;
- }
-
switch (status) {
case 0:
usb_mark_last_busy(acm->dev);
@@ -733,6 +741,7 @@ static void acm_port_shutdown(struct tty_port *port)
* Need to grab write_lock to prevent race with resume, but no need to
* hold it due to the tty-port initialised flag.
*/
+ acm_poison_urbs(acm);
spin_lock_irq(&acm->write_lock);
spin_unlock_irq(&acm->write_lock);
@@ -749,7 +758,8 @@ static void acm_port_shutdown(struct tty_port *port)
usb_autopm_put_interface_async(acm->control);
}
- acm_kill_urbs(acm);
+ acm_unpoison_urbs(acm);
+
}
static void acm_tty_cleanup(struct tty_struct *tty)
@@ -1542,8 +1552,14 @@ static void acm_disconnect(struct usb_interface *intf)
if (!acm)
return;
- mutex_lock(&acm->mutex);
acm->disconnected = true;
+ /*
+ * there is a circular dependency. acm_softint() can resubmit
+ * the URBs in error handling so we need to block any
+ * submission right away
+ */
+ acm_poison_urbs(acm);
+ mutex_lock(&acm->mutex);
if (acm->country_codes) {
device_remove_file(&acm->control->dev,
&dev_attr_wCountryCodes);
@@ -1562,7 +1578,6 @@ static void acm_disconnect(struct usb_interface *intf)
tty_kref_put(tty);
}
- acm_kill_urbs(acm);
cancel_delayed_work_sync(&acm->dwork);
tty_unregister_device(acm_tty_driver, acm->minor);
@@ -1604,7 +1619,7 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
if (cnt)
return 0;
- acm_kill_urbs(acm);
+ acm_poison_urbs(acm);
cancel_delayed_work_sync(&acm->dwork);
acm->urbs_in_error_delay = 0;
@@ -1617,6 +1632,7 @@ static int acm_resume(struct usb_interface *intf)
struct urb *urb;
int rv = 0;
+ acm_unpoison_urbs(acm);
spin_lock_irq(&acm->write_lock);
if (--acm->susp_count)
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-11 13:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-11 12:59 [PATCH 0/2] CDC-ACM: fix circular dependency Oliver Neukum
2021-03-11 12:59 ` [PATCH 1/2] cdc-acm: untangle a circular dependency between callback and softint Oliver Neukum
2021-03-11 13:00 ` [PATCH 2/2] CDC-ACM: downgrade message to debug Oliver Neukum
2021-03-11 13:01 [PATCH 1/2] cdc-acm: untangle a circular dependency between callback and softint Oliver Neukum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.