Revert "cdc-acm: implement put_char() and flush_chars()"

Revert "cdc-acm: implement put_char() and flush_chars()"

[Greg Kroah-Hartman]

On Thu, Aug 30, 2018 at 11:18:36AM +0200, Oliver Neukum wrote:
> This reverts commit a81cf9799ad7299b03a4dff020d9685f9ac5f3e0.
> 
> The patch causes a regression, which I cannot find the reason for.
> So let's revert for now, as a revert hurts only performance.
> 
> I was trying to resolve the problem with Oliver but we don't get any
> conclusion
> for 5 months, so I am now sending this to mail list and cdc_acm authors.
> 
> I am using simple request-response protocol to obtain the boiller
> parameters
> in constant intervals.
> 
> A simple one transaction is:
> 1. opening the /dev/ttyACM0
> 2. sending the following 10-bytes request to the device:
>     unsigned char req[] = {0x02, 0xfe, 0x01, 0x05, 0x08, 0x02, 0x01,
> 0x69, 0xab, 0x03};
> 3. reading response (frame of 74 bytes length).
> 4. closing the descriptor
> I am doing this transaction with 5 seconds intervals.
> 
> Before the bad commit everything was working correctly: I've got a
> requests and
> a responses in a timely manner.
> 
> After the bad commit more time I am using the kernel module, more
> problems I have.
> The graph [2] is showing the problem.
> 
> As you can see after module load all seems fine but after about 30
> minutes I've got
> a plenty of EAGAINs when doing read()'s and trying to read back the
> data.
> 
> When I rmmod and insmod the cdc_acm module again, then the situation is
> starting
> over again: running ok shortly after load, and more time it is running,
> more EAGAINs
> I have when calling read().
> 
> As a bonus I can see the problem on the device itself:
> The device is configured as you can see here on this screen [3].
> It has two transmision LEDs: TX and RX. Blink duration is set for 100ms.
> This is a recording before the bad commit when all is working fine: [4]
> And this is with the bad commit: [5]
> As you can see the TX led is blinking wrongly long (indicating
> transmission?)
> and I have problems doing read() calls (EAGAIN).
> 
> Reported-by: manio <manio@skyboo.net>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Fixes: a81cf9799ad7 ("cdc-acm: implement put_char() and flush_chars()")
> Cc: stable <stable@vger.kernel.org>
> ---
>  drivers/usb/class/cdc-acm.c | 92 ---------------------------------------------
>  drivers/usb/class/cdc-acm.h |  1 -
>  2 files changed, 93 deletions(-)

This does not apply to Linus's tree right now, can you refresh it and
resend?

thanks,

greg k-h

Revert "cdc-acm: implement put_char() and flush_chars()"

[Oliver Neukum]

This reverts commit a81cf9799ad7299b03a4dff020d9685f9ac5f3e0.

The patch causes a regression, which I cannot find the reason for.
So let's revert for now, as a revert hurts only performance.

I was trying to resolve the problem with Oliver but we don't get any
conclusion
for 5 months, so I am now sending this to mail list and cdc_acm authors.

I am using simple request-response protocol to obtain the boiller
parameters
in constant intervals.

A simple one transaction is:
1. opening the /dev/ttyACM0
2. sending the following 10-bytes request to the device:
    unsigned char req[] = {0x02, 0xfe, 0x01, 0x05, 0x08, 0x02, 0x01,
0x69, 0xab, 0x03};
3. reading response (frame of 74 bytes length).
4. closing the descriptor
I am doing this transaction with 5 seconds intervals.

Before the bad commit everything was working correctly: I've got a
requests and
a responses in a timely manner.

After the bad commit more time I am using the kernel module, more
problems I have.
The graph [2] is showing the problem.

As you can see after module load all seems fine but after about 30
minutes I've got
a plenty of EAGAINs when doing read()'s and trying to read back the
data.

When I rmmod and insmod the cdc_acm module again, then the situation is
starting
over again: running ok shortly after load, and more time it is running,
more EAGAINs
I have when calling read().

As a bonus I can see the problem on the device itself:
The device is configured as you can see here on this screen [3].
It has two transmision LEDs: TX and RX. Blink duration is set for 100ms.
This is a recording before the bad commit when all is working fine: [4]
And this is with the bad commit: [5]
As you can see the TX led is blinking wrongly long (indicating
transmission?)
and I have problems doing read() calls (EAGAIN).

