Re: [PATCH 4/5] usb-hcd,usb-console: poll hcd device to force usbconsole writes

[Johan Hovold]

Hi,

On Tue, Mar 16, 2010 at 04:05:45PM -0500, Jason Wessel wrote:
> This patch tries to solve the problem that data is lost because there
> are too many outstanding transmit urb's while trying to execute
> printk's to a console.  The same is true if you try something like
> "echo t > /proc/sysrq-trigger".
> 
> This patch takes the route of forcibly polling the hcd device to drain
> the urb queue in order to initiate the bulk write call backs.  This
> only happens if the device is a usb serial console device that sets
> the max_in_flight_urbs to a non zero value in the serial device
> structure.

Why do you need to use max_in_flight_urbs for this? Shouldn't any usb
serial device be able to use the polling mode? 

Currently the parameter is only used by usb_debug to limit the number of
outstanding urbs for the generic multi-urb write implementation. But now
you're adding a second meaning to the variable and use it as a flag
when you set it to -1 for pl2303 (which uses a fifo-implementation) or
URB_UPPER_LIMIT for ftdi_sio (you could simply have used -1 here as
well). If there are drivers that definitely should not use the polling
mode, it seems to me a new flag such as console_poll (or
no_console_poll) would be more appropriate.

That is, either always poll if a console or if necessary poll only if
serial->type->console_poll is set (or no_console_poll isn't).


There are a few more comments below.


> A few millisecond penalty will get incurred to allow the hcd controller
> to complete a write urb, else the console data is thrown away.
> 
> The max_in_flight_urbs was reduced in the usb_debug driver because it
> is highly desired to push things out to the console in a timely
> fashion and there is no need to have a queue that large for the
> interrupt driven mode of operation when used through the tty
> interface.
> 
> CC: Greg Kroah-Hartman <gregkh@xxxxxxx>
> CC: Alan Cox <alan@xxxxxxxxxxxxxxx>
> CC: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> CC: Oliver Neukum <oliver@xxxxxxxxxx>
> CC: linux-usb@xxxxxxxxxxxxxxx
> Signed-off-by: Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
> ---
>  drivers/usb/core/hcd.c         |   10 +++++++++
>  drivers/usb/serial/console.c   |   42 +++++++++++++++++++++++++--------------
>  drivers/usb/serial/ftdi_sio.c  |    7 +++--
>  drivers/usb/serial/pl2303.c    |    6 +++-
>  drivers/usb/serial/usb_debug.c |    2 +-
>  include/linux/usb.h            |    3 ++
>  6 files changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 2f8cedd..dd710d7 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2271,6 +2271,16 @@ usb_hcd_platform_shutdown(struct platform_device* dev)
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_platform_shutdown);
>  
> +void
> +usb_poll_irq(struct usb_device *udev)
> +{
> +	struct usb_hcd *hcd;
> +
> +	hcd = bus_to_hcd(udev->bus);
> +	usb_hcd_irq(0, hcd);
> +}
> +EXPORT_SYMBOL_GPL(usb_poll_irq);
> +
>  /*-------------------------------------------------------------------------*/
>  
>  #if defined(CONFIG_USB_MON) || defined(CONFIG_USB_MON_MODULE)
> diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
> index 1ee6b2a..b6b96ff 100644
> --- a/drivers/usb/serial/console.c
> +++ b/drivers/usb/serial/console.c
> @@ -197,13 +197,37 @@ static int usb_console_setup(struct console *co, char *options)
>  	return retval;
>  }
>  
> +static void usb_do_console_write(struct usb_serial *serial,
> +				 struct usb_serial_port *port,
> +				 const char *buf, unsigned count)
> +{
> +	int retval;
> +	int loops = 100;
> +try_again:
> +	/* pass on to the driver specific version of this function if
> +	   it is available */
> +	if (serial->type->write)
> +		retval = serial->type->write(NULL, port, buf, count);
> +	else
> +		retval = usb_serial_generic_write(NULL, port, buf, count);
> +	if (retval < count && retval >= 0 &&
> +	    serial->type->max_in_flight_urbs != 0 && loops--) {
> +		/* poll the hcd device because the queue is full */
> +		count -= retval;
> +		buf += retval;
> +		udelay(100);
> +		usb_poll_irq(serial->dev);
> +		goto try_again;
> +	}
> +	dbg("%s - return value : %d", __func__, retval);
> +}
> +
>  static void usb_console_write(struct console *co,
>  					const char *buf, unsigned count)
>  {
>  	static struct usbcons_info *info = &usbcons_info;
>  	struct usb_serial_port *port = info->port;
>  	struct usb_serial *serial;
> -	int retval = -ENODEV;
>  
>  	if (!port || port->serial->dev->state == USB_STATE_NOTATTACHED)
>  		return;
> @@ -230,23 +254,11 @@ static void usb_console_write(struct console *co,
>  				break;
>  			}
>  		}
> -		/* pass on to the driver specific version of this function if
> -		   it is available */
> -		if (serial->type->write)
> -			retval = serial->type->write(NULL, port, buf, i);
> -		else
> -			retval = usb_serial_generic_write(NULL, port, buf, i);
> -		dbg("%s - return value : %d", __func__, retval);
> +		usb_do_console_write(serial, port, buf, i);
>  		if (lf) {
>  			/* append CR after LF */
>  			unsigned char cr = 13;
> -			if (serial->type->write)
> -				retval = serial->type->write(NULL,
> -								port, &cr, 1);
> -			else
> -				retval = usb_serial_generic_write(NULL,
> -								port, &cr, 1);
> -			dbg("%s - return value : %d", __func__, retval);
> +			usb_do_console_write(serial, port, &cr, 1);
>  		}
>  		buf += i;
>  		count -= i;
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 95ec748..c7f559c 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -53,6 +53,9 @@
>  #define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@xxxxxxxxx>, Bill Ryder <bryder@xxxxxxx>, Kuba Ober <kuba@xxxxxxxxxxxxxxx>, Andreas Mohr"
>  #define DRIVER_DESC "USB FTDI Serial Converters Driver"
>  
> +/* number of outstanding urbs to prevent userspace DoS from happening */
> +#define URB_UPPER_LIMIT	42
> +
>  static int debug;
>  static __u16 vendor = FTDI_VID;
>  static __u16 product;
> @@ -838,6 +841,7 @@ static struct usb_serial_driver ftdi_sio_device = {
>  	.ioctl =		ftdi_ioctl,
>  	.set_termios =		ftdi_set_termios,
>  	.break_ctl =		ftdi_break_ctl,
> +	.max_in_flight_urbs =	URB_UPPER_LIMIT,

Here max_in_flight_urbs is simply used as a flag (could have used -1
here as well).

>  };
>  
>  
> @@ -848,9 +852,6 @@ static struct usb_serial_driver ftdi_sio_device = {
>  #define HIGH 1
>  #define LOW 0
>  
> -/* number of outstanding urbs to prevent userspace DoS from happening */
> -#define URB_UPPER_LIMIT	42
> -
>  /*
>   * ***************************************************************************
>   * Utility functions
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index 1891cfb..2615fe1 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -453,8 +453,9 @@ static void pl2303_send(struct usb_serial_port *port)
>  	port->write_urb->transfer_buffer_length = count;
>  	result = usb_submit_urb(port->write_urb, GFP_ATOMIC);
>  	if (result) {
> -		dev_err(&port->dev, "%s - failed submitting write urb,"
> -			" error %d\n", __func__, result);
> +		if (!(port->port.console))
> +			dev_err(&port->dev, "%s - failed submitting write urb,"
> +				" error %d\n", __func__, result);

Why do you treat pl2303 different from all other drivers (including
ftdi_sio and usb_debug) which all report error when submitting an urb
failed? Is this crucial to not get locked up in some recursion?

>  		priv->write_urb_in_use = 0;
>  		/* TODO: reschedule pl2303_send */
>  	}
> @@ -1185,6 +1186,7 @@ static struct usb_serial_driver pl2303_device = {
>  	.chars_in_buffer =	pl2303_chars_in_buffer,
>  	.attach =		pl2303_startup,
>  	.release =		pl2303_release,
> +	.max_in_flight_urbs =	-1,
>  };

