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

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.

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: a81cf9799ad7 ("cdc-acm: implement put_char() and flush_chars()")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 27346d69f3938c0222fc59b467bbeb6e9ce778e7..f9b40a9dc4d33eb458d6a3469a0d41acdeab404b 100644 (file)
--- 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,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,

diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index eacc116e83da2ccf38b2640aa5b5b02b5b3621c8..ca06b20d7af9cc9567da1f243ad99935f8bc99e7 100644 (file)
--- 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 */