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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160715111926.GH8809@localhost>
Date:	Fri, 15 Jul 2016 13:19:26 +0200
From:	Johan Hovold <johan@...nel.org>
To:	Mathieu OTHACEHE <m.othacehe@...il.com>
Cc:	johan@...nel.org, gregkh@...uxfoundation.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/36] usb: serial: ti_usb_3410_5052: Use generic
 read/write callbacks

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@...il.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