[PATCH] cdc-acm: some enhancement on acm delayed write

[PATCH] cdc-acm: some enhancement on acm delayed write

[Xiao Jin]

From: xiao jin <jin.xiao@intel.com>
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 <jin.xiao@intel.com>
Reviewed-by: David Cohen <david.a.cohen@linux.intel.com>
---
  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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Johan Hovold]

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 <jin.xiao@intel.com>
> Reviewed-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
>   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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Oliver Neukum]

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 <oneukum@suse.de>
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 <oneukum@suse.de>
CC: stable@vger.kernel.org
---
 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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Oliver Neukum]

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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[One Thousand Gnomes]

> (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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Johan Hovold]

[ +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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Johan Hovold]

[ +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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Oliver Neukum]

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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Johan Hovold]

> > > 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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Xiao Jin]

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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[David Cohen]

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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Oliver Neukum]

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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Xiao Jin]

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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Oliver Neukum]

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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Johan Hovold]

[ +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

[RFC 1/2] n_tty: fix dropped output characters

[Johan Hovold]

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 <jhovold@gmail.com>
---
 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

[RFC 2/2] USB: cdc-acm: fix broken runtime suspend

[Johan Hovold]

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 <jhovold@gmail.com>
---
 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

Re: [PATCH] cdc-acm: some enhancement on acm delayed write

[Johan Hovold]

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 <oneukum@suse.de>
> 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 <oneukum@suse.de>
> CC: stable@vger.kernel.org
> ---
>  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 */
>  	}

Re: [RFC 1/2] n_tty: fix dropped output characters

[One Thousand Gnomes]

On Fri, 11 Apr 2014 11:41:24 +0200
Johan Hovold <jhovold@gmail.com> 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

Re: [RFC 1/2] n_tty: fix dropped output characters

[Oliver Neukum]

On Mon, 2014-04-14 at 13:53 +0100, One Thousand Gnomes wrote:
> On Fri, 11 Apr 2014 11:41:24 +0200
> Johan Hovold <jhovold@gmail.com> 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

Re: [RFC 1/2] n_tty: fix dropped output characters

[Johan Hovold]

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 <jhovold@gmail.com> 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

Re: [RFC 1/2] n_tty: fix dropped output characters

[One Thousand Gnomes]

On Mon, 14 Apr 2014 15:05:49 +0200
Oliver Neukum <oneukum@suse.de> 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 <jhovold@gmail.com> 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

[PATCH] USB: cdc-acm: fix broken runtime suspend

[Johan Hovold]

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 <jin.xiao@intel.com>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---

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

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Xiao Jin]

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 <jin.xiao@intel.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
>
> 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
>

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Oliver Neukum]

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

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Johan Hovold]

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 <jhovold@gmail.com>
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

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Johan Hovold]

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

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Johan Hovold]

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

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Johan Hovold]

On Mon, Apr 14, 2014 at 09:58:12PM +0200, 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 <jin.xiao@intel.com>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>

Greg, 

I understand yoǘ're on your way home from Japan and are getting ready to
work through the 3.16 patch queue.

Could you please discard this one for now? I've found a couple of more
PM related problems and I'll submit a slight update of this one as part
of a larger series of fixes instead.

Thanks,
Johan

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Greg Kroah-Hartman]

On Sat, May 24, 2014 at 04:42:42PM +0200, Johan Hovold wrote:
> On Mon, Apr 14, 2014 at 09:58:12PM +0200, 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 <jin.xiao@intel.com>
> > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> 
> Greg, 
> 
> I understand yoǘ're on your way home from Japan and are getting ready to
> work through the 3.16 patch queue.
> 
> Could you please discard this one for now? I've found a couple of more
> PM related problems and I'll submit a slight update of this one as part
> of a larger series of fixes instead.

I think it's already in my tree, right?  I don't see it in my to-apply
queue.  Can you send me the git id to revert, or just a patch that
reverts it?

thanks,

greg k-h

Re: [PATCH] USB: cdc-acm: fix broken runtime suspend

[Johan Hovold]

On Sat, May 24, 2014 at 12:59:07PM -0700, Greg Kroah-Hartman wrote:
> On Sat, May 24, 2014 at 04:42:42PM +0200, Johan Hovold wrote:

> > Could you please discard this one for now? I've found a couple of more
> > PM related problems and I'll submit a slight update of this one as part
> > of a larger series of fixes instead.
> 
> I think it's already in my tree, right?  I don't see it in my to-apply
> queue.  Can you send me the git id to revert, or just a patch that
> reverts it?

No, you haven't applied it yet, so that's good. I should be able to send
the complete series in a few days.

Thanks,
Johan

[PATCH 00/63] USB: (mostly runtime PM) patches for v3.16-rc

[Johan Hovold]