Reported-by: manio <manio@skyboo.net>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
Fixes: a81cf9799ad7299b03a4dff020d9685f9ac5f3e0
---
 drivers/usb/class/cdc-acm.c | 92 ---------------------------------------------
 drivers/usb/class/cdc-acm.h |  1 -
 2 files changed, 93 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index f95bbdc86578..f9b40a9dc4d3 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -780,20 +780,9 @@ static int acm_tty_write(struct tty_struct *tty,
 	}
 
 	if (acm->susp_count) {
-		if (acm->putbuffer) {
-			/* now to preserve order */
-			usb_anchor_urb(acm->putbuffer->urb, &acm->delayed);
-			acm->putbuffer = NULL;
-		}
 		usb_anchor_urb(wb->urb, &acm->delayed);
 		spin_unlock_irqrestore(&acm->write_lock, flags);
 		return count;
-	} else {
-		if (acm->putbuffer) {
-			/* at this point there is no good way to handle errors */
-			acm_start_wb(acm, acm->putbuffer);
-			acm->putbuffer = NULL;
-		}
 	}
 
 	stat = acm_start_wb(acm, wb);
@@ -804,85 +793,6 @@ static int acm_tty_write(struct tty_struct *tty,
 	return count;
 }
 
-static void acm_tty_flush_chars(struct tty_struct *tty)
-{
-	struct acm *acm = tty->driver_data;
-	struct acm_wb *cur;
-	int err;
-	unsigned long flags;
-
-	spin_lock_irqsave(&acm->write_lock, flags);
-
-	cur = acm->putbuffer;
-	if (!cur) /* nothing to do */
-		goto out;
-
-	acm->putbuffer = NULL;
-	err = usb_autopm_get_interface_async(acm->control);
-	if (err < 0) {
-		cur->use = 0;
-		acm->putbuffer = cur;
-		dev_dbg(&acm->control->dev, "Resumption failure\n");
-		goto out;
-	}
-
-	if (acm->susp_count) {
-		dev_dbg(&acm->control->dev, "Anchored buffer of %u at %p \n", cur->len, cur);
-		usb_anchor_urb(cur->urb, &acm->delayed);
-	} else {
-		dev_dbg(&acm->control->dev, "Writing out buffer of %u at %p \n", cur->len, cur);
-		acm_start_wb(acm, cur);
-	}
-out:
-	spin_unlock_irqrestore(&acm->write_lock, flags);
-	return;
-}
-
-static int acm_tty_put_char(struct tty_struct *tty, unsigned char ch)
-{
-	struct acm *acm = tty->driver_data;
-	struct acm_wb *cur;
-	int wbn;
-	unsigned long flags;
-
-overflow:
-	cur = acm->putbuffer;
-	if (!cur) {
-		spin_lock_irqsave(&acm->write_lock, flags);
-		wbn = acm_wb_alloc(acm);
-		if (wbn >= 0) {
-			cur = &acm->wb[wbn];
-			acm->putbuffer = cur;
-		}
-		spin_unlock_irqrestore(&acm->write_lock, flags);
-		if (!cur) {
-			dev_dbg(&acm->control->dev, "Allocation failed\n");
-			return 0;
-		} else {
-			dev_dbg(&acm->control->dev, "Allocated new buffer %d at %p \n",
-				wbn, cur);
-		}
-	}
-
-	dev_dbg(&acm->control->dev, "Writing character %u at position %d in buffer %p\n",
-		ch, cur->len, cur);
-
-	if (cur->len == acm->writesize) {
-		dev_dbg(&acm->control->dev, "Flush overdue\n");
-		/* this is now the error case */
-		acm_tty_flush_chars(tty);
-		goto overflow;
-	}
-
-	cur->buf[cur->len++] = ch;
-	if (cur->len == acm->writesize) {
-		dev_dbg(&acm->control->dev, "Triggering a flush\n");
-		acm_tty_flush_chars(tty);
-	}
-
-	return 1;
-}
-
 static int acm_tty_write_room(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
@@ -2006,8 +1916,6 @@ static const struct tty_operations acm_ops = {
 	.cleanup =		acm_tty_cleanup,
 	.hangup =		acm_tty_hangup,
 	.write =		acm_tty_write,
-	.put_char =		acm_tty_put_char,
-	.flush_chars =		acm_tty_flush_chars,
 	.write_room =		acm_tty_write_room,
 	.ioctl =		acm_tty_ioctl,
 	.throttle =		acm_tty_throttle,
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index eacc116e83da..ca06b20d7af9 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -96,7 +96,6 @@ struct acm {
 	unsigned long read_urbs_free;
 	struct urb *read_urbs[ACM_NR];
 	struct acm_rb read_buffers[ACM_NR];
-	struct acm_wb *putbuffer;			/* for acm_tty_put_char() */
 	int rx_buflimit;
 	spinlock_t read_lock;
 	u8 *notification_buffer;			/* to reassemble fragmented notifications */