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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
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 */
-- 
2.16.4




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux