From: xiao jin <[email protected]>
Date: Tue, 25 Mar 2014 15:54:36 +0800
Subject: [PATCH] cdc-acm: some enhancement on acm delayed write
We find two problems on acm tty write delayed mechanism.
(1) When acm resume, the delayed wb will be started. But now
only one write can be saved during acm suspend. More acm write
may be abandoned.
(2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
close. If acm resume callback run after ASYNCB_INITIALIZED flag
cleared, there will have no chance for delayed write to start.
That lead to acm_wb.use can't be cleared. If user space open
acm tty again and try to setd, tty will be blocked in
tty_wait_until_sent for ever.
This patch have two modification.
(1) use list_head to save the write acm_wb during acm suspend.
It can ensure no acm write abandoned.
(2) enable flush buffer callback when acm tty close. acm delayed
wb will start before acm port shutdown callback, it make sure
the delayed acm_wb.use to be cleared. The patch also clear
acm_wb.use and acm.transmitting when port shutdown.
Signed-off-by: xiao jin <[email protected]>
Reviewed-by: David Cohen <[email protected]>
---
drivers/usb/class/cdc-acm.c | 56
++++++++++++++++++++++++++++++-------------
drivers/usb/class/cdc-acm.h | 3 ++-
2 files changed, 42 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff..cfe00b2 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -217,6 +217,24 @@ static int acm_start_wb(struct acm *acm, struct
acm_wb *wb)
}
/*
+ * Delayed write.
+ */
+static int acm_start_delayed_wb(struct acm *acm)
+{
+ struct acm_wb *wb, *n_wb;
+ LIST_HEAD(delay_wb_list);
+
+ spin_lock_irq(&acm->write_lock);
+ list_replace_init(&acm->delayed_wb_list, &delay_wb_list);
+ spin_unlock_irq(&acm->write_lock);
+ list_for_each_entry_safe(wb, n_wb,
+ &delay_wb_list, delay) {
+ list_del(&wb->delay);
+ acm_start_wb(acm, wb);
+ }
+}
+
+/*
* attributes exported through sysfs
*/
static ssize_t show_caps
@@ -580,8 +598,11 @@ static void acm_port_shutdown(struct tty_port *port)
usb_autopm_get_interface(acm->control);
acm_set_control(acm, acm->ctrlout = 0);
usb_kill_urb(acm->ctrlurb);
- for (i = 0; i < ACM_NW; i++)
+ acm->transmitting = 0;
+ for (i = 0; i < ACM_NW; i++) {
usb_kill_urb(acm->wb[i].urb);
+ acm->wb[i].use = 0;
+ }
for (i = 0; i < acm->rx_buflimit; i++)
usb_kill_urb(acm->read_urbs[i]);
acm->control->needs_remote_wakeup = 0;
@@ -608,6 +629,8 @@ static void acm_tty_close(struct tty_struct *tty,
struct file *filp)
{
struct acm *acm = tty->driver_data;
dev_dbg(&acm->control->dev, "%s\n", __func__);
+ /* Set flow_stopped to enable flush buffer*/
+ tty->flow_stopped = 1;
tty_port_close(&acm->port, tty, filp);
}
@@ -646,10 +669,7 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
- acm->delayed_wb = wb;
- else
- usb_autopm_put_interface_async(acm->control);
+ list_add_tail(&wb->delay, &acm->delayed_wb_list);
spin_unlock_irqrestore(&acm->write_lock, flags);
return count; /* A white lie */
}
@@ -688,6 +708,18 @@ static int acm_tty_chars_in_buffer(struct
tty_struct *tty)
return (ACM_NW - acm_wb_is_avail(acm)) * acm->writesize;
}
+static void acm_tty_flush_buffer(struct tty_struct *tty)
+{
+ struct acm *acm = tty->driver_data;
+
+ /* flush delayed write buffer */
+ if (!acm->disconnected) {
+ usb_autopm_get_interface(acm->control);
+ acm_start_delayed_wb(acm);
+ usb_autopm_put_interface(acm->control);
+ }
+}
+
static void acm_tty_throttle(struct tty_struct *tty)
{
struct acm *acm = tty->driver_data;
@@ -1346,6 +1378,7 @@ made_compressed_probe:
snd->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
snd->instance = acm;
}
+ INIT_LIST_HEAD(&acm->delayed_wb_list);
usb_set_intfdata(intf, acm);
@@ -1540,7 +1573,6 @@ static int acm_suspend(struct usb_interface *intf,
pm_message_t message)
static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
- struct acm_wb *wb;
int rv = 0;
int cnt;
@@ -1555,16 +1587,7 @@ static int acm_resume(struct usb_interface *intf)
if (test_bit(ASYNCB_INITIALIZED, &acm->port.flags)) {
rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
- spin_lock_irq(&acm->write_lock);
- if (acm->delayed_wb) {
- wb = acm->delayed_wb;
- acm->delayed_wb = NULL;
- spin_unlock_irq(&acm->write_lock);
- acm_start_wb(acm, wb);
- } else {
- spin_unlock_irq(&acm->write_lock);
- }
-
+ acm_start_delayed_wb(acm);
/*
* delayed error checking because we must
* do the write path at all cost
@@ -1823,6 +1846,7 @@ static const struct tty_operations acm_ops = {
.throttle = acm_tty_throttle,
.unthrottle = acm_tty_unthrottle,
.chars_in_buffer = acm_tty_chars_in_buffer,
+ .flush_buffer = acm_tty_flush_buffer,
.break_ctl = acm_tty_break_ctl,
.set_termios = acm_tty_set_termios,
.tiocmget = acm_tty_tiocmget,
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index e38dc78..42d6427 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -69,6 +69,7 @@ struct acm_wb {
int use;
struct urb *urb;
struct acm *instance;
+ struct list_head delay;
};
struct acm_rb {
@@ -120,7 +121,7 @@ struct acm {
unsigned int throttled:1; /* actually throttled */
unsigned int throttle_req:1; /* throttle requested */
u8 bInterval;
- struct acm_wb *delayed_wb; /* write queued for a device about to be
woken */
+ struct list_head delayed_wb_list; /* delayed wb list */
};
#define CDC_DATA_INTERFACE_TYPE 0x0a
--
1.7.9.5
On Tue, Apr 08, 2014 at 11:05:20AM +0800, Xiao Jin wrote:
> We find two problems on acm tty write delayed mechanism.
Then you should split this into two patches.
> (1) When acm resume, the delayed wb will be started. But now
> only one write can be saved during acm suspend. More acm write
> may be abandoned.
Why not simply return 0 in write and use the tty buffering rather than
implement another buffer in cdc_acm?
> (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> close. If acm resume callback run after ASYNCB_INITIALIZED flag
> cleared, there will have no chance for delayed write to start.
> That lead to acm_wb.use can't be cleared. If user space open
> acm tty again and try to setd, tty will be blocked in
> tty_wait_until_sent for ever.
>
> This patch have two modification.
> (1) use list_head to save the write acm_wb during acm suspend.
> It can ensure no acm write abandoned.
> (2) enable flush buffer callback when acm tty close. acm delayed
> wb will start before acm port shutdown callback, it make sure
> the delayed acm_wb.use to be cleared. The patch also clear
> acm_wb.use and acm.transmitting when port shutdown.
This is not the right way to do this. See below.
> Signed-off-by: xiao jin <[email protected]>
> Reviewed-by: David Cohen <[email protected]>
> ---
> drivers/usb/class/cdc-acm.c | 56
> ++++++++++++++++++++++++++++++-------------
> drivers/usb/class/cdc-acm.h | 3 ++-
> 2 files changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 900f7ff..cfe00b2 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -217,6 +217,24 @@ static int acm_start_wb(struct acm *acm, struct
> acm_wb *wb)
> }
>
> /*
> + * Delayed write.
> + */
> +static int acm_start_delayed_wb(struct acm *acm)
> +{
> + struct acm_wb *wb, *n_wb;
> + LIST_HEAD(delay_wb_list);
> +
> + spin_lock_irq(&acm->write_lock);
> + list_replace_init(&acm->delayed_wb_list, &delay_wb_list);
> + spin_unlock_irq(&acm->write_lock);
> + list_for_each_entry_safe(wb, n_wb,
> + &delay_wb_list, delay) {
> + list_del(&wb->delay);
> + acm_start_wb(acm, wb);
> + }
> +}
> +
> +/*
> * attributes exported through sysfs
> */
> static ssize_t show_caps
> @@ -580,8 +598,11 @@ static void acm_port_shutdown(struct tty_port *port)
> usb_autopm_get_interface(acm->control);
> acm_set_control(acm, acm->ctrlout = 0);
> usb_kill_urb(acm->ctrlurb);
> - for (i = 0; i < ACM_NW; i++)
> + acm->transmitting = 0;
No need to reset transmitting which is balanced by usb_kill_urb below.
> + for (i = 0; i < ACM_NW; i++) {
> usb_kill_urb(acm->wb[i].urb);
> + acm->wb[i].use = 0;
Same here: use is generally cleared by the completion handler -- you
only need to handle any delayed urb.
> + }
> for (i = 0; i < acm->rx_buflimit; i++)
> usb_kill_urb(acm->read_urbs[i]);
> acm->control->needs_remote_wakeup = 0;
> @@ -608,6 +629,8 @@ static void acm_tty_close(struct tty_struct *tty,
> struct file *filp)
> {
> struct acm *acm = tty->driver_data;
> dev_dbg(&acm->control->dev, "%s\n", __func__);
> + /* Set flow_stopped to enable flush buffer*/
> + tty->flow_stopped = 1;
You shouldn't abuse the flow_stopped flag. Simply clear the delayed urb
in shutdown (rather than try to use flush_buffer).
> tty_port_close(&acm->port, tty, filp);
> }
>
> @@ -646,10 +669,7 @@ static int acm_tty_write(struct tty_struct *tty,
>
> usb_autopm_get_interface_async(acm->control);
> if (acm->susp_count) {
> - if (!acm->delayed_wb)
> - acm->delayed_wb = wb;
> - else
> - usb_autopm_put_interface_async(acm->control);
> + list_add_tail(&wb->delay, &acm->delayed_wb_list);
> spin_unlock_irqrestore(&acm->write_lock, flags);
> return count; /* A white lie */
> }
> @@ -688,6 +708,18 @@ static int acm_tty_chars_in_buffer(struct
> tty_struct *tty)
> return (ACM_NW - acm_wb_is_avail(acm)) * acm->writesize;
> }
>
> +static void acm_tty_flush_buffer(struct tty_struct *tty)
> +{
> + struct acm *acm = tty->driver_data;
> +
> + /* flush delayed write buffer */
> + if (!acm->disconnected) {
> + usb_autopm_get_interface(acm->control);
> + acm_start_delayed_wb(acm);
> + usb_autopm_put_interface(acm->control);
> + }
> +}
> +
Again, don't use flush_buffer for this (it's used to discard output
buffers).
Johan
> static void acm_tty_throttle(struct tty_struct *tty)
> {
> struct acm *acm = tty->driver_data;
> @@ -1346,6 +1378,7 @@ made_compressed_probe:
> snd->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> snd->instance = acm;
> }
> + INIT_LIST_HEAD(&acm->delayed_wb_list);
>
> usb_set_intfdata(intf, acm);
>
> @@ -1540,7 +1573,6 @@ static int acm_suspend(struct usb_interface *intf,
> pm_message_t message)
> static int acm_resume(struct usb_interface *intf)
> {
> struct acm *acm = usb_get_intfdata(intf);
> - struct acm_wb *wb;
> int rv = 0;
> int cnt;
>
> @@ -1555,16 +1587,7 @@ static int acm_resume(struct usb_interface *intf)
> if (test_bit(ASYNCB_INITIALIZED, &acm->port.flags)) {
> rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
>
> - spin_lock_irq(&acm->write_lock);
> - if (acm->delayed_wb) {
> - wb = acm->delayed_wb;
> - acm->delayed_wb = NULL;
> - spin_unlock_irq(&acm->write_lock);
> - acm_start_wb(acm, wb);
> - } else {
> - spin_unlock_irq(&acm->write_lock);
> - }
> -
> + acm_start_delayed_wb(acm);
> /*
> * delayed error checking because we must
> * do the write path at all cost
> @@ -1823,6 +1846,7 @@ static const struct tty_operations acm_ops = {
> .throttle = acm_tty_throttle,
> .unthrottle = acm_tty_unthrottle,
> .chars_in_buffer = acm_tty_chars_in_buffer,
> + .flush_buffer = acm_tty_flush_buffer,
> .break_ctl = acm_tty_break_ctl,
> .set_termios = acm_tty_set_termios,
> .tiocmget = acm_tty_tiocmget,
> diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
> index e38dc78..42d6427 100644
> --- a/drivers/usb/class/cdc-acm.h
> +++ b/drivers/usb/class/cdc-acm.h
> @@ -69,6 +69,7 @@ struct acm_wb {
> int use;
> struct urb *urb;
> struct acm *instance;
> + struct list_head delay;
> };
>
> struct acm_rb {
> @@ -120,7 +121,7 @@ struct acm {
> unsigned int throttled:1; /* actually throttled */
> unsigned int throttle_req:1; /* throttle requested */
> u8 bInterval;
> - struct acm_wb *delayed_wb; /* write queued for a device about to be
> woken */
> + struct list_head delayed_wb_list; /* delayed wb list */
> };
>
> #define CDC_DATA_INTERFACE_TYPE 0x0a
On Tue, 2014-04-08 at 09:33 +0200, Johan Hovold wrote:
> On Tue, Apr 08, 2014 at 11:05:20AM +0800, Xiao Jin wrote:
> > We find two problems on acm tty write delayed mechanism.
>
> Then you should split this into two patches.
>
> > (1) When acm resume, the delayed wb will be started. But now
> > only one write can be saved during acm suspend. More acm write
> > may be abandoned.
>
> Why not simply return 0 in write and use the tty buffering rather than
> implement another buffer in cdc_acm?
Yes. We need a single buffer because the tty layer is not happy
when you accept no data. But we should be able to refuse subsequent
writes. Could you test this patch?
Regards
Oliver
>From 1d44c1f2a10b5617824a37c8ec51f5547e482259 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[email protected]>
Date: Tue, 8 Apr 2014 12:17:39 +0200
Subject: [PATCH] cdc-acm: fix consecutive writes while device is suspended
CDC-ACM needs to handle one attempt to write to a suspended
device because we told the tty layer that there is room.
A second attempt may and must fail or we drop data.
Signed-off-by: Oliver Neukum <[email protected]>
CC: [email protected]
---
drivers/usb/class/cdc-acm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff..7ad3105 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -646,10 +646,12 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
+ if (!acm->delayed_wb) {
acm->delayed_wb = wb;
- else
+ } else {
usb_autopm_put_interface_async(acm->control);
+ count = 0;
+ }
spin_unlock_irqrestore(&acm->write_lock, flags);
return count; /* A white lie */
}
--
1.8.4.5
On Tue, 2014-04-08 at 09:33 +0200, Johan Hovold wrote:
> On Tue, Apr 08, 2014 at 11:05:20AM +0800, Xiao Jin wrote:
> > (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> > close. If acm resume callback run after ASYNCB_INITIALIZED flag
> > cleared, there will have no chance for delayed write to start.
> > That lead to acm_wb.use can't be cleared. If user space open
> > acm tty again and try to setd, tty will be blocked in
> > tty_wait_until_sent for ever.
> >
> > This patch have two modification.
> > (1) use list_head to save the write acm_wb during acm suspend.
> > It can ensure no acm write abandoned.
> > (2) enable flush buffer callback when acm tty close. acm delayed
> > wb will start before acm port shutdown callback, it make sure
> > the delayed acm_wb.use to be cleared. The patch also clear
> > acm_wb.use and acm.transmitting when port shutdown.
>
> This is not the right way to do this. See below.
If I see this correctly, then ASYNCB_INITIALIZED is cleared in
tty_port_close() we is called from acm_tty_close(). Thus it should
be enough to make sure that the device is resumed at the beginning
of acm_tty_close() and acm_resume() will do the job automatically.
What do you think?
Regards
Oliver
> (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> close. If acm resume callback run after ASYNCB_INITIALIZED flag
> cleared, there will have no chance for delayed write to start.
> That lead to acm_wb.use can't be cleared. If user space open
> acm tty again and try to setd, tty will be blocked in
> tty_wait_until_sent for ever.
If there is data pending when the close occurs the close path should
block until either the close timeout occurs or the buffer is written.
This sounds more like the implementation of the ACM chars_in_buffer
method is wrong and not counting the deferred bytes as unsent ?
Alan
[ +CC: Oliver ]
On Tue, Apr 08, 2014 at 12:22:29PM +0100, One Thousand Gnomes wrote:
> > (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> > close. If acm resume callback run after ASYNCB_INITIALIZED flag
> > cleared, there will have no chance for delayed write to start.
> > That lead to acm_wb.use can't be cleared. If user space open
> > acm tty again and try to setd, tty will be blocked in
> > tty_wait_until_sent for ever.
>
> If there is data pending when the close occurs the close path should
> block until either the close timeout occurs or the buffer is written.
> This sounds more like the implementation of the ACM chars_in_buffer
> method is wrong and not counting the deferred bytes as unsent ?
The ACM chars_in_buffer does count the deferred bytes so I guess this
can only happen if close happens fast enough that resume is never called
before shutdown (e.g. closing_wait = ASYNC_CLOSING_WAIT_NONE and
drain_delay = 0).
Johan
[ +CC: Alan ]
On Tue, Apr 08, 2014 at 12:33:31PM +0200, Oliver Neukum wrote:
> On Tue, 2014-04-08 at 09:33 +0200, Johan Hovold wrote:
> > On Tue, Apr 08, 2014 at 11:05:20AM +0800, Xiao Jin wrote:
>
> > > (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> > > close. If acm resume callback run after ASYNCB_INITIALIZED flag
> > > cleared, there will have no chance for delayed write to start.
> > > That lead to acm_wb.use can't be cleared. If user space open
> > > acm tty again and try to setd, tty will be blocked in
> > > tty_wait_until_sent for ever.
> > >
> > > This patch have two modification.
> > > (1) use list_head to save the write acm_wb during acm suspend.
> > > It can ensure no acm write abandoned.
> > > (2) enable flush buffer callback when acm tty close. acm delayed
> > > wb will start before acm port shutdown callback, it make sure
> > > the delayed acm_wb.use to be cleared. The patch also clear
> > > acm_wb.use and acm.transmitting when port shutdown.
> >
> > This is not the right way to do this. See below.
>
> If I see this correctly, then ASYNCB_INITIALIZED is cleared in
> tty_port_close() we is called from acm_tty_close(). Thus it should
> be enough to make sure that the device is resumed at the beginning
> of acm_tty_close() and acm_resume() will do the job automatically.
> What do you think?
But the device should already be about to be resumed, right? If the port
is closed fast enough that resume hasn't had time to run before
shutdown is called I think the right thing to do is simply to discard
the delayed bytes (in shutdown).
Johan
On Tue, 2014-04-08 at 15:17 +0200, Johan Hovold wrote:
> [ +CC: Alan ]
>
> On Tue, Apr 08, 2014 at 12:33:31PM +0200, Oliver Neukum wrote:
> > On Tue, 2014-04-08 at 09:33 +0200, Johan Hovold wrote:
> > > On Tue, Apr 08, 2014 at 11:05:20AM +0800, Xiao Jin wrote:
> >
> > > > (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> > > > close. If acm resume callback run after ASYNCB_INITIALIZED flag
> > > > cleared, there will have no chance for delayed write to start.
> > > > That lead to acm_wb.use can't be cleared. If user space open
> > > > acm tty again and try to setd, tty will be blocked in
> > > > tty_wait_until_sent for ever.
> > > >
> > > > This patch have two modification.
> > > > (1) use list_head to save the write acm_wb during acm suspend.
> > > > It can ensure no acm write abandoned.
> > > > (2) enable flush buffer callback when acm tty close. acm delayed
> > > > wb will start before acm port shutdown callback, it make sure
> > > > the delayed acm_wb.use to be cleared. The patch also clear
> > > > acm_wb.use and acm.transmitting when port shutdown.
> > >
> > > This is not the right way to do this. See below.
> >
> > If I see this correctly, then ASYNCB_INITIALIZED is cleared in
> > tty_port_close() we is called from acm_tty_close(). Thus it should
> > be enough to make sure that the device is resumed at the beginning
> > of acm_tty_close() and acm_resume() will do the job automatically.
> > What do you think?
>
> But the device should already be about to be resumed, right? If the port
Yes.
> is closed fast enough that resume hasn't had time to run before
> shutdown is called I think the right thing to do is simply to discard
> the delayed bytes (in shutdown).
I think if we said we have transmitted then we should do so.
Regards
Oliver
> > > If I see this correctly, then ASYNCB_INITIALIZED is cleared in
> > > tty_port_close() we is called from acm_tty_close(). Thus it should
> > > be enough to make sure that the device is resumed at the beginning
> > > of acm_tty_close() and acm_resume() will do the job automatically.
> > > What do you think?
> >
> > But the device should already be about to be resumed, right? If the port
>
> Yes.
>
> > is closed fast enough that resume hasn't had time to run before
> > shutdown is called I think the right thing to do is simply to discard
> > the delayed bytes (in shutdown).
>
> I think if we said we have transmitted then we should do so.
We haven't made any such promises -- only that we've buffered the data.
Just like any write urb can be cancelled at close / shutdown before
having been transmitted.
The user needs to use closing_wait or drain_delay to give the buffers a
chance to empty. If not used at all, or we get a time out, then the data
should be discarded.
Johan
Thanks all for the review. We meet with the problems when developing
product. I would like to explain my understanding.
On 04/08/2014 11:05 AM, Xiao Jin wrote:
>
> We find two problems on acm tty write delayed mechanism.
> (1) When acm resume, the delayed wb will be started. But now
> only one write can be saved during acm suspend. More acm write
> may be abandoned.
The scenario usually happened when user space write series AT after acm
suspend. If acm accept the first AT, what's the reason for acm to refuse
the second AT? If write return 0, user space will try repeatedly until
resume. It looks simpler that acm accept all the data and sent out urb
when resume.
> (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> close. If acm resume callback run after ASYNCB_INITIALIZED flag
> cleared, there will have no chance for delayed write to start.
> That lead to acm_wb.use can't be cleared. If user space open
> acm tty again and try to setd, tty will be blocked in
> tty_wait_until_sent for ever.
>
We see tty write and close concurrently after acm suspend in this case.
It looks no method to avoid it from tty layer. acm_tty_write and
acm_resume call after acm_port_shutdown. It looks any action in
acm_port_shutdown can't solve the problem. As acm has accepted the user
space data, we can only find a way to send out urb. I feel anyway to
discard the data looks like a lie to user space.
In my understanding acm should accept data as much as possible, and send
out urb as soon as possible. What do you think of?
Br, Jin
On Wed, Apr 09, 2014 at 10:57:10PM +0800, Xiao Jin wrote:
> Thanks all for the review. We meet with the problems when developing
> product. I would like to explain my understanding.
>
> On 04/08/2014 11:05 AM, Xiao Jin wrote:
> >
> >We find two problems on acm tty write delayed mechanism.
> >(1) When acm resume, the delayed wb will be started. But now
> >only one write can be saved during acm suspend. More acm write
> >may be abandoned.
>
> The scenario usually happened when user space write series AT after acm
> suspend. If acm accept the first AT, what's the reason for acm to refuse the
> second AT? If write return 0, user space will try repeatedly until resume.
> It looks simpler that acm accept all the data and sent out urb when resume.
That was my understanding too. The delayed mechanism is not being
implemented by this patch, just improved.
Br, David
>
> >(2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> >close. If acm resume callback run after ASYNCB_INITIALIZED flag
> >cleared, there will have no chance for delayed write to start.
> >That lead to acm_wb.use can't be cleared. If user space open
> >acm tty again and try to setd, tty will be blocked in
> >tty_wait_until_sent for ever.
> >
>
> We see tty write and close concurrently after acm suspend in this case. It
> looks no method to avoid it from tty layer. acm_tty_write and acm_resume
> call after acm_port_shutdown. It looks any action in acm_port_shutdown can't
> solve the problem. As acm has accepted the user space data, we can only find
> a way to send out urb. I feel anyway to discard the data looks like a lie to
> user space.
>
> In my understanding acm should accept data as much as possible, and send out
> urb as soon as possible. What do you think of?
>
> Br, Jin
On Wed, 2014-04-09 at 22:57 +0800, Xiao Jin wrote:
> Thanks all for the review. We meet with the problems when developing
> product. I would like to explain my understanding.
>
> On 04/08/2014 11:05 AM, Xiao Jin wrote:
> >
> > We find two problems on acm tty write delayed mechanism.
> > (1) When acm resume, the delayed wb will be started. But now
> > only one write can be saved during acm suspend. More acm write
> > may be abandoned.
>
> The scenario usually happened when user space write series AT after acm
> suspend. If acm accept the first AT, what's the reason for acm to refuse
> the second AT? If write return 0, user space will try repeatedly until
> resume. It looks simpler that acm accept all the data and sent out urb
> when resume.
No. We cannot accept an arbitrary amount of data. It would let any
user OOM the system. There will have to be an arbitrary limit.
The simplest limit is 1 urb. And that is because we said that we
are ready to accept data.
> > (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> > close. If acm resume callback run after ASYNCB_INITIALIZED flag
> > cleared, there will have no chance for delayed write to start.
> > That lead to acm_wb.use can't be cleared. If user space open
> > acm tty again and try to setd, tty will be blocked in
> > tty_wait_until_sent for ever.
> >
>
> We see tty write and close concurrently after acm suspend in this case.
> It looks no method to avoid it from tty layer. acm_tty_write and
There is a delay user space can set.
> acm_resume call after acm_port_shutdown. It looks any action in
> acm_port_shutdown can't solve the problem. As acm has accepted the user
> space data, we can only find a way to send out urb. I feel anyway to
> discard the data looks like a lie to user space.
>
> In my understanding acm should accept data as much as possible, and send
> out urb as soon as possible. What do you think of?
There's certainly no problem with sending out the data. Yet
simply resuming the device in shutdown() should do the job.
Regards
Oliver
Hi, Oliver,
On 04/10/2014 04:02 PM, Oliver Neukum wrote:
> On Wed, 2014-04-09 at 22:57 +0800, Xiao Jin wrote:
>> Thanks all for the review. We meet with the problems when developing
>> product. I would like to explain my understanding.
>>
>> On 04/08/2014 11:05 AM, Xiao Jin wrote:
>>>
>>> We find two problems on acm tty write delayed mechanism.
>>> (1) When acm resume, the delayed wb will be started. But now
>>> only one write can be saved during acm suspend. More acm write
>>> may be abandoned.
>>
>> The scenario usually happened when user space write series AT after acm
>> suspend. If acm accept the first AT, what's the reason for acm to refuse
>> the second AT? If write return 0, user space will try repeatedly until
>> resume. It looks simpler that acm accept all the data and sent out urb
>> when resume.
>
> No. We cannot accept an arbitrary amount of data. It would let any
> user OOM the system. There will have to be an arbitrary limit.
> The simplest limit is 1 urb. And that is because we said that we
> are ready to accept data.
>
We apply cdc-acm for modem AT data. I can find other usb modem driver
usb_wwan_write use list to accept more data when suspend, maybe usbnet
is the same. Do you have any more reason for me to understand why
cdc-acm accept only one?
>>> (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
>>> close. If acm resume callback run after ASYNCB_INITIALIZED flag
>>> cleared, there will have no chance for delayed write to start.
>>> That lead to acm_wb.use can't be cleared. If user space open
>>> acm tty again and try to setd, tty will be blocked in
>>> tty_wait_until_sent for ever.
>>>
>>
>> We see tty write and close concurrently after acm suspend in this case.
>> It looks no method to avoid it from tty layer. acm_tty_write and
>
> There is a delay user space can set.
>
>> acm_resume call after acm_port_shutdown. It looks any action in
>> acm_port_shutdown can't solve the problem. As acm has accepted the user
>> space data, we can only find a way to send out urb. I feel anyway to
>> discard the data looks like a lie to user space.
>>
>> In my understanding acm should accept data as much as possible, and send
>> out urb as soon as possible. What do you think of?
>
> There's certainly no problem with sending out the data. Yet
> simply resuming the device in shutdown() should do the job.
>
We see tty write and close concurrently, we have debug log to show that
acm_tty_write and acm_resume is called after acm_port_shutdown, I don't
understand "resuming the device in shutdown() should do the job".
> Regards
> Oliver
>
>
My understanding may be superficial, please correct me if anything
wrong. Thanks.
Br, Jin
[ +CC: Jiri and Peter ]
On Thu, Apr 10, 2014 at 10:02:03AM +0200, Oliver Neukum wrote:
> On Wed, 2014-04-09 at 22:57 +0800, Xiao Jin wrote:
> > Thanks all for the review. We meet with the problems when developing
> > product. I would like to explain my understanding.
> >
> > On 04/08/2014 11:05 AM, Xiao Jin wrote:
> > >
> > > We find two problems on acm tty write delayed mechanism.
> > > (1) When acm resume, the delayed wb will be started. But now
> > > only one write can be saved during acm suspend. More acm write
> > > may be abandoned.
> >
> > The scenario usually happened when user space write series AT after acm
> > suspend. If acm accept the first AT, what's the reason for acm to refuse
> > the second AT? If write return 0, user space will try repeatedly until
> > resume. It looks simpler that acm accept all the data and sent out urb
> > when resume.
>
> No. We cannot accept an arbitrary amount of data. It would let any
> user OOM the system. There will have to be an arbitrary limit.
> The simplest limit is 1 urb. And that is because we said that we
> are ready to accept data.
That doesn't make much sense. Either tty can handle write returning 0 or
it doesn't. Consider what happens when you get two consecutive writes
(both preceded by write_room). Why should buffering the first write
solve anything? In fact, it doesn't (at least not data being dropped).
If buffering is indeed needed, we must buffer at least as much data as
reported by write_room, which in this cause should amount to buffering
all write_urbs as Jin suggests.
And by the way, OOM is not an issue with Jin's patch as only ACM_NW (16)
urbs are allocated and can be queued (the buffer should probably have
been implemented using urb anchors, though).
> > > (2) acm tty port ASYNCB_INITIALIZED flag will be cleared when
> > > close. If acm resume callback run after ASYNCB_INITIALIZED flag
> > > cleared, there will have no chance for delayed write to start.
> > > That lead to acm_wb.use can't be cleared. If user space open
> > > acm tty again and try to setd, tty will be blocked in
> > > tty_wait_until_sent for ever.
> > >
> >
> > We see tty write and close concurrently after acm suspend in this case.
> > It looks no method to avoid it from tty layer. acm_tty_write and
>
> There is a delay user space can set.
>
> > acm_resume call after acm_port_shutdown. It looks any action in
> > acm_port_shutdown can't solve the problem. As acm has accepted the user
> > space data, we can only find a way to send out urb. I feel anyway to
> > discard the data looks like a lie to user space.
It's not a lie. Consider what happens if you write a large buffer to a
device using a 300 baud line? We have mechanisms (e.g. tcdrain() and
closing_wait) to let the user decide whether to wait for that data to
drain or not. Please also note that when using flow control, this could
take literally forever.
Jin, what is closing_wait set to in your application? The default is 30
seconds, which means there should have been plenty of the time for the
device to resume (and submit any buffered data).
> > In my understanding acm should accept data as much as possible, and send
> > out urb as soon as possible. What do you think of?
>
> There's certainly no problem with sending out the data. Yet
> simply resuming the device in shutdown() should do the job.
As soon as possible, yes, but again, we shouldn't be sending out urbs
at shutdown (that is were we stop transmissions). (Resuming the device at
close could be ok, but that would still be redundant as we have already
done the async autopm_get in write.)
If the data is precious, make sure to have a reasonable closing_wait, or
it may be dropped.
All that said, there are some serious bugs in the ACM driver: write is
dropping data and leaking write urbs, _and_ buffered urbs are never
reclaimed at shutdown.
If it is ok to not buffer anything in write even though write_room
returned >0, then I think we should simply rip out the buffering from
the ACM driver and rely on the tty buffers. Otherwise, we must make sure
that the buffer space matches what is returned by write_room and buffer
all 16 write-urbs if needed (preferably, using urb anchors).
When implementing the first option, I came across what appears to be a
line-discipline bug which needs to be addressed first, though. Please
have a look at the follow-up RFC.
Thanks,
Johan
Fix characters being dropped by n_tty_write() due to a failure to
check the return value of tty_put_char() in do_output_char().
Characters are currently being dropped by write if a tty driver claims
to have write room available, but still fails to buffer any data (e.g.
if a driver without internal buffer is run-time suspended between
write_room and write).
Note that process_output_block() handles such a failed buffer attempt,
but that the consecutive process_output() of the first character will
drop it. This could lead to the whole buffer being silently dropped
as the remainder of the buffer is processed in a loop.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/n_tty.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d15624c1b751..d22a2d8f1cb7 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -513,7 +513,9 @@ static int do_output_char(unsigned char c, struct tty_struct *tty, int space)
break;
}
- tty_put_char(tty, c);
+ if (!tty_put_char(tty, c))
+ return -1;
+
return 1;
}
--
1.8.3.2
The current runtime suspend implementation is broken in several ways:
Firstly, it buffers only the first write request being made while
suspended -- any further writes are silently dropped.
Secondly, writes being dropped also leak write urbs which are never
reclaimed (until the device is unbound).
Thirdly, even the single buffered write is not cleared at shutdown
(which may happen before the device is resumed), something which can
lead to another urb leak.
Fix this by simple removing the broken and redundant buffer
implementation from the driver, and rely on the tty buffers instead.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/class/cdc-acm.c | 28 +++++++---------------------
drivers/usb/class/cdc-acm.h | 1 -
2 files changed, 7 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff805ee..c85bf3062bba 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -646,12 +646,10 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
- acm->delayed_wb = wb;
- else
- usb_autopm_put_interface_async(acm->control);
+ wb->use = 0;
+ usb_autopm_put_interface_async(acm->control);
spin_unlock_irqrestore(&acm->write_lock, flags);
- return count; /* A white lie */
+ return 0;
}
usb_mark_last_busy(acm->dev);
@@ -1540,7 +1538,6 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
- struct acm_wb *wb;
int rv = 0;
int cnt;
@@ -1554,25 +1551,14 @@ static int acm_resume(struct usb_interface *intf)
if (test_bit(ASYNCB_INITIALIZED, &acm->port.flags)) {
rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
-
- spin_lock_irq(&acm->write_lock);
- if (acm->delayed_wb) {
- wb = acm->delayed_wb;
- acm->delayed_wb = NULL;
- spin_unlock_irq(&acm->write_lock);
- acm_start_wb(acm, wb);
- } else {
- spin_unlock_irq(&acm->write_lock);
- }
-
- /*
- * delayed error checking because we must
- * do the write path at all cost
- */
if (rv < 0)
goto err_out;
rv = acm_submit_read_urbs(acm, GFP_NOIO);
+ if (rv < 0)
+ goto err_out;
+
+ schedule_work(&acm->work);
}
err_out:
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index e38dc785808f..538bb1b3b0bc 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -120,7 +120,6 @@ struct acm {
unsigned int throttled:1; /* actually throttled */
unsigned int throttle_req:1; /* throttle requested */
u8 bInterval;
- struct acm_wb *delayed_wb; /* write queued for a device about to be woken */
};
#define CDC_DATA_INTERFACE_TYPE 0x0a
--
1.8.3.2
On Tue, Apr 08, 2014 at 12:22:22PM +0200, Oliver Neukum wrote:
> On Tue, 2014-04-08 at 09:33 +0200, Johan Hovold wrote:
> > On Tue, Apr 08, 2014 at 11:05:20AM +0800, Xiao Jin wrote:
> > > We find two problems on acm tty write delayed mechanism.
> >
> > Then you should split this into two patches.
> >
> > > (1) When acm resume, the delayed wb will be started. But now
> > > only one write can be saved during acm suspend. More acm write
> > > may be abandoned.
> >
> > Why not simply return 0 in write and use the tty buffering rather than
> > implement another buffer in cdc_acm?
>
> Yes. We need a single buffer because the tty layer is not happy
> when you accept no data. But we should be able to refuse subsequent
> writes. Could you test this patch?
>
> Regards
> Oliver
>
> From 1d44c1f2a10b5617824a37c8ec51f5547e482259 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <[email protected]>
> Date: Tue, 8 Apr 2014 12:17:39 +0200
> Subject: [PATCH] cdc-acm: fix consecutive writes while device is suspended
>
> CDC-ACM needs to handle one attempt to write to a suspended
> device because we told the tty layer that there is room.
> A second attempt may and must fail or we drop data.
>
> Signed-off-by: Oliver Neukum <[email protected]>
> CC: [email protected]
> ---
> drivers/usb/class/cdc-acm.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 900f7ff..7ad3105 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -646,10 +646,12 @@ static int acm_tty_write(struct tty_struct *tty,
>
> usb_autopm_get_interface_async(acm->control);
> if (acm->susp_count) {
> - if (!acm->delayed_wb)
> + if (!acm->delayed_wb) {
> acm->delayed_wb = wb;
> - else
> + } else {
> usb_autopm_put_interface_async(acm->control);
> + count = 0;
You would still leak write urbs here, unless you set wb.use = 0.
> + }
> spin_unlock_irqrestore(&acm->write_lock, flags);
> return count; /* A white lie */
> }
On Fri, 2014-04-11 at 06:51 +0800, Xiao Jin wrote:
> Hi, Oliver,
>
> On 04/10/2014 04:02 PM, Oliver Neukum wrote:
> > On Wed, 2014-04-09 at 22:57 +0800, Xiao Jin wrote:
> >> Thanks all for the review. We meet with the problems when developing
> >> product. I would like to explain my understanding.
> >>
> >> On 04/08/2014 11:05 AM, Xiao Jin wrote:
> >>>
> >>> We find two problems on acm tty write delayed mechanism.
> >>> (1) When acm resume, the delayed wb will be started. But now
> >>> only one write can be saved during acm suspend. More acm write
> >>> may be abandoned.
> >>
> >> The scenario usually happened when user space write series AT after acm
> >> suspend. If acm accept the first AT, what's the reason for acm to refuse
> >> the second AT? If write return 0, user space will try repeatedly until
> >> resume. It looks simpler that acm accept all the data and sent out urb
> >> when resume.
> >
> > No. We cannot accept an arbitrary amount of data. It would let any
> > user OOM the system. There will have to be an arbitrary limit.
> > The simplest limit is 1 urb. And that is because we said that we
> > are ready to accept data.
> >
>
> We apply cdc-acm for modem AT data. I can find other usb modem driver
> usb_wwan_write use list to accept more data when suspend, maybe usbnet
> is the same. Do you have any more reason for me to understand why
> cdc-acm accept only one?
User space must be ready to deal with a device that cannot
accept any more data. There is simply no additional benefit
in more caching.
> We see tty write and close concurrently, we have debug log to show that
> acm_tty_write and acm_resume is called after acm_port_shutdown, I don't
> understand "resuming the device in shutdown() should do the job".
There we have a problem. In fact it looks like a bug in the tty layer.
Could you post the logs?
Regards
Oliver
On Fri, 11 Apr 2014 11:41:24 +0200
Johan Hovold <[email protected]> wrote:
> Fix characters being dropped by n_tty_write() due to a failure to
> check the return value of tty_put_char() in do_output_char().
>
> Characters are currently being dropped by write if a tty driver claims
> to have write room available, but still fails to buffer any data
Your driver is buggy. If you advertise a buffer you must honour it and
neither shrink nor revoke it.
For an URB based device you almost certainly want internal buffering so
you can do proper packetisation. USB serial these days gets it right - see
drivers/usb/serial/generic.c for a fairly simple kfifo based approach.
Whether applying it to cdc_acm would make sense I don't know but it looks
like it might be simpler over-all than the current arrangement.
Alan
On Mon, 2014-04-14 at 13:53 +0100, One Thousand Gnomes wrote:
> On Fri, 11 Apr 2014 11:41:24 +0200
> Johan Hovold <[email protected]> wrote:
>
> > Fix characters being dropped by n_tty_write() due to a failure to
> > check the return value of tty_put_char() in do_output_char().
> >
> > Characters are currently being dropped by write if a tty driver claims
> > to have write room available, but still fails to buffer any data
>
> Your driver is buggy. If you advertise a buffer you must honour it and
> neither shrink nor revoke it.
Is that a general rule or does it apply only till the next write?
Regards
Oliver
On Mon, Apr 14, 2014 at 01:53:33PM +0100, One Thousand Gnomes wrote:
> On Fri, 11 Apr 2014 11:41:24 +0200
> Johan Hovold <[email protected]> wrote:
>
> > Fix characters being dropped by n_tty_write() due to a failure to
> > check the return value of tty_put_char() in do_output_char().
> >
> > Characters are currently being dropped by write if a tty driver claims
> > to have write room available, but still fails to buffer any data
>
> Your driver is buggy. If you advertise a buffer you must honour it and
> neither shrink nor revoke it.
Very well, that settles the question.
> For an URB based device you almost certainly want internal buffering so
> you can do proper packetisation. USB serial these days gets it right - see
> drivers/usb/serial/generic.c for a fairly simple kfifo based approach.
I'm quite aware of the usb-serial approach as I implemented it. ;)
I have considered "porting" it to the ACM driver, and unless anyone is
against the extra copy the fifo would imply, I'll go ahead and do just
that (although, I know of at least one buggy ACM device that violates
the ACM specification and cannot handle merged writes...).
> Whether applying it to cdc_acm would make sense I don't know but it looks
> like it might be simpler over-all than the current arrangement.
With the current fifo-less implementation, the ACM driver could use the
approach taken by the usb-wwan and sierra usb-serial drivers, which
queue up all there write urbs while suspended. That way the available
buffer space never shrinks.
Thanks,
Johan
On Mon, 14 Apr 2014 15:05:49 +0200
Oliver Neukum <[email protected]> wrote:
> On Mon, 2014-04-14 at 13:53 +0100, One Thousand Gnomes wrote:
> > On Fri, 11 Apr 2014 11:41:24 +0200
> > Johan Hovold <[email protected]> wrote:
> >
> > > Fix characters being dropped by n_tty_write() due to a failure to
> > > check the return value of tty_put_char() in do_output_char().
> > >
> > > Characters are currently being dropped by write if a tty driver claims
> > > to have write room available, but still fails to buffer any data
> >
> > Your driver is buggy. If you advertise a buffer you must honour it and
> > neither shrink nor revoke it.
>
> Is that a general rule or does it apply only till the next write?
Its a general rule. If you've advertised 5 bytes then you can only
advertise 0 after 5 bytes have been sent your way (or a hangup etc
occurred).
Alan
The current ACM runtime-suspend implementation is broken in several
ways:
Firstly, it buffers only the first write request being made while
suspended -- any further writes are silently dropped.
Secondly, writes being dropped also leak write urbs, which are never
reclaimed (until the device is unbound).
Thirdly, even the single buffered write is not cleared at shutdown
(which may happen before the device is resumed), something which can
lead to another urb leak as well as a PM usage-counter leak.
Fix this by implementing a delayed-write queue using urb anchors and
making sure to discard the queue properly at shutdown.
Reported-by: Xiao Jin <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
Let's fix the current runtime-suspend implementation before considering
adding a write fifo.
Since this has never really worked, I'll let someone else decide whether
the fix should be back-ported to stable or not.
Jin, did you check what closing_wait setting your application is using?
Could you give this patch a try as well?
Thanks,
Johan
drivers/usb/class/cdc-acm.c | 36 +++++++++++++++++++++++-------------
drivers/usb/class/cdc-acm.h | 2 +-
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 900f7ff805ee..ebbcc7a6a7c8 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -571,6 +571,8 @@ static void acm_port_destruct(struct tty_port *port)
static void acm_port_shutdown(struct tty_port *port)
{
struct acm *acm = container_of(port, struct acm, port);
+ struct urb *urb;
+ struct acm_wb *wb;
int i;
dev_dbg(&acm->control->dev, "%s\n", __func__);
@@ -578,6 +580,16 @@ static void acm_port_shutdown(struct tty_port *port)
mutex_lock(&acm->mutex);
if (!acm->disconnected) {
usb_autopm_get_interface(acm->control);
+ spin_lock_irq(&acm->write_lock);
+ for (;;) {
+ urb = usb_get_from_anchor(&acm->delayed);
+ if (!urb)
+ break;
+ wb = urb->context;
+ wb->use = 0;
+ usb_autopm_put_interface_async(acm->control);
+ }
+ spin_unlock_irq(&acm->write_lock);
acm_set_control(acm, acm->ctrlout = 0);
usb_kill_urb(acm->ctrlurb);
for (i = 0; i < ACM_NW; i++)
@@ -646,12 +658,9 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
- if (!acm->delayed_wb)
- acm->delayed_wb = wb;
- else
- usb_autopm_put_interface_async(acm->control);
+ usb_anchor_urb(wb->urb, &acm->delayed);
spin_unlock_irqrestore(&acm->write_lock, flags);
- return count; /* A white lie */
+ return count;
}
usb_mark_last_busy(acm->dev);
@@ -1267,6 +1276,7 @@ made_compressed_probe:
acm->bInterval = epread->bInterval;
tty_port_init(&acm->port);
acm->port.ops = &acm_port_ops;
+ init_usb_anchor(&acm->delayed);
buf = usb_alloc_coherent(usb_dev, ctrlsize, GFP_KERNEL, &acm->ctrl_dma);
if (!buf) {
@@ -1540,7 +1550,7 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
static int acm_resume(struct usb_interface *intf)
{
struct acm *acm = usb_get_intfdata(intf);
- struct acm_wb *wb;
+ struct urb *urb;
int rv = 0;
int cnt;
@@ -1556,14 +1566,14 @@ static int acm_resume(struct usb_interface *intf)
rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
spin_lock_irq(&acm->write_lock);
- if (acm->delayed_wb) {
- wb = acm->delayed_wb;
- acm->delayed_wb = NULL;
- spin_unlock_irq(&acm->write_lock);
- acm_start_wb(acm, wb);
- } else {
- spin_unlock_irq(&acm->write_lock);
+ for (;;) {
+ urb = usb_get_from_anchor(&acm->delayed);
+ if (!urb)
+ break;
+
+ acm_start_wb(acm, urb->context);
}
+ spin_unlock_irq(&acm->write_lock);
/*
* delayed error checking because we must
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index e38dc785808f..80826f843e04 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -120,7 +120,7 @@ struct acm {
unsigned int throttled:1; /* actually throttled */
unsigned int throttle_req:1; /* throttle requested */
u8 bInterval;
- struct acm_wb *delayed_wb; /* write queued for a device about to be woken */
+ struct usb_anchor delayed; /* writes queued for a device about to be woken */
};
#define CDC_DATA_INTERFACE_TYPE 0x0a
--
1.8.3.2
Hi, Johan,
On 04/15/2014 03:58 AM, Johan Hovold wrote:
> The current ACM runtime-suspend implementation is broken in several
> ways:
>
> Firstly, it buffers only the first write request being made while
> suspended -- any further writes are silently dropped.
>
> Secondly, writes being dropped also leak write urbs, which are never
> reclaimed (until the device is unbound).
>
> Thirdly, even the single buffered write is not cleared at shutdown
> (which may happen before the device is resumed), something which can
> lead to another urb leak as well as a PM usage-counter leak.
>
> Fix this by implementing a delayed-write queue using urb anchors and
> making sure to discard the queue properly at shutdown.
>
> Reported-by: Xiao Jin <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
>
> Let's fix the current runtime-suspend implementation before considering
> adding a write fifo.
>
> Since this has never really worked, I'll let someone else decide whether
> the fix should be back-ported to stable or not.
>
> Jin, did you check what closing_wait setting your application is using?
I check the closing_wait is 30s by default. Below is the trace we get
when reproduced problem.
<...>-1360 [003] d.s5 1843.061418: acm_tty_write:
acm_tty_write - write 65
<...>-1360 [003] d.s5 1843.061425: acm_write_start:
acm_write_start - susp_count 2
<...>-2535 [002] .... 1843.180687: acm_tty_close:
acm_tty_close
<...>-2535 [002] .... 1843.181217: acm_wb_is_avail: avail n=15
<...>-2535 [002] .... 1843.181238: acm_port_shutdown:
acm_port_shutdown
<...>-438 [003] .... 1843.182803: acm_wb_is_avail: avail n=16
<...>-438 [003] d..1 1843.182817: acm_tty_write:
acm_tty_write - write 11
<...>-438 [003] d..1 1843.182826: acm_write_start:
acm_write_start - susp_count 2
<...>-37 [003] .... 1843.202884: acm_resume: wgq[acm_resume]
<...>-37 [003] .... 1843.202892: acm_resume: wgq[acm_resume]
<...>-37 [003] d..1 1843.203195: acm_resume: send
d_wb-1046297992
<...>-37 [003] .... 1843.203199: acm_start_wb:
acm_start_wb, acm->transmitting=0
<idle>-0 [000] d.h2 1843.203343: acm_write_done.isra.11:
acm_write_done, acm->transmitting=1
<...>-1989 [001] .... 1843.207197: acm_tty_cleanup:
acm_tty_cleanup
There are two acms in the case, acm0 and acm3. acm0 have delayed 65
bytes before close. When acm0 close, ASYNCB_INITIALIZED flag is cleared,
that lead to acm0 have no chance to start delayed wb during acm resume.
acm3 delayed 11 bytes send out because it still is opened.
It looks closing_wait didn't take effect at that time. I am not sure the
reason why because we have no more debug log. Now We are checking the
issue again.
> Could you give this patch a try as well?
I try the write and resume part of this patch, anchor urb works well.
>
> Thanks,
> Johan
>
>
> drivers/usb/class/cdc-acm.c | 36 +++++++++++++++++++++++-------------
> drivers/usb/class/cdc-acm.h | 2 +-
> 2 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 900f7ff805ee..ebbcc7a6a7c8 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -571,6 +571,8 @@ static void acm_port_destruct(struct tty_port *port)
> static void acm_port_shutdown(struct tty_port *port)
> {
> struct acm *acm = container_of(port, struct acm, port);
> + struct urb *urb;
> + struct acm_wb *wb;
> int i;
>
> dev_dbg(&acm->control->dev, "%s\n", __func__);
> @@ -578,6 +580,16 @@ static void acm_port_shutdown(struct tty_port *port)
> mutex_lock(&acm->mutex);
> if (!acm->disconnected) {
> usb_autopm_get_interface(acm->control);
> + spin_lock_irq(&acm->write_lock);
> + for (;;) {
> + urb = usb_get_from_anchor(&acm->delayed);
> + if (!urb)
> + break;
> + wb = urb->context;
> + wb->use = 0;
> + usb_autopm_put_interface_async(acm->control);
> + }
> + spin_unlock_irq(&acm->write_lock);
> acm_set_control(acm, acm->ctrlout = 0);
> usb_kill_urb(acm->ctrlurb);
> for (i = 0; i < ACM_NW; i++)
> @@ -646,12 +658,9 @@ static int acm_tty_write(struct tty_struct *tty,
>
> usb_autopm_get_interface_async(acm->control);
> if (acm->susp_count) {
> - if (!acm->delayed_wb)
> - acm->delayed_wb = wb;
> - else
> - usb_autopm_put_interface_async(acm->control);
> + usb_anchor_urb(wb->urb, &acm->delayed);
> spin_unlock_irqrestore(&acm->write_lock, flags);
> - return count; /* A white lie */
> + return count;
> }
> usb_mark_last_busy(acm->dev);
>
> @@ -1267,6 +1276,7 @@ made_compressed_probe:
> acm->bInterval = epread->bInterval;
> tty_port_init(&acm->port);
> acm->port.ops = &acm_port_ops;
> + init_usb_anchor(&acm->delayed);
>
> buf = usb_alloc_coherent(usb_dev, ctrlsize, GFP_KERNEL, &acm->ctrl_dma);
> if (!buf) {
> @@ -1540,7 +1550,7 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
> static int acm_resume(struct usb_interface *intf)
> {
> struct acm *acm = usb_get_intfdata(intf);
> - struct acm_wb *wb;
> + struct urb *urb;
> int rv = 0;
> int cnt;
>
> @@ -1556,14 +1566,14 @@ static int acm_resume(struct usb_interface *intf)
> rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
>
> spin_lock_irq(&acm->write_lock);
> - if (acm->delayed_wb) {
> - wb = acm->delayed_wb;
> - acm->delayed_wb = NULL;
> - spin_unlock_irq(&acm->write_lock);
> - acm_start_wb(acm, wb);
> - } else {
> - spin_unlock_irq(&acm->write_lock);
> + for (;;) {
> + urb = usb_get_from_anchor(&acm->delayed);
> + if (!urb)
> + break;
> +
> + acm_start_wb(acm, urb->context);
> }
> + spin_unlock_irq(&acm->write_lock);
>
> /*
> * delayed error checking because we must
> diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
> index e38dc785808f..80826f843e04 100644
> --- a/drivers/usb/class/cdc-acm.h
> +++ b/drivers/usb/class/cdc-acm.h
> @@ -120,7 +120,7 @@ struct acm {
> unsigned int throttled:1; /* actually throttled */
> unsigned int throttle_req:1; /* throttle requested */
> u8 bInterval;
> - struct acm_wb *delayed_wb; /* write queued for a device about to be woken */
> + struct usb_anchor delayed; /* writes queued for a device about to be woken */
> };
>
> #define CDC_DATA_INTERFACE_TYPE 0x0a
>
On Mon, 2014-04-14 at 21:58 +0200, Johan Hovold wrote:
> Fix this by implementing a delayed-write queue using urb anchors and
> making sure to discard the queue properly at shutdown.
Looks very good, with one exception: acm_tty_close() must
synchronously resume the device so that the anchor is emptied
before ASYNCB is cleared.
Regards
Oliver
On Tue, Apr 15, 2014 at 04:24:10PM +0800, Xiao Jin wrote:
> On 04/15/2014 03:58 AM, Johan Hovold wrote:
> > Jin, did you check what closing_wait setting your application is using?
>
> I check the closing_wait is 30s by default. Below is the trace we get
> when reproduced problem.
>
> <...>-1360 [003] d.s5 1843.061418: acm_tty_write:
> acm_tty_write - write 65
> <...>-1360 [003] d.s5 1843.061425: acm_write_start:
> acm_write_start - susp_count 2
> <...>-2535 [002] .... 1843.180687: acm_tty_close:
> acm_tty_close
> <...>-2535 [002] .... 1843.181217: acm_wb_is_avail: avail n=15
> <...>-2535 [002] .... 1843.181238: acm_port_shutdown:
> acm_port_shutdown
> <...>-438 [003] .... 1843.182803: acm_wb_is_avail: avail n=16
> <...>-438 [003] d..1 1843.182817: acm_tty_write:
> acm_tty_write - write 11
> <...>-438 [003] d..1 1843.182826: acm_write_start:
> acm_write_start - susp_count 2
> <...>-37 [003] .... 1843.202884: acm_resume: wgq[acm_resume]
> <...>-37 [003] .... 1843.202892: acm_resume: wgq[acm_resume]
> <...>-37 [003] d..1 1843.203195: acm_resume: send
> d_wb-1046297992
> <...>-37 [003] .... 1843.203199: acm_start_wb:
> acm_start_wb, acm->transmitting=0
> <idle>-0 [000] d.h2 1843.203343: acm_write_done.isra.11:
> acm_write_done, acm->transmitting=1
> <...>-1989 [001] .... 1843.207197: acm_tty_cleanup:
> acm_tty_cleanup
>
> There are two acms in the case, acm0 and acm3. acm0 have delayed 65
> bytes before close. When acm0 close, ASYNCB_INITIALIZED flag is cleared,
> that lead to acm0 have no chance to start delayed wb during acm resume.
The ASYNCB_INITIALIZED flag is not cleared until chars_in_buffer returns
0 (or closing_wait times out), so this should not be a problem.
What kernel are you using here?
Can you try to reproduce this using a single ACM device with the
diagnostic patch below applied (on top of v3.14 and my autosuspend fix)?
> acm3 delayed 11 bytes send out because it still is opened.
> It looks closing_wait didn't take effect at that time. I am not sure the
> reason why because we have no more debug log. Now We are checking the
> issue again.
>
> > Could you give this patch a try as well?
>
> I try the write and resume part of this patch, anchor urb works well.
Thanks for testing.
Johan
>From 3c622243c33a815f66e606531edd3a7ff4e579bf Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Tue, 15 Apr 2014 10:44:59 +0200
Subject: [PATCH] USB: cdc-acm: add and enable some extra debugging
---
drivers/usb/class/cdc-acm.c | 35 ++++++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index ebbcc7a6a7c8..44aa8a620f89 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -28,8 +28,8 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
-#undef DEBUG
-#undef VERBOSE_DEBUG
+#define DEBUG
+#define VERBOSE_DEBUG
#include <linux/kernel.h>
#include <linux/errno.h>
@@ -175,6 +175,7 @@ static int acm_wb_is_avail(struct acm *acm)
spin_lock_irqsave(&acm->write_lock, flags);
for (i = 0; i < ACM_NW; i++)
n -= acm->wb[i].use;
+ dev_dbg(&acm->data->dev, "%s - %d\n", __func__, n);
spin_unlock_irqrestore(&acm->write_lock, flags);
return n;
}
@@ -433,6 +434,8 @@ static void acm_write_bulk(struct urb *urb)
struct acm *acm = wb->instance;
unsigned long flags;
+ dev_dbg(&acm->data->dev, "%s\n", __func__);
+
if (urb->status || (urb->actual_length != urb->transfer_buffer_length))
dev_vdbg(&acm->data->dev, "%s - len %d/%d, status %d\n",
__func__,
@@ -632,11 +635,11 @@ static int acm_tty_write(struct tty_struct *tty,
int wbn;
struct acm_wb *wb;
+ dev_vdbg(&acm->data->dev, "%s - count %d, data = %*ph\n", __func__,
+ count, count, buf);
if (!count)
return 0;
- dev_vdbg(&acm->data->dev, "%s - count %d\n", __func__, count);
-
spin_lock_irqsave(&acm->write_lock, flags);
wbn = acm_wb_alloc(acm);
if (wbn < 0) {
@@ -658,6 +661,8 @@ static int acm_tty_write(struct tty_struct *tty,
usb_autopm_get_interface_async(acm->control);
if (acm->susp_count) {
+ dev_dbg(&acm->data->dev, "%s - suspended", __func__);
+
usb_anchor_urb(wb->urb, &acm->delayed);
spin_unlock_irqrestore(&acm->write_lock, flags);
return count;
@@ -675,26 +680,38 @@ static int acm_tty_write(struct tty_struct *tty,
static int acm_tty_write_room(struct tty_struct *tty)
{
struct acm *acm = tty->driver_data;
+ int room;
+
/*
* Do not let the line discipline to know that we have a reserve,
* or it might get too enthusiastic.
*/
- return acm_wb_is_avail(acm) ? acm->writesize : 0;
+ room = acm_wb_is_avail(acm) ? acm->writesize : 0;
+
+ dev_dbg(&acm->data->dev, "%s - %d\n", __func__, room);
+
+ return room;
}
static int acm_tty_chars_in_buffer(struct tty_struct *tty)
{
struct acm *acm = tty->driver_data;
+ int chars = 0;
+
/*
* if the device was unplugged then any remaining characters fell out
* of the connector ;)
*/
if (acm->disconnected)
- return 0;
+ goto out;
/*
* This is inaccurate (overcounts), but it works.
*/
- return (ACM_NW - acm_wb_is_avail(acm)) * acm->writesize;
+ chars = (ACM_NW - acm_wb_is_avail(acm)) * acm->writesize;
+out:
+ dev_dbg(&acm->data->dev, "%s - %d\n", __func__, chars);
+
+ return chars;
}
static void acm_tty_throttle(struct tty_struct *tty)
@@ -1522,6 +1539,8 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
struct acm *acm = usb_get_intfdata(intf);
int cnt;
+ dev_dbg(&intf->dev, "%s\n", __func__);
+
if (PMSG_IS_AUTO(message)) {
int b;
@@ -1554,6 +1573,8 @@ static int acm_resume(struct usb_interface *intf)
int rv = 0;
int cnt;
+ dev_dbg(&intf->dev, "%s\n", __func__);
+
spin_lock_irq(&acm->read_lock);
acm->susp_count -= 1;
cnt = acm->susp_count;
--
1.8.3.2
On Tue, Apr 15, 2014 at 10:35:19AM +0200, Oliver Neukum wrote:
> On Mon, 2014-04-14 at 21:58 +0200, Johan Hovold wrote:
>
> > Fix this by implementing a delayed-write queue using urb anchors and
> > making sure to discard the queue properly at shutdown.
>
> Looks very good, with one exception: acm_tty_close() must
> synchronously resume the device so that the anchor is emptied
> before ASYNCB is cleared.
That isn't necessary as the device is already about to be resumed and
the initialised flag will not be cleared until chars_in_buffer() returns
0 or closing wait times out.
In the first case, the queue has been emptied as the urbs are submitted
at resume(), and in the second case the queue is discarded at
shutdown().
Johan
On Tue, Apr 15, 2014 at 11:13:17AM +0200, Johan Hovold wrote:
> On Tue, Apr 15, 2014 at 10:35:19AM +0200, Oliver Neukum wrote:
> > On Mon, 2014-04-14 at 21:58 +0200, Johan Hovold wrote:
> >
> > > Fix this by implementing a delayed-write queue using urb anchors and
> > > making sure to discard the queue properly at shutdown.
> >
> > Looks very good, with one exception: acm_tty_close() must
> > synchronously resume the device so that the anchor is emptied
> > before ASYNCB is cleared.
>
> That isn't necessary as the device is already about to be resumed and
> the initialised flag will not be cleared until chars_in_buffer() returns
> 0 or closing wait times out.
>
> In the first case, the queue has been emptied as the urbs are submitted
> at resume(), and in the second case the queue is discarded at
> shutdown().
Turns out that the usb_wwan (and thus ipw, option, and qcserial) and
sierra usb-serial drivers also fail to get runtime-suspend right. The
first one leaks urbs and the second does synchronous resume and submits
urbs during shutdown.
I'll fix it up.
Johan