Re: [PATCH 12/36] usb: serial: ti_usb_3410_5052: Use generic read/write callbacks

From: Johan Hovold <johan@kernel.org>
To: Mathieu OTHACEHE <m.othacehe@gmail.com>
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
Date: Fri, 15 Jul 2016 13:19:26 +0200	[thread overview]
Message-ID: <20160715111926.GH8809@localhost> (raw)
In-Reply-To: <1463042948-12205-13-git-send-email-m.othacehe@gmail.com>

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 <m.othacehe@gmail.com>
> ---
>  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