From patchwork Thu Aug 30 09:18:36 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Oliver Neukum X-Patchwork-Id: 10581387 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4C10E14E1 for ; Thu, 30 Aug 2018 09:26:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3D8632B83A for ; Thu, 30 Aug 2018 09:26:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 314DD2B844; Thu, 30 Aug 2018 09:26:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6FA882B83A for ; Thu, 30 Aug 2018 09:26:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728062AbeH3N2F (ORCPT ); Thu, 30 Aug 2018 09:28:05 -0400 Received: from mx2.suse.de ([195.135.220.15]:47520 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727780AbeH3N2F (ORCPT ); Thu, 30 Aug 2018 09:28:05 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 68F55AEFC; Thu, 30 Aug 2018 09:26:50 +0000 (UTC) From: Oliver Neukum To: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org Cc: Oliver Neukum Subject: [PATCH] Revert "cdc-acm: implement put_char() and flush_chars()" Date: Thu, 30 Aug 2018 11:18:36 +0200 Message-Id: <20180830091836.6215-1-oneukum@suse.com> X-Mailer: git-send-email 2.16.4 Sender: linux-usb-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 Signed-off-by: Oliver Neukum 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 */