Here max_in_flight_urbs is again used as a flag in a way that is
unrelated to its original meaning as pl2303 does not uses the generic
multi-urb writes but rather a custom fifo and a single urb.

>  static int __init pl2303_init(void)
> diff --git a/drivers/usb/serial/usb_debug.c b/drivers/usb/serial/usb_debug.c
> index 252cc2d..4a04552 100644
> --- a/drivers/usb/serial/usb_debug.c
> +++ b/drivers/usb/serial/usb_debug.c
> @@ -15,7 +15,7 @@
>  #include <linux/usb.h>
>  #include <linux/usb/serial.h>
>  
> -#define URB_DEBUG_MAX_IN_FLIGHT_URBS	4000
> +#define URB_DEBUG_MAX_IN_FLIGHT_URBS	42
>  #define USB_DEBUG_MAX_PACKET_SIZE	8
>  #define USB_DEBUG_BRK_SIZE		8
>  static char USB_DEBUG_BRK[USB_DEBUG_BRK_SIZE] = {
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 8c9f053..a7d6cf7 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -569,6 +569,9 @@ static inline void usb_mark_last_busy(struct usb_device *udev)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +/* for polling the hcd device */
> +extern void usb_poll_irq(struct usb_device *udev);
> +
>  /* for drivers using iso endpoints */
>  extern int usb_get_current_frame_number(struct usb_device *usb_dev);

Regards,
Johan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/