Turns out that the three current implementations of aggressive runtime
power management in USB-TTY drivers were quite badly broken (and has so
been since first merged in v2.6.27 and v2.6.32).

These patches fix this runtime PM mess, but also do some clean up and
fix a few other issues.

Note that all patches but the cdc-acm ones (and the single wusbcore one)
are usb-serial, but I decided to include them all in one series as
several bugs were reproduced in both cdc-acm and usb-serial.

For reference, I'm responding to the thread with Xiao Jin's initial bug
report, but the full thread is posted to linux-usb only.

Johan


Johan Hovold (61):
  USB: sierra: fix AA deadlock in open error path
  USB: sierra: fix use after free at suspend/resume
  USB: sierra: fix urb and memory leak in resume error path
  USB: sierra: fix urb and memory leak on disconnect
  USB: sierra: fix remote wakeup
  USB: sierra: fix characters being dropped at close
  USB: sierra: fix urbs not being killed on shutdown
  USB: sierra: fix resume error reporting
  USB: sierra: fix line-control pipe direction
  USB: sierra: remove bogus endpoint test
  USB: sierra: remove unused variable
  USB: sierra: remove unimplemented set_termios
  USB: sierra: remove disconnected test from close
  USB: sierra: do not resume I/O on closed ports
  USB: sierra: remove redundant modem-control requests
  USB: sierra: use interface-data accessors
  USB: sierra: clean up suspend
  USB: sierra: refactor delayed-urb submission
  USB: sierra: minimise no-suspend window during close
  USB: sierra: do not resume I/O on closing ports
  USB: option: fix runtime PM handling
  USB: option: fix line-control pipe direction
  USB: option: add missing usb_mark_last_busy
  USB: usb_wwan: fix write and suspend race
  USB: usb_wwan: fix urb leak at shutdown
  USB: usb_wwan: fix potential NULL-deref at resume
  USB: usb_wwan: fix potential blocked I/O after resume
  USB: usb_wwan: fix discarded writes on resume errors
  USB: usb_wwan: fix remote wakeup
  USB: usb_wwan: remove redundant modem-control request
  USB: usb_wwan: remove unimplemented set_termios
  USB: usb_wwan: remove redundant urb kill from port remove
  USB: usb_wwan: kill interrupt urb explicitly at suspend
  USB: usb_wwan: make resume error messages uniform
  USB: usb_wwan: use interface-data accessors
  USB: usb_wwan: clean up delayed-urb submission
  USB: usb_wwan: remove comment from close
  USB: usb_wwan: remove some superfluous comments
  USB: usb_wwan: remove bogus function prototype
  USB: usb_wwan: report failed submissions as errors
  USB: usb_wwan: do not resume I/O on closing ports
  USB: serial: fix potential runtime pm imbalance at device remove
  USB: serial: remove overly defensive port tests
  USB: kobil_sct: fix control requests without data stage
  USB: cdc-acm: fix write and suspend race
  USB: cdc-acm: fix write and resume race
  USB: cdc-acm: fix broken runtime suspend
  USB: cdc-acm: fix runtime PM for control messages
  USB: cdc-acm: fix shutdown and suspend race
  USB: cdc-acm: fix potential urb leak and PM imbalance in write
  USB: cdc-acm: fix open and suspend race
  USB: cdc-acm: fix failed open not being detected
  USB: cdc-acm: fix I/O after failed open
  USB: cdc-acm: fix runtime PM imbalance at shutdown
  USB: cdc-acm: simplify runtime PM locking
  USB: cdc-acm: remove redundant disconnected test from shutdown
  USB: cdc-acm: minimise no-suspend window during shutdown
  USB: cdc-acm: do not update PM busy on read errors
  USB: cdc-acm: remove redundant usb_mark_last_busy
  USB: cdc-acm: use tty-port dtr_rts
  USB: wusbcore: fix control-pipe directions

xiao jin (2):
  USB: usb_wwan: fix urb leak in write error path
  USB: usb_wwan: fix race between write and resume

 drivers/usb/class/cdc-acm.c     | 177 +++++++++++++++++-------------
 drivers/usb/class/cdc-acm.h     |   2 +-
 drivers/usb/serial/bus.c        |  14 ++-
 drivers/usb/serial/kobil_sct.c  |  22 ++--
 drivers/usb/serial/option.c     |  13 ++-
 drivers/usb/serial/sierra.c     | 198 ++++++++++++++++++++--------------
 drivers/usb/serial/usb-serial.c |  38 +++----
 drivers/usb/serial/usb-wwan.h   |   6 +-
 drivers/usb/serial/usb_wwan.c   | 234 ++++++++++++++++++++--------------------
 drivers/usb/wusbcore/wa-rpipe.c |   4 +-
 10 files changed, 390 insertions(+), 318 deletions(-)

-- 
1.8.5.5