Actually fix CDC ACM error recovery path (#712) (#921)

Instead of reverting the CDC ACM cool-down patch fix the intention of
that change. This should fix the error recovery paths in the CDC ACM
driver and allow CDC ACM devices to continue working even in the event
of USB issues.
This commit is contained in:
Stefan Agner 2020-10-21 20:34:26 +02:00 committed by GitHub
parent 5f0a8fe627
commit fdcb94f0d8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 124 additions and 132 deletions

View File

@ -1,132 +0,0 @@
From 5edf98e1fa176a480686ec77a5782b61eb009842 Mon Sep 17 00:00:00 2001
From: Jerome Brunet <jbrunet@baylibre.com>
Date: Thu, 15 Oct 2020 13:58:14 +0200
Subject: [PATCH] Revert "cdc-acm: introduce a cool down"
This reverts commit a4e7279cd1d19f48f0af2a10ed020febaa9ac092.
---
drivers/usb/class/cdc-acm.c | 30 ++----------------------------
drivers/usb/class/cdc-acm.h | 5 +----
2 files changed, 3 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index f1a9043bdfe5..0f47d74c857d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -410,12 +410,9 @@ static void acm_ctrl_irq(struct urb *urb)
exit:
retval = usb_submit_urb(urb, GFP_ATOMIC);
- if (retval && retval != -EPERM && retval != -ENODEV)
+ if (retval && retval != -EPERM)
dev_err(&acm->control->dev,
"%s - usb_submit_urb failed: %d\n", __func__, retval);
- else
- dev_vdbg(&acm->control->dev,
- "control resubmission terminated %d\n", retval);
}
static int acm_submit_read_urb(struct acm *acm, int index, gfp_t mem_flags)
@@ -431,8 +428,6 @@ static int acm_submit_read_urb(struct acm *acm, int index, gfp_t mem_flags)
dev_err(&acm->data->dev,
"urb %d failed submission with %d\n",
index, res);
- } else {
- dev_vdbg(&acm->data->dev, "intended failure %d\n", res);
}
set_bit(index, &acm->read_urbs_free);
return res;
@@ -474,7 +469,6 @@ static void acm_read_bulk_callback(struct urb *urb)
int status = urb->status;
bool stopped = false;
bool stalled = false;
- bool cooldown = false;
dev_vdbg(&acm->data->dev, "got urb %d, len %d, status %d\n",
rb->index, urb->actual_length, status);
@@ -501,14 +495,6 @@ static void acm_read_bulk_callback(struct urb *urb)
__func__, status);
stopped = true;
break;
- case -EOVERFLOW:
- case -EPROTO:
- dev_dbg(&acm->data->dev,
- "%s - cooling babbling device\n", __func__);
- usb_mark_last_busy(acm->dev);
- set_bit(rb->index, &acm->urbs_in_error_delay);
- cooldown = true;
- break;
default:
dev_dbg(&acm->data->dev,
"%s - nonzero urb status received: %d\n",
@@ -530,11 +516,9 @@ static void acm_read_bulk_callback(struct urb *urb)
*/
smp_mb__after_atomic();
- if (stopped || stalled || cooldown) {
+ if (stopped || stalled) {
if (stalled)
schedule_work(&acm->work);
- else if (cooldown)
- schedule_delayed_work(&acm->dwork, HZ / 2);
return;
}
@@ -581,12 +565,6 @@ static void acm_softint(struct work_struct *work)
}
}
- if (test_and_clear_bit(ACM_ERROR_DELAY, &acm->flags)) {
- for (i = 0; i < acm->rx_buflimit; i++)
- if (test_and_clear_bit(i, &acm->urbs_in_error_delay))
- acm_submit_read_urb(acm, i, GFP_NOIO);
- }
-
if (test_and_clear_bit(EVENT_TTY_WAKEUP, &acm->flags))
tty_port_tty_wakeup(&acm->port);
}
@@ -1353,7 +1331,6 @@ static int acm_probe(struct usb_interface *intf,
acm->readsize = readsize;
acm->rx_buflimit = num_rx_buf;
INIT_WORK(&acm->work, acm_softint);
- INIT_DELAYED_WORK(&acm->dwork, acm_softint);
init_waitqueue_head(&acm->wioctl);
spin_lock_init(&acm->write_lock);
spin_lock_init(&acm->read_lock);
@@ -1563,7 +1540,6 @@ static void acm_disconnect(struct usb_interface *intf)
acm_kill_urbs(acm);
cancel_work_sync(&acm->work);
- cancel_delayed_work_sync(&acm->dwork);
tty_unregister_device(acm_tty_driver, acm->minor);
@@ -1606,8 +1582,6 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
acm_kill_urbs(acm);
cancel_work_sync(&acm->work);
- cancel_delayed_work_sync(&acm->dwork);
- acm->urbs_in_error_delay = 0;
return 0;
}
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index cd5e9d8ab237..ca1c026382c2 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -109,11 +109,8 @@ struct acm {
# define EVENT_TTY_WAKEUP 0
# define EVENT_RX_STALL 1
# define ACM_THROTTLED 2
-# define ACM_ERROR_DELAY 3
- unsigned long urbs_in_error_delay; /* these need to be restarted after a delay */
struct usb_cdc_line_coding line; /* bits, stop, parity */
- struct work_struct work; /* work queue entry for various purposes*/
- struct delayed_work dwork; /* for cool downs needed in error recovery */
+ struct work_struct work; /* work queue entry for line discipline waking up */
unsigned int ctrlin; /* input control lines (DCD, DSR, RI, break, overruns) */
unsigned int ctrlout; /* output control lines (DTR, RTS) */
struct async_icount iocount; /* counters for control line changes */
--
2.25.4

View File

@ -0,0 +1,124 @@
From f6fce2e974fe091fd233301bd7c127ca18304039 Mon Sep 17 00:00:00 2001
Message-Id: <f6fce2e974fe091fd233301bd7c127ca18304039.1603303549.git.stefan@agner.ch>
From: Jerome Brunet <jbrunet@baylibre.com>
Date: Mon, 19 Oct 2020 19:07:02 +0200
Subject: [PATCH] usb: cdc-acm: fix cooldown mechanism
Commit a4e7279cd1d1 ("cdc-acm: introduce a cool down") is causing
regression if there is some USB error, such as -EPROTO.
This has been reported on some samples of the Odroid-N2 using the Combee II
Zibgee USB dongle.
> struct acm *acm = container_of(work, struct acm, work)
is incorrect in case of a delayed work and causes warnings, usually from
the workqueue:
> WARNING: CPU: 0 PID: 0 at kernel/workqueue.c:1474 __queue_work+0x480/0x528.
When this happens, USB eventually stops working completely after a while.
Also the ACM_ERROR_DELAY bit is never set, so the cooldown mechanism
previously introduced cannot be triggered and acm_submit_read_urb() is
never called.
This changes makes the cdc-acm driver use a single delayed work, fixing the
pointer arithmetic in acm_softint() and set the ACM_ERROR_DELAY when the
cooldown mechanism appear to be needed.
Fixes: a4e7279cd1d1 ("cdc-acm: introduce a cool down")
Reported-by: Pascal Vizeli <pascal.vizeli@nabucasa.com>
Cc: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
drivers/usb/class/cdc-acm.c | 12 +++++-------
drivers/usb/class/cdc-acm.h | 3 +--
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7f6f3ab5b8a6..8f087499196a 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -507,6 +507,7 @@ static void acm_read_bulk_callback(struct urb *urb)
"%s - cooling babbling device\n", __func__);
usb_mark_last_busy(acm->dev);
set_bit(rb->index, &acm->urbs_in_error_delay);
+ set_bit(ACM_ERROR_DELAY, &acm->flags);
cooldown = true;
break;
default:
@@ -532,7 +533,7 @@ static void acm_read_bulk_callback(struct urb *urb)
if (stopped || stalled || cooldown) {
if (stalled)
- schedule_work(&acm->work);
+ schedule_delayed_work(&acm->dwork, 0);
else if (cooldown)
schedule_delayed_work(&acm->dwork, HZ / 2);
return;
@@ -562,13 +563,13 @@ static void acm_write_bulk(struct urb *urb)
acm_write_done(acm, wb);
spin_unlock_irqrestore(&acm->write_lock, flags);
set_bit(EVENT_TTY_WAKEUP, &acm->flags);
- schedule_work(&acm->work);
+ schedule_delayed_work(&acm->dwork, 0);
}
static void acm_softint(struct work_struct *work)
{
int i;
- struct acm *acm = container_of(work, struct acm, work);
+ struct acm *acm = container_of(work, struct acm, dwork.work);
if (test_bit(EVENT_RX_STALL, &acm->flags)) {
smp_mb(); /* against acm_suspend() */
@@ -584,7 +585,7 @@ static void acm_softint(struct work_struct *work)
if (test_and_clear_bit(ACM_ERROR_DELAY, &acm->flags)) {
for (i = 0; i < acm->rx_buflimit; i++)
if (test_and_clear_bit(i, &acm->urbs_in_error_delay))
- acm_submit_read_urb(acm, i, GFP_NOIO);
+ acm_submit_read_urb(acm, i, GFP_KERNEL);
}
if (test_and_clear_bit(EVENT_TTY_WAKEUP, &acm->flags))
@@ -1352,7 +1353,6 @@ static int acm_probe(struct usb_interface *intf,
acm->ctrlsize = ctrlsize;
acm->readsize = readsize;
acm->rx_buflimit = num_rx_buf;
- INIT_WORK(&acm->work, acm_softint);
INIT_DELAYED_WORK(&acm->dwork, acm_softint);
init_waitqueue_head(&acm->wioctl);
spin_lock_init(&acm->write_lock);
@@ -1562,7 +1562,6 @@ static void acm_disconnect(struct usb_interface *intf)
}
acm_kill_urbs(acm);
- cancel_work_sync(&acm->work);
cancel_delayed_work_sync(&acm->dwork);
tty_unregister_device(acm_tty_driver, acm->minor);
@@ -1605,7 +1604,6 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
return 0;
acm_kill_urbs(acm);
- cancel_work_sync(&acm->work);
cancel_delayed_work_sync(&acm->dwork);
acm->urbs_in_error_delay = 0;
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index cd5e9d8ab237..b95ff769072e 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -112,8 +112,7 @@ struct acm {
# define ACM_ERROR_DELAY 3
unsigned long urbs_in_error_delay; /* these need to be restarted after a delay */
struct usb_cdc_line_coding line; /* bits, stop, parity */
- struct work_struct work; /* work queue entry for various purposes*/
- struct delayed_work dwork; /* for cool downs needed in error recovery */
+ struct delayed_work dwork; /* work queue entry for various purposes */
unsigned int ctrlin; /* input control lines (DCD, DSR, RI, break, overruns) */
unsigned int ctrlout; /* output control lines (DTR, RTS) */
struct async_icount iocount; /* counters for control line changes */
--
2.28.0