linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Oliver Neukum <oneukum@suse.com>
To: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
Cc: Oliver Neukum <oneukum@suse.com>
Subject: Revert "cdc-acm: implement put_char() and flush_chars()"
Date: Thu, 30 Aug 2018 11:18:36 +0200	[thread overview]
Message-ID: <20180830091836.6215-1-oneukum@suse.com> (raw)

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

             reply	other threads:[~2018-08-30  9:18 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-30  9:18 Oliver Neukum [this message]
  -- strict thread matches above, loose matches on Subject: below --
2018-09-05 12:49 Revert "cdc-acm: implement put_char() and flush_chars()" Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180830091836.6215-1-oneukum@suse.com \
    --to=oneukum@suse.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-usb@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).