Revert "cdc-acm: implement put_char() and flush_chars()" (df3aa13c) · Commits · 戴 / test

This reverts commit . The patch causes a regression, which I cannot find the reason for. So let's revert for now, as a revert hurts only performance. Original report: 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:

Mariusz Bialonczyk <manio@skyboo.net> Signed-off-by:

Oliver Neukum <oneukum@suse.com> Fixes: ("cdc-acm: implement put_char() and flush_chars()") Cc: stable <stable@vger.kernel.org> Signed-off-by:

Greg Kroah-Hartman <gregkh@linuxfoundation.org>

drivers/usb/class/cdc-acm.c

+0 −73

Original line number	Diff line number	Diff line
		@@ -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,66 +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;
		goto out;
		}

		if (acm->susp_count)
		usb_anchor_urb(cur->urb, &acm->delayed);
		else
		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)
		return 0;
		}

		if (cur->len == acm->writesize) {
		acm_tty_flush_chars(tty);
		goto overflow;
		}

		cur->buf[cur->len++] = ch;
		return 1;
		}

		static int acm_tty_write_room(struct tty_struct *tty)
		{
		struct acm *acm = tty->driver_data;
		@@ -1987,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,

drivers/usb/class/cdc-acm.h

+0 −1

Original line number	Diff line number	Diff line
		@@ -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 */

Admin message