From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932654AbcGOLTa (ORCPT ); Fri, 15 Jul 2016 07:19:30 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:35122 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932483AbcGOLTZ (ORCPT ); Fri, 15 Jul 2016 07:19:25 -0400 Date: Fri, 15 Jul 2016 13:19:26 +0200 From: Johan Hovold To: Mathieu OTHACEHE Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 12/36] usb: serial: ti_usb_3410_5052: Use generic read/write callbacks Message-ID: <20160715111926.GH8809@localhost> References: <1463042948-12205-1-git-send-email-m.othacehe@gmail.com> <1463042948-12205-13-git-send-email-m.othacehe@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1463042948-12205-13-git-send-email-m.othacehe@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 12, 2016 at 10:48:44AM +0200, Mathieu OTHACEHE wrote: > Remove read_bulk_callback, write_bulk_callback, write, write_room, > chars_in_buffer, throttle and unthrottle callbacks who uselessly > reimplements generic functions. > > Signed-off-by: Mathieu OTHACEHE > --- > drivers/usb/serial/ti_usb_3410_5052.c | 315 ---------------------------------- > 1 file changed, 315 deletions(-) > > -static void ti_throttle(struct tty_struct *tty) > -{ > - struct usb_serial_port *port = tty->driver_data; > - struct ti_port *tport = usb_get_serial_port_data(port); > - > - if (I_IXOFF(tty) || C_CRTSCTS(tty)) > - ti_stop_read(tport, tty); > - >-} > - > - > -static void ti_unthrottle(struct tty_struct *tty) > -{ > - struct usb_serial_port *port = tty->driver_data; > - struct ti_port *tport = usb_get_serial_port_data(port); > - int status; > - > - if (I_IXOFF(tty) || C_CRTSCTS(tty)) { > - status = ti_restart_read(tport, tty); > - if (status) > - dev_err(&port->dev, "%s - cannot restart read, %d\n", > - __func__, status); > - } > -} > - > static int ti_ioctl(struct tty_struct *tty, > unsigned int cmd, unsigned long arg) > { > @@ -978,8 +866,6 @@ static void ti_set_termios(struct tty_struct *tty, > if ((C_BAUD(tty)) != B0) > config->wFlags |= TI_UART_ENABLE_RTS_IN; > config->wFlags |= TI_UART_ENABLE_CTS_OUT; > - } else { > - ti_restart_read(tport, tty); > } > > if (I_IXOFF(tty) || I_IXON(tty)) { > @@ -988,8 +874,6 @@ static void ti_set_termios(struct tty_struct *tty, > > if (I_IXOFF(tty)) > config->wFlags |= TI_UART_ENABLE_X_IN; > - else > - ti_restart_read(tport, tty); > > if (I_IXON(tty)) > config->wFlags |= TI_UART_ENABLE_X_OUT; > @@ -1193,168 +1077,6 @@ exit: > __func__, retval); > } The interactions with software flow control here needs to be looked at more closely, as the generic implementation ignores them. > > - > -static void ti_bulk_in_callback(struct urb *urb) > -{ > - struct ti_port *tport = urb->context; > - struct usb_serial_port *port = tport->tp_port; > - struct device *dev = &urb->dev->dev; > - int status = urb->status; > - int retval = 0; > - > - switch (status) { > - case 0: > - break; > - case -ECONNRESET: > - case -ENOENT: > - case -ESHUTDOWN: > - dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status); > - tport->tp_tdev->td_urb_error = 1; > - return; > - default: > - dev_err(dev, "%s - nonzero urb status, %d\n", > - __func__, status); > - tport->tp_tdev->td_urb_error = 1; > - } > - > - if (status == -EPIPE) > - goto exit; > - > - if (status) { > - dev_err(dev, "%s - stopping read!\n", __func__); > - return; > - } > - > - if (urb->actual_length) { > - usb_serial_debug_data(dev, __func__, urb->actual_length, > - urb->transfer_buffer); > - > - if (!tport->tp_is_open) > - dev_dbg(dev, "%s - port closed, dropping data\n", > - __func__); > - else > - ti_recv(port, urb->transfer_buffer, urb->actual_length); > - spin_lock(&tport->tp_lock); > - port->icount.rx += urb->actual_length; icount.tx/rx is not updated by the generic implementations either (there are a few reasons why this driver has not simply been converted to use the generic implementation already). A bit too much is going on here at once, and we risk introducing regression such as the issues raised above. Please at least try to do the conversion in two steps for the rx and tx paths. Thanks, Johan