[PATCH] Airprime driver improvements to allow full speed EvDO transfers

[PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

Adapted from an earlier patch by Greg KH <gregkh@suse.de>.
That patch added multiple read urbs and larger transfer buffers to allow
data transfers at full EvDO speed.

This version includes additional device IDs and fixes a memory leak in
the transfer buffer allocation.

Some (maybe all?) of the supported devices present multiple bulk endpoints,
the additional EPs can be used for control and status functions.
This version allocates 3 EPs by default, that can be changed using
the 'endpoints' module parameter.

Tested with Sierra Wireless EM5625 and MC5720 embedded modules.

Device ID (0x0c88, 0x17da) for the Kyocera Wireless KPC650/Passport
was added but is not yet tested.

Signed-off-by: Andy Gay <andy@andynet.net>

---
commit 3d1346863aac4b3c016acb409a3b9e6651af8f7a
tree f4359718b8550ce0d95b57ba1b5b0d902bf2ada8
parent 501b7c77de3e90519e95fd99e923bf9a29cd120d
author andy <andy@tahini.andynet.net> Fri, 30 Jun 2006 01:27:13 -0400
committer andy <andy@tahini.andynet.net> Fri, 30 Jun 2006 01:27:13 -0400

 drivers/usb/serial/airprime.c |  227 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 222 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/airprime.c b/drivers/usb/serial/airprime.c
index 94b9ba0..5a18f77 100644
--- a/drivers/usb/serial/airprime.c
+++ b/drivers/usb/serial/airprime.c
@@ -1,7 +1,7 @@
 /*
  * AirPrime CDMA Wireless Serial USB driver
  *
- * Copyright (C) 2005 Greg Kroah-Hartman <gregkh@suse.de>
+ * Copyright (C) 2005-2006 Greg Kroah-Hartman <gregkh@suse.de>
  *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License version
@@ -11,26 +11,232 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/tty.h>
+#include <linux/tty_flip.h>
 #include <linux/module.h>
 #include <linux/usb.h>
 #include "usb-serial.h"
 
 static struct usb_device_id id_table [] = {
 	{ USB_DEVICE(0x0c88, 0x17da) }, /* Kyocera Wireless KPC650/Passport */
-	{ USB_DEVICE(0xf3d, 0x0112) },  /* AirPrime CDMA Wireless PC Card */
+	{ USB_DEVICE(0x0f3d, 0x0112) },	/* AirPrime CDMA Wireless PC Card */
 	{ USB_DEVICE(0x1410, 0x1110) }, /* Novatel Wireless Merlin CDMA */
+	{ USB_DEVICE(0x1199, 0x0017) }, /* Sierra Wireless EM5625 */
+	{ USB_DEVICE(0x1199, 0x0018) }, /* Sierra Wireless MC5720 */
 	{ USB_DEVICE(0x1199, 0x0112) }, /* Sierra Wireless Aircard 580 */
-	{ USB_DEVICE(0x1199, 0x0218) }, /* Sierra Wireless MC5720 */
 	{ },
 };
 MODULE_DEVICE_TABLE(usb, id_table);
+#define URB_TRANSFER_BUFFER_SIZE	4096
+#define NUM_READ_URBS			4
+#define NUM_WRITE_URBS			4
+#define NUM_BULK_EPS			3
+#define MAX_BULK_EPS			6
+
+/* if overridden by the user, then use their value for the size of the
+ * read and write urbs, and the number of endpoints */
+static int buffer_size = URB_TRANSFER_BUFFER_SIZE;
+static int endpoints = NUM_BULK_EPS;
+static int debug;
+struct airprime_private {
+	spinlock_t lock;
+	int outstanding_urbs;
+	int throttled;
+	struct urb *read_urbp[NUM_READ_URBS];
+};
+
+static void airprime_read_bulk_callback(struct urb *urb, struct pt_regs *regs)
+{
+	struct usb_serial_port *port = urb->context;
+	unsigned char *data = urb->transfer_buffer;
+	struct tty_struct *tty;
+	int result;
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	if (urb->status) {
+		dbg("%s - nonzero read bulk status received: %d",
+		    __FUNCTION__, urb->status);
+		/* something happened, so free up the memory for this urb /*
+		if (urb->transfer_buffer) {
+			kfree (urb->transfer_buffer);
+			urb->transfer_buffer = NULL;
+		}
+		return;
+	}
+	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, urb->actual_length, data);
+
+	tty = port->tty;
+	if (tty && urb->actual_length) {
+		tty_buffer_request_room(tty, urb->actual_length);
+		tty_insert_flip_string(tty, data, urb->actual_length);
+		tty_flip_buffer_push(tty);
+	}
+	/* should this use GFP_KERNEL? */
+	result = usb_submit_urb(urb, GFP_ATOMIC);
+	if (result)
+		dev_err(&port->dev, "%s - failed resubmitting read urb, error %d\n",
+			__FUNCTION__, result);
+	return;
+}
+
+static void airprime_write_bulk_callback(struct urb *urb, struct pt_regs *regs)
+{
+	struct usb_serial_port *port = urb->context;
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+
+	/* free up the transfer buffer, as usb_free_urb() does not do this */
+	kfree (urb->transfer_buffer);
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	if (urb->status)
+		dbg("%s - nonzero write bulk status received: %d",
+		    __FUNCTION__, urb->status);
+	spin_lock_irqsave(&priv->lock, flags);
+	--priv->outstanding_urbs;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	usb_serial_port_softint(port);
+}
+
+static int airprime_open(struct usb_serial_port *port, struct file *filp)
+{
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	struct usb_serial *serial = port->serial;
+	struct urb *urb;
+	char *buffer;
+	int i;
+	int result = 0;
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	/* initialize our private data structure if it isn't already created */
+	if (!priv) {
+		priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+		if (!priv)
+			return -ENOMEM;
+		spin_lock_init(&priv->lock);
+		usb_set_serial_port_data(port, priv);
+	}
+	/* TODO handle error conditions better, right now we leak memory */
+	for (i = 0; i < NUM_READ_URBS; ++i) {
+		buffer = kmalloc(buffer_size, GFP_KERNEL);
+		if (!buffer) {
+			dev_err(&port->dev, "%s - out of memory.\n",
+				__FUNCTION__);
+			return -ENOMEM;
+		}
+		urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!urb) {
+			dev_err(&port->dev, "%s - no more urbs?\n",
+				__FUNCTION__);
+			return -ENOMEM;
+		}
+		usb_fill_bulk_urb(urb, serial->dev,
+				  usb_rcvbulkpipe(serial->dev,
+						  port->bulk_out_endpointAddress),
+				  buffer, buffer_size,
+				  airprime_read_bulk_callback, port);
+		result = usb_submit_urb(urb, GFP_KERNEL);
+		if (result) {
+			dev_err(&port->dev,
+				"%s - failed submitting read urb %d for port %d, error %d\n",
+				__FUNCTION__, i, port->number, result);
+			return result;
+		}
+		/* fun with reference counting, when this urb is finished, the
+		 * host driver will free it up automatically */
+		/* don't do this here, we need the urb to stay around until the close
+		   function can take care of it */
+		//usb_free_urb (urb);
+		/* instead remember this urb so we can kill it when the
+		   port is closed */
+		priv->read_urbp[i] = urb;
+	}
+	return result;
+}
+
+static void airprime_close(struct usb_serial_port *port, struct file * filp)
+{
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	int i;
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	/* killing the urb will invoke read_bulk_callback() with an error status,
+	   so the transfer buffer will be freed there */
+	for (i = 0; i < NUM_READ_URBS; ++i) {
+		usb_kill_urb (priv->read_urbp[i]);
+		usb_free_urb (priv->read_urbp[i]);
+	}
+
+	/* free up private structure? */
+}
+
+static int airprime_write(struct usb_serial_port *port,
+			  const unsigned char *buf, int count)
+{
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	struct usb_serial *serial = port->serial;
+	struct urb *urb;
+	unsigned char *buffer;
+	unsigned long flags;
+	int status;
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (priv->outstanding_urbs > NUM_WRITE_URBS) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		dbg("%s - write limit hit\n", __FUNCTION__);
+		return 0;
+	}
+	spin_unlock_irqrestore(&priv->lock, flags);
+	buffer = kmalloc(count, GFP_ATOMIC);
+	if (!buffer) {
+		dev_err(&port->dev, "out of memory\n");
+		return -ENOMEM;
+	}
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		dev_err(&port->dev, "no more free urbs\n");
+		kfree (buffer);
+		return -ENOMEM;
+	}
+	memcpy (buffer, buf, count);
+
+	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count, buffer);
+
+	usb_fill_bulk_urb(urb, serial->dev,
+			  usb_sndbulkpipe(serial->dev,
+					  port->bulk_out_endpointAddress),
+			  buffer, count,
+			  airprime_write_bulk_callback, port);
+
+	/* send it down the pipe */
+	status = usb_submit_urb(urb, GFP_ATOMIC);
+	if (status) {
+		dev_err(&port->dev,
+			"%s - usb_submit_urb(write bulk) failed with status = %d\n",
+			__FUNCTION__, status);
+		count = status;
+		kfree (buffer);
+	} else {
+		spin_lock_irqsave(&priv->lock, flags);
+		++priv->outstanding_urbs;
+		spin_unlock_irqrestore(&priv->lock, flags);
+	}
+	/* we are done with this urb, so let the host driver
+	 * really free it when it is finished with it */
+	usb_free_urb (urb);
+	return count;
+}
 
 static struct usb_driver airprime_driver = {
 	.name =		"airprime",
 	.probe =	usb_serial_probe,
 	.disconnect =	usb_serial_disconnect,
 	.id_table =	id_table,
-	.no_dynamic_id = 	1,
+	.no_dynamic_id =	1,
 };
 
 static struct usb_serial_driver airprime_device = {
@@ -42,13 +248,17 @@ static struct usb_serial_driver airprime
 	.num_interrupt_in =	NUM_DONT_CARE,
 	.num_bulk_in =		NUM_DONT_CARE,
 	.num_bulk_out =		NUM_DONT_CARE,
-	.num_ports =		1,
+	.open =			airprime_open,
+	.close =		airprime_close,
+	.write =		airprime_write,
 };
 
 static int __init airprime_init(void)
 {
 	int retval;
 
+	airprime_device.num_ports =
+		(endpoints > 0 && endpoints <= MAX_BULK_EPS) ? endpoints : NUM_BULK_EPS;
 	retval = usb_serial_register(&airprime_device);
 	if (retval)
 		return retval;
@@ -67,3 +277,10 @@ static void __exit airprime_exit(void)
 module_init(airprime_init);
 module_exit(airprime_exit);
 MODULE_LICENSE("GPL");
+
+module_param(debug, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(debug, "Debug enabled or not");
+module_param(buffer_size, int, 0);
+MODULE_PARM_DESC(buffer_size, "Size of the transfer buffers");
+module_param(endpoints, int, 0);
+MODULE_PARM_DESC(endpoints, "Number of bulk EPs to configure (default 3)");

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andrew Morton]

On Fri, 30 Jun 2006 01:48:02 -0400
Andy Gay <andy@andynet.net> wrote:

> 
> Adapted from an earlier patch by Greg KH <gregkh@suse.de>.
> That patch added multiple read urbs and larger transfer buffers to allow
> data transfers at full EvDO speed.
> 
> This version includes additional device IDs and fixes a memory leak in
> the transfer buffer allocation.
> 
> Some (maybe all?) of the supported devices present multiple bulk endpoints,
> the additional EPs can be used for control and status functions.
> This version allocates 3 EPs by default, that can be changed using
> the 'endpoints' module parameter.
> 
> Tested with Sierra Wireless EM5625 and MC5720 embedded modules.
> 
> Device ID (0x0c88, 0x17da) for the Kyocera Wireless KPC650/Passport
> was added but is not yet tested.
> 
> ...
>
> +static void airprime_read_bulk_callback(struct urb *urb, struct pt_regs *regs)
> +{
> +	struct usb_serial_port *port = urb->context;
> +	unsigned char *data = urb->transfer_buffer;
> +	struct tty_struct *tty;
> +	int result;
> +
> +	dbg("%s - port %d", __FUNCTION__, port->number);
> +
> +	if (urb->status) {
> +		dbg("%s - nonzero read bulk status received: %d",
> +		    __FUNCTION__, urb->status);
> +		/* something happened, so free up the memory for this urb /*
> +		if (urb->transfer_buffer) {
> +			kfree (urb->transfer_buffer);
> +			urb->transfer_buffer = NULL;
> +		}
> +		return;
> +	}
> +	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, urb->actual_length, data);
> +
> +	tty = port->tty;
> +	if (tty && urb->actual_length) {
> +		tty_buffer_request_room(tty, urb->actual_length);
> +		tty_insert_flip_string(tty, data, urb->actual_length);

Is it correct to ignore the return value from those two functions?

> +		tty_flip_buffer_push(tty);
> +	}
> +	/* should this use GFP_KERNEL? */
> +	result = usb_submit_urb(urb, GFP_ATOMIC);

If possible, yep.

> ...
>
> +static int airprime_open(struct usb_serial_port *port, struct file *filp)
> +{
> +	struct airprime_private *priv = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	struct urb *urb;
> +	char *buffer;
> +	int i;
> +	int result = 0;
> +
> +	dbg("%s - port %d", __FUNCTION__, port->number);
> +
> +	/* initialize our private data structure if it isn't already created */
> +	if (!priv) {
> +		priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +		if (!priv)
> +			return -ENOMEM;
> +		spin_lock_init(&priv->lock);
> +		usb_set_serial_port_data(port, priv);
> +	}
> +	/* TODO handle error conditions better, right now we leak memory */
> +	for (i = 0; i < NUM_READ_URBS; ++i) {
> +		buffer = kmalloc(buffer_size, GFP_KERNEL);
> +		if (!buffer) {
> +			dev_err(&port->dev, "%s - out of memory.\n",
> +				__FUNCTION__);
> +			return -ENOMEM;
> +		}
> +		urb = usb_alloc_urb(0, GFP_KERNEL);
> +		if (!urb) {
> +			dev_err(&port->dev, "%s - no more urbs?\n",
> +				__FUNCTION__);
> +			return -ENOMEM;
> +		}
> +		usb_fill_bulk_urb(urb, serial->dev,
> +				  usb_rcvbulkpipe(serial->dev,
> +						  port->bulk_out_endpointAddress),
> +				  buffer, buffer_size,
> +				  airprime_read_bulk_callback, port);
> +		result = usb_submit_urb(urb, GFP_KERNEL);
> +		if (result) {
> +			dev_err(&port->dev,
> +				"%s - failed submitting read urb %d for port %d, error %d\n",
> +				__FUNCTION__, i, port->number, result);
> +			return result;
> +		}
> +		/* fun with reference counting, when this urb is finished, the
> +		 * host driver will free it up automatically */
> +		/* don't do this here, we need the urb to stay around until the close
> +		   function can take care of it */
> +		//usb_free_urb (urb);
> +		/* instead remember this urb so we can kill it when the
> +		   port is closed */
> +		priv->read_urbp[i] = urb;
> +	}
> +	return result;
> +}
> +

This function leaks memory all over the place if something goes wrong.

Please redesign it to have a single `return' statement.  You'll find that'll
help avoid leaks now and during any later enhancements.


> +{
> +	struct airprime_private *priv = usb_get_serial_port_data(port);
> +	int i;
> +
> +	dbg("%s - port %d", __FUNCTION__, port->number);
> +
> +	/* killing the urb will invoke read_bulk_callback() with an error status,
> +	   so the transfer buffer will be freed there */
> +	for (i = 0; i < NUM_READ_URBS; ++i) {
> +		usb_kill_urb (priv->read_urbp[i]);
> +		usb_free_urb (priv->read_urbp[i]);
> +	}
> +
> +	/* free up private structure? */

Yes please ;)

> +}
> +
> +static int airprime_write(struct usb_serial_port *port,
> +			  const unsigned char *buf, int count)
> +{
> +	struct airprime_private *priv = usb_get_serial_port_data(port);
> +	struct usb_serial *serial = port->serial;
> +	struct urb *urb;
> +	unsigned char *buffer;
> +	unsigned long flags;
> +	int status;
> +	dbg("%s - port %d", __FUNCTION__, port->number);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	if (priv->outstanding_urbs > NUM_WRITE_URBS) {
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +		dbg("%s - write limit hit\n", __FUNCTION__);
> +		return 0;
> +	}
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +	buffer = kmalloc(count, GFP_ATOMIC);
> +	if (!buffer) {
> +		dev_err(&port->dev, "out of memory\n");
> +		return -ENOMEM;
> +	}
> +	urb = usb_alloc_urb(0, GFP_ATOMIC);
> +	if (!urb) {
> +		dev_err(&port->dev, "no more free urbs\n");
> +		kfree (buffer);
> +		return -ENOMEM;
> +	}
> +	memcpy (buffer, buf, count);
> +
> +	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count, buffer);
> +
> +	usb_fill_bulk_urb(urb, serial->dev,
> +			  usb_sndbulkpipe(serial->dev,
> +					  port->bulk_out_endpointAddress),
> +			  buffer, count,
> +			  airprime_write_bulk_callback, port);
> +
> +	/* send it down the pipe */
> +	status = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (status) {
> +		dev_err(&port->dev,
> +			"%s - usb_submit_urb(write bulk) failed with status = %d\n",
> +			__FUNCTION__, status);
> +		count = status;
> +		kfree (buffer);
> +	} else {
> +		spin_lock_irqsave(&priv->lock, flags);
> +		++priv->outstanding_urbs;
> +		spin_unlock_irqrestore(&priv->lock, flags);
> +	}
> +	/* we are done with this urb, so let the host driver
> +	 * really free it when it is finished with it */
> +	usb_free_urb (urb);
> +	return count;
> +}

Is usb_serial_driver.write() really called in a context in which it is
forced to use GFP_ATOMIC?

Again, implementing this function as single-exit would make it easier to
maintain.

> +MODULE_PARM_DESC(debug, "Debug enabled or not");

Just "Debug enabled".

> +module_param(buffer_size, int, 0);
> +MODULE_PARM_DESC(buffer_size, "Size of the transfer buffers");

Units?

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Pete Zaitcev]

On Fri, 30 Jun 2006 00:10:21 -0700, Andrew Morton <akpm@osdl.org> wrote:

> > +static void airprime_read_bulk_callback(struct urb *urb, struct pt_regs *regs)
>.......
> > +	/* should this use GFP_KERNEL? */
> > +	result = usb_submit_urb(urb, GFP_ATOMIC);
> 
> If possible, yep.

You can't be serious. It's a callback function we're discussing here,
and you even quoted it.

> > +	/* free up private structure? */
> 
> Yes please ;)

+1

> Is usb_serial_driver.write() really called in a context in which it is
> forced to use GFP_ATOMIC?

There are cases when it is. It happens when a line discipline does it.
The n_tty does it if the line is in cooked mode, which is the default.
n_hdlc does it always, though I have no idea if this is applicable
to airprime. I think PPP writes from a tasklet as well.

The idea to allocate a URB for every little user write bothers me as
well. It was a dirty code thrown together quickly by someone who could
not be bothered to use a circular buffer and two URBs. It was fine
for the visor.c, but the Airprime is a higher performance card, and it
can be used in a home gateway with a low-power CPU. I'm not happy.

-- Pete

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Andrew Morton <akpm@osdl.org> writes:
> On Fri, 30 Jun 2006 01:48:02 -0400
> Andy Gay <andy@andynet.net> wrote:
[...]
>> +	if (tty && urb->actual_length) {
>> +		tty_buffer_request_room(tty, urb->actual_length);
>> +		tty_insert_flip_string(tty, data, urb->actual_length);
>
> Is it correct to ignore the return value from those two functions?

In fact, according to Alan Cox answer, the first call is useless here at
all, i.e., tty_buffer_request_room() is for subsequent
tty_insert_flip_char() calls in a loop, not for
tty_insert_flip_string(). tty_insert_flip_string() calls
tty_buffer_request_room() itself, and does it in a loop in attempt to
find as much memory as possible.

tty_insert_flip_string() returns number of bytes it has actually
inserted, but I don't believe one can do much if it returns less than
has been requested as it means that we are out of kernel memory.

Overall, it seems it should be just:

+	if (tty && urb->actual_length) {
+		tty_insert_flip_string(tty, data, urb->actual_length);

-- 
Sergei.

Re: [linux-usb-devel] [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Arjan van de Ven]

> > tty_insert_flip_string() returns number of bytes it has actually
> > inserted, but I don't believe one can do much if it returns less than
> > has been requested as it means that we are out of kernel memory.
> 
> Yes. I've been wondering if we should log the failure case somewhere,
> either as a tty-> object or printk.

printk gets... interesting if you use serial console ;)

both locking and buffer space wise

Re: [linux-usb-devel] [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Alan Cox]

Ar Gwe, 2006-06-30 am 14:51 +0400, ysgrifennodd Sergei Organov:
> In fact, according to Alan Cox answer, the first call is useless here at
> all, i.e., tty_buffer_request_room() is for subsequent
> tty_insert_flip_char() calls in a loop, not for
> tty_insert_flip_string(). tty_insert_flip_string() calls
> tty_buffer_request_room() itself, and does it in a loop in attempt to
> find as much memory as possible.

Yep. Think of it as a hint that "I'm about to stuff xyz bytes into
memory" to get best memory efficiency.

> tty_insert_flip_string() returns number of bytes it has actually
> inserted, but I don't believe one can do much if it returns less than
> has been requested as it means that we are out of kernel memory.

Yes. I've been wondering if we should log the failure case somewhere,
either as a tty-> object or printk.

Alan

Re: [linux-usb-devel] [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Alan Cox]

Ar Gwe, 2006-06-30 am 14:02 +0200, ysgrifennodd Arjan van de Ven:
> > Yes. I've been wondering if we should log the failure case somewhere,
> > either as a tty-> object or printk.
> 
> printk gets... interesting if you use serial console ;)
> both locking and buffer space wise

Not particularly. This is on the receive path not the transmit path so
the locking considerations don't arise.

Alan

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Fri, 2006-06-30 at 00:10 -0700, Andrew Morton wrote:

> > 
> > ...
> >
> > +static void airprime_read_bulk_callback(struct urb *urb, struct pt_regs *regs)
> > +{
...

> > +	tty = port->tty;
> > +	if (tty && urb->actual_length) {
> > +		tty_buffer_request_room(tty, urb->actual_length);
> > +		tty_insert_flip_string(tty, data, urb->actual_length);
> 
> Is it correct to ignore the return value from those two functions?
Not my code :) - generic.c and several other drivers do the same
thing... Not that that's an excuse, of course :)
Actually though, I think it's OK to ignore the tty_insert_flip_string
result. These adapters are used at layer 1 for ppp connection, the
higher layers will attempt to recover.

I will remove the tty_buffer_request_room() call though, as suggested by
Sergei Organov and Alan Cox.

> 
> > +		tty_flip_buffer_push(tty);
> > +	}
> > +	/* should this use GFP_KERNEL? */
> > +	result = usb_submit_urb(urb, GFP_ATOMIC);
> 
> If possible, yep.
Oops - that was a comment to myself I left in by mistake. As pointed out
by Pete Zaitcev, this is a callback function. I think it has to be
GFP_ATOMIC here, doesn't it?

> 
> > ...
> >
> > +static int airprime_open(struct usb_serial_port *port, struct file *filp)
> > +{
> > +	struct airprime_private *priv = usb_get_serial_port_data(port);
> > +	struct usb_serial *serial = port->serial;
> > +	struct urb *urb;
> > +	char *buffer;
> > +	int i;
> > +	int result = 0;
> > +
> > +	dbg("%s - port %d", __FUNCTION__, port->number);
> > +
> > +	/* initialize our private data structure if it isn't already created */
> > +	if (!priv) {
> > +		priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +		if (!priv)
> > +			return -ENOMEM;
> > +		spin_lock_init(&priv->lock);
> > +		usb_set_serial_port_data(port, priv);
> > +	}
> > +	/* TODO handle error conditions better, right now we leak memory */
> > +	for (i = 0; i < NUM_READ_URBS; ++i) {
> > +		buffer = kmalloc(buffer_size, GFP_KERNEL);
> > +		if (!buffer) {
> > +			dev_err(&port->dev, "%s - out of memory.\n",
> > +				__FUNCTION__);
> > +			return -ENOMEM;
> > +		}
> > +		urb = usb_alloc_urb(0, GFP_KERNEL);
> > +		if (!urb) {
> > +			dev_err(&port->dev, "%s - no more urbs?\n",
> > +				__FUNCTION__);
> > +			return -ENOMEM;
> > +		}
> > +		usb_fill_bulk_urb(urb, serial->dev,
> > +				  usb_rcvbulkpipe(serial->dev,
> > +						  port->bulk_out_endpointAddress),
> > +				  buffer, buffer_size,
> > +				  airprime_read_bulk_callback, port);
> > +		result = usb_submit_urb(urb, GFP_KERNEL);
> > +		if (result) {
> > +			dev_err(&port->dev,
> > +				"%s - failed submitting read urb %d for port %d, error %d\n",
> > +				__FUNCTION__, i, port->number, result);
> > +			return result;
> > +		}
> > +		/* fun with reference counting, when this urb is finished, the
> > +		 * host driver will free it up automatically */
> > +		/* don't do this here, we need the urb to stay around until the close
> > +		   function can take care of it */
> > +		//usb_free_urb (urb);
> > +		/* instead remember this urb so we can kill it when the
> > +		   port is closed */
> > +		priv->read_urbp[i] = urb;
> > +	}
> > +	return result;
> > +}
> > +
> 
> This function leaks memory all over the place if something goes wrong.
Indeed. Hence the TODO comment.

> 
> Please redesign it to have a single `return' statement.  You'll find that'll
> help avoid leaks now and during any later enhancements.
OK, I'll work on that.
> 
> 
> > +{
> > +	struct airprime_private *priv = usb_get_serial_port_data(port);
> > +	int i;
> > +
> > +	dbg("%s - port %d", __FUNCTION__, port->number);
> > +
> > +	/* killing the urb will invoke read_bulk_callback() with an error status,
> > +	   so the transfer buffer will be freed there */
> > +	for (i = 0; i < NUM_READ_URBS; ++i) {
> > +		usb_kill_urb (priv->read_urbp[i]);
> > +		usb_free_urb (priv->read_urbp[i]);
> > +	}
> > +
> > +	/* free up private structure? */
> 
> Yes please ;)
Easily done. But we'd need another one next time the port is opened. Why
not just allocate it once and keep reusing it?

> 
> > +}
> > +
> > +static int airprime_write(struct usb_serial_port *port,
> > +			  const unsigned char *buf, int count)
> > +{
> > +	struct airprime_private *priv = usb_get_serial_port_data(port);
> > +	struct usb_serial *serial = port->serial;
> > +	struct urb *urb;
> > +	unsigned char *buffer;
> > +	unsigned long flags;
> > +	int status;
> > +	dbg("%s - port %d", __FUNCTION__, port->number);
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	if (priv->outstanding_urbs > NUM_WRITE_URBS) {
> > +		spin_unlock_irqrestore(&priv->lock, flags);
> > +		dbg("%s - write limit hit\n", __FUNCTION__);
> > +		return 0;
> > +	}
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +	buffer = kmalloc(count, GFP_ATOMIC);
> > +	if (!buffer) {
> > +		dev_err(&port->dev, "out of memory\n");
> > +		return -ENOMEM;
> > +	}
> > +	urb = usb_alloc_urb(0, GFP_ATOMIC);
> > +	if (!urb) {
> > +		dev_err(&port->dev, "no more free urbs\n");
> > +		kfree (buffer);
> > +		return -ENOMEM;
> > +	}
> > +	memcpy (buffer, buf, count);
> > +
> > +	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count, buffer);
> > +
> > +	usb_fill_bulk_urb(urb, serial->dev,
> > +			  usb_sndbulkpipe(serial->dev,
> > +					  port->bulk_out_endpointAddress),
> > +			  buffer, count,
> > +			  airprime_write_bulk_callback, port);
> > +
> > +	/* send it down the pipe */
> > +	status = usb_submit_urb(urb, GFP_ATOMIC);
> > +	if (status) {
> > +		dev_err(&port->dev,
> > +			"%s - usb_submit_urb(write bulk) failed with status = %d\n",
> > +			__FUNCTION__, status);
> > +		count = status;
> > +		kfree (buffer);
> > +	} else {
> > +		spin_lock_irqsave(&priv->lock, flags);
> > +		++priv->outstanding_urbs;
> > +		spin_unlock_irqrestore(&priv->lock, flags);
> > +	}
> > +	/* we are done with this urb, so let the host driver
> > +	 * really free it when it is finished with it */
> > +	usb_free_urb (urb);
> > +	return count;
> > +}
> 
> Is usb_serial_driver.write() really called in a context in which it is
> forced to use GFP_ATOMIC?
No idea. Safer to leave this as is, I think.

> 
> Again, implementing this function as single-exit would make it easier to
> maintain.
OK.

> 
> > +MODULE_PARM_DESC(debug, "Debug enabled or not");
> 
> Just "Debug enabled".
> 
> > +module_param(buffer_size, int, 0);
> > +MODULE_PARM_DESC(buffer_size, "Size of the transfer buffers");
> 
> Units?
> 
I'll fix these.
Thanks for reviewing this.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Fri, 2006-06-30 at 01:52 -0700, Pete Zaitcev wrote:

> The idea to allocate a URB for every little user write bothers me as
> well. It was a dirty code thrown together quickly by someone who could
> not be bothered to use a circular buffer and two URBs. It was fine
> for the visor.c, but the Airprime is a higher performance card, and it
> can be used in a home gateway with a low-power CPU. I'm not happy.

I agree completely. I was already planning to work on improving the
write path at some point.
The read path is more important though, these networks are asymmetric -
Sierra Wireless specifies max 2.4Mbps download vs 153Kbps upload for the
EM5625 and MC5720, presumably the other devices are similar. So this
patch was focused on getting the best read performance.

> 
> -- Pete

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Roland Dreier]

 > +		/* something happened, so free up the memory for this urb /*

an obvious glitch here at the end of the line...

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Fri, 2006-06-30 at 13:04 -0700, Roland Dreier wrote:
>  > +		/* something happened, so free up the memory for this urb /*
> 
> an obvious glitch here at the end of the line...
Oops. Sorry 'bout that.. that comment had some more stuff that no longer
applies, I edited it just before I submitted the patch.

I'll make sure to at least compile test before submitting in future....

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Roland Dreier]

this works well on my kyocera kpc650 -- throughput is up to about 1
mbit/sec vs. ~250 kbit/sec with the stock airprime driver.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Sun, 2006-07-02 at 11:48 -0700, Roland Dreier wrote:
> this works well on my kyocera kpc650 -- throughput is up to about 1
> mbit/sec vs. ~250 kbit/sec with the stock airprime driver.
> -
Thanks for the feedback.

I'm working on fixing the concerns Andrew Morton expressed regarding
memory leaks in the open function. I'll send an updated driver soon.

BTW - Jeremy suggested that the number of EPs to configure should be
determined from the device ID. Makes sense to me, but then many users
may have no use for the additional EPs. Alternatively, Greg suggested
that maybe this should split into 2 drivers. Any preferences, anyone?

I don't know which of these devices present multiple EPs though. Can you
send me the appropriate section from 'cat /proc/bus/usb/devices'?

Anyone with an actual Airprime (ID 0xf3d, 0x0112) or Novatel Merlin
(0x1410, 0x1110) please do the same.

Thanks -
Andy

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Roland Dreier]

 > I don't know which of these devices present multiple EPs though. Can you
 > send me the appropriate section from 'cat /proc/bus/usb/devices'?

Sure, no problem:

T:  Bus=05 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12  MxCh= 0
D:  Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
P:  Vendor=0c88 ProdID=17da Rev= 0.00
S:  Manufacturer=Qualcomm, Incorporated
S:  Product=Qualcomm CDMA Technologies MSM
C:* #Ifs= 2 Cfg#= 1 Atr=a0 MxPwr=100mA
I:  If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=airprime
E:  Ad=81(I) Atr=03(Int.) MxPS=  16 Ivl=128ms
E:  Ad=82(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
I:  If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=airprime
E:  Ad=84(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=04(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Jeremy Fitzhardinge]

Andy Gay wrote:
> BTW - Jeremy suggested that the number of EPs to configure should be
> determined from the device ID. Makes sense to me, but then many users
> may have no use for the additional EPs. Alternatively, Greg suggested
> that maybe this should split into 2 drivers. Any preferences, anyone?
>   
I think if the hardware has the EPs, they should be exposed by the 
driver.  You can tweak usermode as to whether they get device nodes, 
what they're called, etc.
> I don't know which of these devices present multiple EPs though. Can you
> send me the appropriate section from 'cat /proc/bus/usb/devices'?
>   
Phil Karn mentions on http://www.ka9q.net/5220.html:

    It may help to know what's actually inside the 5220 card. It
    contains a Qualcomm MSM 5500 mobile station modem chip that
    implements the actual 1xEV-DO functionality. This chip has a native
    USB 1.1 interface that emulates two USB serial ports. The first
    provides a classic serial modem interface that accepts AT commands
    and PPP data. The second is reserved for diagnostics and is unused.

For completeness:

T:  Bus=03 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12  MxCh= 0
D:  Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
P:  Vendor=1199 ProdID=0218 Rev= 0.01
S:  Manufacturer=Sierra Wireless
S:  Product=Sierra Wireless MC5720 Modem
C:* #Ifs= 1 Cfg#= 1 Atr=e0 MxPwr=  0mA
I:  If#= 0 Alt= 0 #EPs= 7 Cls=ff(vend.) Sub=ff Prot=ff Driver=airprime
E:  Ad=81(I) Atr=03(Int.) MxPS=  16 Ivl=128ms
E:  Ad=82(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=84(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=04(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=85(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=05(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms


    J

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Mon, 2006-07-03 at 00:00 -0700, Jeremy Fitzhardinge wrote:
> Andy Gay wrote:
> > BTW - Jeremy suggested that the number of EPs to configure should be
> > determined from the device ID. Makes sense to me, but then many users
> > may have no use for the additional EPs. Alternatively, Greg suggested
> > that maybe this should split into 2 drivers. Any preferences, anyone?
> >   
> I think if the hardware has the EPs, they should be exposed by the 
> driver.  You can tweak usermode as to whether they get device nodes, 
> what they're called, etc.
I tend to agree. I'm thinking for now I should leave it as is, so it
defaults to configuring 3 EPs. Perhaps later I'll try to collect #EP
info for all the supported devices.

> > I don't know which of these devices present multiple EPs though. Can you
> > send me the appropriate section from 'cat /proc/bus/usb/devices'?
> >   
> Phil Karn mentions on http://www.ka9q.net/5220.html:
> 
>     It may help to know what's actually inside the 5220 card. It
>     contains a Qualcomm MSM 5500 mobile station modem chip that
>     implements the actual 1xEV-DO functionality. This chip has a native
>     USB 1.1 interface that emulates two USB serial ports. The first
>     provides a classic serial modem interface that accepts AT commands
>     and PPP data. The second is reserved for diagnostics and is unused.
> 
> For completeness:
> 
> T:  Bus=03 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12  MxCh= 0
> D:  Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> P:  Vendor=1199 ProdID=0218 Rev= 0.01
> S:  Manufacturer=Sierra Wireless
> S:  Product=Sierra Wireless MC5720 Modem
This is curious. I saw that '0218' in Greg's code, and 'corrected' it to
0018, because here's what I get with my MC5720:
P:  Vendor=1199 ProdID=0018 Rev= 0.01
S:  Manufacturer=Sierra Wireless
S:  Product=Sierra Wireless MC5720 Modem

So evidently there are also multiple variants of each modem.

> C:* #Ifs= 1 Cfg#= 1 Atr=e0 MxPwr=  0mA
> I:  If#= 0 Alt= 0 #EPs= 7 Cls=ff(vend.) Sub=ff Prot=ff Driver=airprime
> E:  Ad=81(I) Atr=03(Int.) MxPS=  16 Ivl=128ms
> E:  Ad=82(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> E:  Ad=84(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> E:  Ad=04(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> E:  Ad=85(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> E:  Ad=05(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> 
> 
>     J

Re: [linux-usb-devel] [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Ken Brush]

On 6/29/06, Andy Gay <andy@andynet.net> wrote:
>
> Adapted from an earlier patch by Greg KH <gregkh@suse.de>.
> That patch added multiple read urbs and larger transfer buffers to allow
> data transfers at full EvDO speed.
>
> This version includes additional device IDs and fixes a memory leak in
> the transfer buffer allocation.
>
> Some (maybe all?) of the supported devices present multiple bulk endpoints,
> the additional EPs can be used for control and status functions.
> This version allocates 3 EPs by default, that can be changed using
> the 'endpoints' module parameter.
>
> Tested with Sierra Wireless EM5625 and MC5720 embedded modules.
>

With my aircard 580, I get 6 TTYUSB devices and a Urb too big message.
You should probably take the 580 out of the driver unless someone
actually has one and it works for them.

-Ken

Re: [linux-usb-devel] [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Mon, 2006-07-03 at 08:43 -0700, Ken Brush wrote:
> On 6/29/06, Andy Gay <andy@andynet.net> wrote:
> >
> > Adapted from an earlier patch by Greg KH <gregkh@suse.de>.
> > That patch added multiple read urbs and larger transfer buffers to allow
> > data transfers at full EvDO speed.
> >
> > This version includes additional device IDs and fixes a memory leak in
> > the transfer buffer allocation.
> >
> > Some (maybe all?) of the supported devices present multiple bulk endpoints,
> > the additional EPs can be used for control and status functions.
> > This version allocates 3 EPs by default, that can be changed using
> > the 'endpoints' module parameter.
> >
> > Tested with Sierra Wireless EM5625 and MC5720 embedded modules.
> >
> 
> With my aircard 580, I get 6 TTYUSB devices and a Urb too big message.
> You should probably take the 580 out of the driver unless someone
> actually has one and it works for them.
Well, that's the easy way out :)
But I'm willing to try to debug this, if you're game.

I don't know how it could see 6 EPs, unless you specify endpoints=6 when
you load the module. Maybe it sees it as 2 devices somehow?
Can you send me the relevant part of 'cat /proc/bus/usb/devices'?

Also the dmesg where you get the 'urb too big'. You could also try to
load the module with debug enabled.

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

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Jeremy Fitzhardinge]

Andy Gay wrote:
>> I think if the hardware has the EPs, they should be exposed by the 
>> driver.  You can tweak usermode as to whether they get device nodes, 
>> what they're called, etc.
>>     
> I tend to agree. I'm thinking for now I should leave it as is, so it
> defaults to configuring 3 EPs. Perhaps later I'll try to collect #EP
> info for all the supported devices.
>   

Why not just expose all the EPs the hardware presents?  Is there a 
chance they might not be a serial connection?

> This is curious. I saw that '0218' in Greg's code, and 'corrected' it to
> 0018, because here's what I get with my MC5720:
>   

Yes, that patch was from me.

> P:  Vendor=1199 ProdID=0018 Rev= 0.01
> S:  Manufacturer=Sierra Wireless
> S:  Product=Sierra Wireless MC5720 Modem
>
> So evidently there are also multiple variants of each modem.
>   
My came embedded in a Thinkpad X60, and I think it is locked to 
Verizon.  The product ID might reflect either or both of those states.

    J

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Greg KH]

On Sun, Jul 02, 2006 at 04:29:01PM -0400, Andy Gay wrote:
> On Sun, 2006-07-02 at 11:48 -0700, Roland Dreier wrote:
> > this works well on my kyocera kpc650 -- throughput is up to about 1
> > mbit/sec vs. ~250 kbit/sec with the stock airprime driver.
> > -
> Thanks for the feedback.
> 
> I'm working on fixing the concerns Andrew Morton expressed regarding
> memory leaks in the open function. I'll send an updated driver soon.
> 
> BTW - Jeremy suggested that the number of EPs to configure should be
> determined from the device ID. Makes sense to me, but then many users
> may have no use for the additional EPs. Alternatively, Greg suggested
> that maybe this should split into 2 drivers. Any preferences, anyone?

Yes, this driver is already split into 2 different ones (look in the
recent -mm releases).  Sierra wants to have their devices be in their
own driver, as the chip is a little different from the other ones.  This
means that those devices are now controlled by a driver called "sierra"
and the other devices still are working with the airprime driver.

This should hopefully fix the different endpoint issue, and allow new
devices to be supported properly, as Sierra Wireless is now maintaining
that driver.

Hope this helps,

greg k-h

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Mon, 2006-07-03 at 09:28 -0700, Jeremy Fitzhardinge wrote:
> Andy Gay wrote:
> >> I think if the hardware has the EPs, they should be exposed by the 
> >> driver.  You can tweak usermode as to whether they get device nodes, 
> >> what they're called, etc.
> >>     
> > I tend to agree. I'm thinking for now I should leave it as is, so it
> > defaults to configuring 3 EPs. Perhaps later I'll try to collect #EP
> > info for all the supported devices.
> >   
> 
> Why not just expose all the EPs the hardware presents?  Is there a 
> chance they might not be a serial connection?
Maybe. I believe it's possible to configure the Sierra kit to present an
NDIS interface. Not at all sure how that works. I'd have thought it
would be another layer on top of ppp, but who knows?

But I'm not sure I know how to do that anyway, without hardware to test
with.
For example, look at the info Roland sent me for his Kyocera:
I:  If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=airprime
E:  Ad=81(I) Atr=03(Int.) MxPS=  16 Ivl=128ms
E:  Ad=82(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
I:  If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=airprime
E:  Ad=84(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
E:  Ad=04(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms

Which is somewhat different from the MC5720 results you and I get.
Notice it appears to show 2 interfaces, with 1 pair of in/out bulk EPs
on each. We see 1 interface with 3 pairs. I don't know if/how that
difference needs to be addressed in the driver.

I wonder if that's why Ken got 6 EPs. Perhaps his 580 card presents 2
interfaces, with 3 EPs each.

Also, I know how to drive the 2nd EP on the MC5720. So I can test that
it actually works. But I have to rely on information I can't disclose to
do that (pesky NDAs). I don't know anything about the 3rd EP. And if
there really are 6 EPs on the 580, how the heck do we test that...

Seems the deeper I dig here, the murkier this all gets. I'm inclined to
keep it simple for now, at least... Really, how many users need those
extra EPs?

> 
> > This is curious. I saw that '0218' in Greg's code, and 'corrected' it to
> > 0018, because here's what I get with my MC5720:
> >   
> 
> Yes, that patch was from me.
> 
> > P:  Vendor=1199 ProdID=0018 Rev= 0.01
> > S:  Manufacturer=Sierra Wireless
> > S:  Product=Sierra Wireless MC5720 Modem
> >
> > So evidently there are also multiple variants of each modem.
> >   
> My came embedded in a Thinkpad X60, and I think it is locked to 
> Verizon.  The product ID might reflect either or both of those states.
> 
>     J

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Mon, 2006-07-03 at 10:00 -0700, Greg KH wrote:
> Yes, this driver is already split into 2 different ones (look in the
> recent -mm releases).  Sierra wants to have their devices be in their
> own driver, as the chip is a little different from the other ones.  This
> means that those devices are now controlled by a driver called "sierra"
> and the other devices still are working with the airprime driver.
> 
> This should hopefully fix the different endpoint issue, and allow new
> devices to be supported properly, as Sierra Wireless is now maintaining
> that driver.
Aha, good news. So this patch is already obsolete, for the Sierra stuff
anyway. And as I only have Sierra kit to work with, I reckon I should
drop out of this now.
I did make some changes to the last patch to do the cleanup stuff in the
open function, do you want to see those?

> Hope this helps,
> 
Sure does!

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

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Jeremy Fitzhardinge]

Andy Gay wrote:
> Aha, good news. So this patch is already obsolete, for the Sierra stuff
> anyway. And as I only have Sierra kit to work with, I reckon I should
> drop out of this now.
> I did make some changes to the last patch to do the cleanup stuff in the
> open function, do you want to see those?
>   
The Sierra driver as it stands is very bare-bones; it doesn't yet have 
the larger buffers.  So for now, I'll keep using the airprime one until 
sierra gets up to speed (though it seems like they should have a fair 
amount of common code).

    J

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Greg KH]

On Mon, Jul 03, 2006 at 01:55:28PM -0400, Andy Gay wrote:
> On Mon, 2006-07-03 at 10:00 -0700, Greg KH wrote:
> > Yes, this driver is already split into 2 different ones (look in the
> > recent -mm releases).  Sierra wants to have their devices be in their
> > own driver, as the chip is a little different from the other ones.  This
> > means that those devices are now controlled by a driver called "sierra"
> > and the other devices still are working with the airprime driver.
> > 
> > This should hopefully fix the different endpoint issue, and allow new
> > devices to be supported properly, as Sierra Wireless is now maintaining
> > that driver.
> Aha, good news. So this patch is already obsolete, for the Sierra stuff
> anyway. And as I only have Sierra kit to work with, I reckon I should
> drop out of this now.

No, not at all.  I'll gladly add your patch to the sierra driver, if you
say it works for your devices.

> I did make some changes to the last patch to do the cleanup stuff in the
> open function, do you want to see those?

Yes, please send the latest version, and I'll apply it.

thanks,

greg k-h

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Mon, 2006-07-03 at 11:16 -0700, Greg KH wrote:
> On Mon, Jul 03, 2006 at 01:55:28PM -0400, Andy Gay wrote:
> > On Mon, 2006-07-03 at 10:00 -0700, Greg KH wrote:
> > > Yes, this driver is already split into 2 different ones (look in the
> > > recent -mm releases).  Sierra wants to have their devices be in their
> > > own driver, as the chip is a little different from the other ones.  This
> > > means that those devices are now controlled by a driver called "sierra"
> > > and the other devices still are working with the airprime driver.
> > > 
> > > This should hopefully fix the different endpoint issue, and allow new
> > > devices to be supported properly, as Sierra Wireless is now maintaining
> > > that driver.
> > Aha, good news. So this patch is already obsolete, for the Sierra stuff
> > anyway. And as I only have Sierra kit to work with, I reckon I should
> > drop out of this now.
> 
> No, not at all.  I'll gladly add your patch to the sierra driver, if you
> say it works for your devices.
> 
> > I did make some changes to the last patch to do the cleanup stuff in the
> > open function, do you want to see those?
> 
> Yes, please send the latest version, and I'll apply it.

OK, here it is!

-----

Adapted from an earlier patch by Greg KH <gregkh@suse.de>.
That patch added multiple read urbs and larger transfer buffers to allow
data transfers at full EvDO speed.

This version includes additional device IDs and fixes a memory leak in
the transfer buffer allocation.

Some (maybe all?) of the supported devices present multiple bulk endpoints,
the additional EPs can be used for control and status functions,
This version allocates 3 EPs by default, that can be changed using
the 'endpoints' module parameter.

Tested with Sierra Wireless EM5625 and MC5720 embedded modules.

Device ID (0x0c88, 0x17da) for the Kyocera Wireless KPC650/Passport
was added but is not yet tested.

---

Changes from previous patch:
 - fixed open function to clean up if anything goes wrong;
 - free private structure in close function;
 - add back alternate ID 0x0218 for the MC5720.

This is now reported as working for the Kyocera KPC650 (by Roland
Dreier <rdreier@cisco.com>), but as not working for the Sierra
Aircard 580 (by Ken Brush <kbrush@gmail.com>).

---

 drivers/usb/serial/airprime.c |  261 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 255 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/airprime.c b/drivers/usb/serial/airprime.c
index 94b9ba0..1add380 100644
--- a/drivers/usb/serial/airprime.c
+++ b/drivers/usb/serial/airprime.c
@@ -1,36 +1,272 @@
 /*
  * AirPrime CDMA Wireless Serial USB driver
  *
- * Copyright (C) 2005 Greg Kroah-Hartman <gregkh@suse.de>
+ * Copyright (C) 2005-2006 Greg Kroah-Hartman <gregkh@suse.de>
  *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License version
  *	2 as published by the Free Software Foundation.
  */
-
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/tty.h>
+#include <linux/tty_flip.h>
 #include <linux/module.h>
 #include <linux/usb.h>
 #include "usb-serial.h"
 
 static struct usb_device_id id_table [] = {
 	{ USB_DEVICE(0x0c88, 0x17da) }, /* Kyocera Wireless KPC650/Passport */
-	{ USB_DEVICE(0xf3d, 0x0112) },  /* AirPrime CDMA Wireless PC Card */
+	{ USB_DEVICE(0x0f3d, 0x0112) },	/* AirPrime CDMA Wireless PC Card */
 	{ USB_DEVICE(0x1410, 0x1110) }, /* Novatel Wireless Merlin CDMA */
+	{ USB_DEVICE(0x1199, 0x0017) }, /* Sierra Wireless EM5625 */
+	{ USB_DEVICE(0x1199, 0x0018) }, /* Sierra Wireless MC5720 */
+	{ USB_DEVICE(0x1199, 0x0218) }, /* Sierra Wireless MC5720 (alt) */
 	{ USB_DEVICE(0x1199, 0x0112) }, /* Sierra Wireless Aircard 580 */
-	{ USB_DEVICE(0x1199, 0x0218) }, /* Sierra Wireless MC5720 */
 	{ },
 };
 MODULE_DEVICE_TABLE(usb, id_table);
+#define URB_TRANSFER_BUFFER_SIZE	4096
+#define NUM_READ_URBS			4
+#define NUM_WRITE_URBS			4
+#define NUM_BULK_EPS			3
+#define MAX_BULK_EPS			6
+
+/* if overridden by the user, then use their value for the size of the
+ * read and write urbs, and the number of endpoints */
+static int buffer_size = URB_TRANSFER_BUFFER_SIZE;
+static int endpoints = NUM_BULK_EPS;
+static int debug;
+struct airprime_private {
+	spinlock_t lock;
+	int outstanding_urbs;
+	int throttled;
+	struct urb *read_urbp[NUM_READ_URBS];
+};
+
+static void airprime_read_bulk_callback(struct urb *urb, struct pt_regs *regs)
+{
+	struct usb_serial_port *port = urb->context;
+	unsigned char *data = urb->transfer_buffer;
+	struct tty_struct *tty;
+	int result;
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	if (urb->status) {
+		dbg("%s - nonzero read bulk status received: %d",
+		    __FUNCTION__, urb->status);
+		/* something happened, so free up the memory for this urb */
+		if (urb->transfer_buffer) {
+			kfree (urb->transfer_buffer);
+			urb->transfer_buffer = NULL;
+		}
+		return;
+	}
+	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, urb->actual_length, data);
+
+	tty = port->tty;
+	if (tty && urb->actual_length) {
+		tty_insert_flip_string (tty, data, urb->actual_length);
+		tty_flip_buffer_push (tty);
+	}
+
+	result = usb_submit_urb (urb, GFP_ATOMIC);
+	if (result)
+		dev_err(&port->dev, "%s - failed resubmitting read urb, error %d\n",
+			__FUNCTION__, result);
+	return;
+}
+
+static void airprime_write_bulk_callback(struct urb *urb, struct pt_regs *regs)
+{
+	struct usb_serial_port *port = urb->context;
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	/* free up the transfer buffer, as usb_free_urb() does not do this */
+	kfree (urb->transfer_buffer);
+
+	if (urb->status)
+		dbg("%s - nonzero write bulk status received: %d",
+		    __FUNCTION__, urb->status);
+	spin_lock_irqsave(&priv->lock, flags);
+	--priv->outstanding_urbs;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	usb_serial_port_softint(port);
+}
+
+static int airprime_open(struct usb_serial_port *port, struct file *filp)
+{
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	struct usb_serial *serial = port->serial;
+	struct urb *urb;
+	char *buffer = NULL;
+	int i;
+	int result = 0;
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	/* initialize our private data structure if it isn't already created */
+	if (!priv) {
+		priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+		if (!priv) {
+			result = -ENOMEM;
+			goto out;
+		}
+		spin_lock_init(&priv->lock);
+		usb_set_serial_port_data(port, priv);
+	}
+
+	for (i = 0; i < NUM_READ_URBS; ++i) {
+		buffer = kmalloc(buffer_size, GFP_KERNEL);
+		if (!buffer) {
+			dev_err(&port->dev, "%s - out of memory.\n",
+				__FUNCTION__);
+			result = -ENOMEM;
+			goto errout;
+		}
+		urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!urb) {
+			dev_err(&port->dev, "%s - no more urbs?\n",
+				__FUNCTION__);
+			result = -ENOMEM;
+			goto errout;
+		}
+		usb_fill_bulk_urb(urb, serial->dev,
+				  usb_rcvbulkpipe(serial->dev,
+						  port->bulk_out_endpointAddress),
+				  buffer, buffer_size,
+				  airprime_read_bulk_callback, port);
+		result = usb_submit_urb(urb, GFP_KERNEL);
+		if (result) {
+			dev_err(&port->dev,
+				"%s - failed submitting read urb %d for port %d, error %d\n",
+				__FUNCTION__, i, port->number, result);
+			goto errout;
+		}
+		/* remember this urb so we can kill it when the port is closed */
+		priv->read_urbp[i] = urb;
+	}
+	goto out;
+
+ errout:
+	/* some error happened, cancel any submitted urbs and clean up anything that
+	   got allocated successfully */
+
+	for ( ; i >= 0; --i) {
+		urb = priv->read_urbp[i];
+		if (urb) {
+			/* This urb was submitted successfully. So we have to
+			   cancel it.
+			   Unlinking the urb will invoke read_bulk_callback()
+			   with an error status, so its transfer buffer will
+			   be freed there */
+			if (usb_unlink_urb (urb) != -EINPROGRESS) {
+				/* comments in drivers/usb/core/urb.c say this
+				   can only happen if the urb was never submitted,
+				   or has completed already.
+				   Either way we may have to free the transfer
+				   buffer here. */
+				if (urb->transfer_buffer) {
+					kfree (urb->transfer_buffer);
+					urb->transfer_buffer = NULL;
+				}
+			}
+			usb_free_urb (urb);
+		}
+	}
+
+ out:
+	return result;
+}
+
+static void airprime_close(struct usb_serial_port *port, struct file * filp)
+{
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	int i;
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	/* killing the urb will invoke read_bulk_callback() with an error status,
+	   so the transfer buffer will be freed there */
+	for (i = 0; i < NUM_READ_URBS; ++i) {
+		usb_kill_urb (priv->read_urbp[i]);
+		usb_free_urb (priv->read_urbp[i]);
+	}
+
+	/* free up private structure */
+	kfree (priv);
+	usb_set_serial_port_data(port, NULL);
+}
+
+static int airprime_write(struct usb_serial_port *port,
+			  const unsigned char *buf, int count)
+{
+	struct airprime_private *priv = usb_get_serial_port_data(port);
+	struct usb_serial *serial = port->serial;
+	struct urb *urb;
+	unsigned char *buffer;
+	unsigned long flags;
+	int status;
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (priv->outstanding_urbs > NUM_WRITE_URBS) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		dbg("%s - write limit hit\n", __FUNCTION__);
+		return 0;
+	}
+	spin_unlock_irqrestore(&priv->lock, flags);
+	buffer = kmalloc(count, GFP_ATOMIC);
+	if (!buffer) {
+		dev_err(&port->dev, "out of memory\n");
+		return -ENOMEM;
+	}
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		dev_err(&port->dev, "no more free urbs\n");
+		kfree (buffer);
+		return -ENOMEM;
+	}
+	memcpy (buffer, buf, count);
+
+	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count, buffer);
+
+	usb_fill_bulk_urb(urb, serial->dev,
+			  usb_sndbulkpipe(serial->dev,
+					  port->bulk_out_endpointAddress),
+			  buffer, count,
+			  airprime_write_bulk_callback, port);
+
+	/* send it down the pipe */
+	status = usb_submit_urb(urb, GFP_ATOMIC);
+	if (status) {
+		dev_err(&port->dev,
+			"%s - usb_submit_urb(write bulk) failed with status = %d\n",
+			__FUNCTION__, status);
+		count = status;
+		kfree (buffer);
+	} else {
+		spin_lock_irqsave(&priv->lock, flags);
+		++priv->outstanding_urbs;
+		spin_unlock_irqrestore(&priv->lock, flags);
+	}
+	/* we are done with this urb, so let the host driver
+	 * really free it when it is finished with it */
+	usb_free_urb (urb);
+	return count;
+}
 
 static struct usb_driver airprime_driver = {
 	.name =		"airprime",
 	.probe =	usb_serial_probe,
 	.disconnect =	usb_serial_disconnect,
 	.id_table =	id_table,
-	.no_dynamic_id = 	1,
+	.no_dynamic_id =	1,
 };
 
 static struct usb_serial_driver airprime_device = {
@@ -42,13 +278,17 @@ static struct usb_serial_driver airprime
 	.num_interrupt_in =	NUM_DONT_CARE,
 	.num_bulk_in =		NUM_DONT_CARE,
 	.num_bulk_out =		NUM_DONT_CARE,
-	.num_ports =		1,
+	.open =			airprime_open,
+	.close =		airprime_close,
+	.write =		airprime_write,
 };
 
 static int __init airprime_init(void)
 {
 	int retval;
 
+	airprime_device.num_ports =
+		(endpoints > 0 && endpoints <= MAX_BULK_EPS) ? endpoints : NUM_BULK_EPS;
 	retval = usb_serial_register(&airprime_device);
 	if (retval)
 		return retval;
@@ -60,6 +300,8 @@ static int __init airprime_init(void)
 
 static void __exit airprime_exit(void)
 {
+	dbg("%s", __FUNCTION__);
+
 	usb_deregister(&airprime_driver);
 	usb_serial_deregister(&airprime_device);
 }
@@ -67,3 +309,10 @@ static void __exit airprime_exit(void)
 module_init(airprime_init);
 module_exit(airprime_exit);
 MODULE_LICENSE("GPL");
+
+module_param(debug, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(debug, "Debug enabled");
+module_param(buffer_size, int, 0);
+MODULE_PARM_DESC(buffer_size, "Size of the transfer buffers in bytes (default 4096)");
+module_param(endpoints, int, 0);
+MODULE_PARM_DESC(endpoints, "Number of bulk EPs to configure (default 3)");

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Andrew Morton <akpm@osdl.org> writes:
> On Fri, 30 Jun 2006 01:48:02 -0400
> Andy Gay <andy@andynet.net> wrote:
[...]
>> +	if (tty && urb->actual_length) {
>> +		tty_buffer_request_room(tty, urb->actual_length);
>> +		tty_insert_flip_string(tty, data, urb->actual_length);
>
> Is it correct to ignore the return value from those two functions?
>
>> +		tty_flip_buffer_push(tty);

It seems that there is much worse problem here. The amount of data that
has been inserted by the tty_insert_flip_string() could be up to
URB_TRANSFER_BUFFER_SIZE (=4096 bytes) and may easily exceed
TTY_THRESHOLD_THROTTLE (=128 bytes) defined in the
char/n_tty.c. Pushing this data to the n_tty line discipline may then
overflow the line discipline buffer resulting in silent data loss. The
line discipline will call throttle() callback some time later then, but
it will be too late :(

This problem I've seen in my own driver with 2.4.x kernels, and I had
just re-checked it still exists with 2.6.17.4 kernel [had a hope Alan
changes to tty buffers fixed it, but no luck]. Besides, there are at
least 2 drivers in the kernel tree that try to deal with this problem,
char/hvsi.c, and serial/crisv10.c. For example, here is comment from
hvsi.c:

/*
 * We could get 252 bytes of data at once here. But the tty layer only
 * throttles us at TTY_THRESHOLD_THROTTLE (128) bytes, so we could overflow
 * it. Accordingly we won't send more than 128 bytes at a time to the flip
 * buffer, which will give the tty buffer a chance to throttle us. Should the
 * value of TTY_THRESHOLD_THROTTLE change in n_tty.c, this code should be
 * revisited.
 */

So, how is it supposed to work? Am I missing something?

Please also notice that attempts to overcome the problem in the drivers
look like fragile hacks that depend on intimate details of particular
line discipline. Any ideas how to fix it in general? Nice occasion to
apply "stable interfaces are evil" once again? ;)

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Alan Cox]

Ar Gwe, 2006-07-07 am 21:23 +0400, ysgrifennodd Sergei Organov:
> It seems that there is much worse problem here. The amount of data that
> has been inserted by the tty_insert_flip_string() could be up to
> URB_TRANSFER_BUFFER_SIZE (=4096 bytes) and may easily exceed
> TTY_THRESHOLD_THROTTLE (=128 bytes) defined in the
> char/n_tty.c. 

You may throttle late but that is always true as there is an implicit
race between the hardware signals and the chip FIFO on all UART devices.
The buffering should be taking care of it, and the tty layer taking care
not to over stuff the ldisc which I thought Paul had fixed while doing
the locking wizardry

Alan

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> Ar Gwe, 2006-07-07 am 21:23 +0400, ysgrifennodd Sergei Organov:
>> It seems that there is much worse problem here. The amount of data that
>> has been inserted by the tty_insert_flip_string() could be up to
>> URB_TRANSFER_BUFFER_SIZE (=4096 bytes) and may easily exceed
>> TTY_THRESHOLD_THROTTLE (=128 bytes) defined in the
>> char/n_tty.c. 
>
> You may throttle late but that is always true as there is an implicit
> race between the hardware signals and the chip FIFO on all UART
> devices.

I don't talk about races here. What I see is simple logical software
problem arose due to poor interface between ldisc and tty. One can't
easily reproduce it with UART drivers unless UART FIFO is larger than
128 bytes, and even if such an UART exists, I think the RS232 is too
slow anyway so that in practice tty never tries to flip more than 128
bytes when ldisc buffer is almost full. I.e., it's hardware
characteristics of UARTs/RS232 that prevent the problem from being
triggered.

However, the problem is easily seen for USB-to-tty drivers where there
are no UARTS anywhere and speeds are rather high so that more than 4096
bytes (the line discipline buffer size) could be received before a task
has a chance to read from the line discipline buffer, and single flip
size is not limited by the hardware.

> The buffering should be taking care of it, and the tty layer taking care
> not to over stuff the ldisc 

Well, it should, but it doesn't seem to, -- that's the problem :(
Moreover, looking into the source code I don't see how tty can take care
not to over-stuff the ldisc. ldisc`s receive_buf() routine doesn't tell
the caller how many chars it actually consumed and silently throws away
whatever doesn't fit into its buffer. After it threw away unknown (to
the caller) amount of bytes, it calls throttle(), but first it's too
late and second tty flip code doesn't handle throttle() itself
anyway. So once again how this "tty layer taking care not to over stuff
the ldisc" is supposed to work?

Overall, the definition of the problem is: one can't reliably flip more
than 128 bytes at once without danger of missing of bytes. [This in turn
implies that with tty->low_latency set to 0 one can't reliably flip any
bytes at all(!) as N flips of M bytes each may be effectively converted
into delayed flip of M*N bytes.]

> which I thought Paul had fixed while doing the locking wizardry

I don't think locking has anything to do about it, but if it's indeed
fixed, where can I take the patch(es) as it doesn't seem to be fixed in
2.17.4?

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Alan Cox]

Ar Llu, 2006-07-10 am 14:36 +0400, ysgrifennodd Sergei Organov:
> However, the problem is easily seen for USB-to-tty drivers where there
> are no UARTS anywhere and speeds are rather high so that more than 4096
> bytes (the line discipline buffer size) could be received before a task
> has a chance to read from the line discipline buffer, and single flip
> size is not limited by the hardware.

There are no flip buffers in 2.6.17, they've gone. The tty buffering is
now a proper queuing system.

> Moreover, looking into the source code I don't see how tty can take care
> not to over-stuff the ldisc. ldisc`s receive_buf() routine doesn't tell
> the caller how many chars it actually consumed and silently throws away

Not in the current kernel tree. The current tree does this:

       spin_lock_irqsave(&tty->buf.lock, flags);
        head = tty->buf.head;
        if (head != NULL) {
                tty->buf.head = NULL;
                for (;;) {
                        int count = head->commit - head->read;
                        if (!count) {
                                if (head->next == NULL)
                                        break;
                                tbuf = head;
                                head = head->next;
                                tty_buffer_free(tty, tbuf);
                                continue;
                        }
                        if (!tty->receive_room) {
                                schedule_delayed_work(&tty->buf.work, 1);
                                break;
                        }
                        if (count > tty->receive_room)
                                count = tty->receive_room;
                        char_buf = head->char_buf_ptr + head->read;
                        flag_buf = head->flag_buf_ptr + head->read;
                        head->read += count;
                        spin_unlock_irqrestore(&tty->buf.lock, flags);
                        disc->receive_buf(tty, char_buf, flag_buf, count);
                        spin_lock_irqsave(&tty->buf.lock, flags);
                }
                tty->buf.head = head;
        }
        spin_unlock_irqrestore(&tty->buf.lock, flags);

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> Ar Llu, 2006-07-10 am 14:36 +0400, ysgrifennodd Sergei Organov:
>> However, the problem is easily seen for USB-to-tty drivers where there
>> are no UARTS anywhere and speeds are rather high so that more than 4096
>> bytes (the line discipline buffer size) could be received before a task
>> has a chance to read from the line discipline buffer, and single flip
>> size is not limited by the hardware.
>
> There are no flip buffers in 2.6.17, they've gone.

Sure they gone, I've used "flip" because of tty_flip_buffer_push() has "flip"
in its name.

> The tty buffering is now a proper queuing system.

Yes, I know it is.

>
>> Moreover, looking into the source code I don't see how tty can take care
>> not to over-stuff the ldisc. ldisc`s receive_buf() routine doesn't tell
>> the caller how many chars it actually consumed and silently throws away
>
> Not in the current kernel tree. The current tree does this:

Which tree? I see rather different code in the 2.6.17.4, see below.

>
>        spin_lock_irqsave(&tty->buf.lock, flags);
>         head = tty->buf.head;
>         if (head != NULL) {
>                 tty->buf.head = NULL;
>                 for (;;) {
>                         int count = head->commit - head->read;
>                         if (!count) {
>                                 if (head->next == NULL)
>                                         break;
>                                 tbuf = head;
>                                 head = head->next;
>                                 tty_buffer_free(tty, tbuf);
>                                 continue;
>                         }
>                         if (!tty->receive_room) {
>                                 schedule_delayed_work(&tty->buf.work, 1);
>                                 break;
>                         }
>                         if (count > tty->receive_room)
>                                 count = tty->receive_room;
>                         char_buf = head->char_buf_ptr + head->read;
>                         flag_buf = head->flag_buf_ptr + head->read;
>                         head->read += count;
>                         spin_unlock_irqrestore(&tty->buf.lock, flags);
>                         disc->receive_buf(tty, char_buf, flag_buf, count);
>                         spin_lock_irqsave(&tty->buf.lock, flags);
>                 }
>                 tty->buf.head = head;
>         }
>         spin_unlock_irqrestore(&tty->buf.lock, flags);

Wow! The code you've quoted seems to be correct! But where did you get
it from? The version of flush_to_ldisc() from drivers/char/tty_io.c from
2.17.4 got last Friday from kernel.org does this:

	spin_lock_irqsave(&tty->buf.lock, flags);
	while((tbuf = tty->buf.head) != NULL) {
		while ((count = tbuf->commit - tbuf->read) != 0) {
			char_buf = tbuf->char_buf_ptr + tbuf->read;
			flag_buf = tbuf->flag_buf_ptr + tbuf->read;
			tbuf->read += count;
			spin_unlock_irqrestore(&tty->buf.lock, flags);
			disc->receive_buf(tty, char_buf, flag_buf, count);
			spin_lock_irqsave(&tty->buf.lock, flags);
		}
		if (tbuf->active)
			break;
		tty->buf.head = tbuf->next;
		if (tty->buf.head == NULL)
			tty->buf.tail = NULL;
		tty_buffer_free(tty, tbuf);
	}
	spin_unlock_irqrestore(&tty->buf.lock, flags);

Nowhere tty->receive_room is checked in tty_io.c! Am I missing something
or is this change not yet in the released kernels? If it's not yet
there, where can I get it and when is it expected to reach released
kernels?

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> Ar Llu, 2006-07-10 am 19:54 +0400, ysgrifennodd Sergei Organov:
>> Wow! The code you've quoted seems to be correct! But where did you get
>> it from? The version of flush_to_ldisc() from drivers/char/tty_io.c from
>> 2.17.4 got last Friday from kernel.org does this:
>
>>From HEAD so it should make 2.6.18. Paul fixed this one.

Great news, -- the time I can finally throw away my ugly work-around is
coming!

Thank you a lot for clarifying the issue!

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Alan Cox]

Ar Llu, 2006-07-10 am 19:54 +0400, ysgrifennodd Sergei Organov:
> Wow! The code you've quoted seems to be correct! But where did you get
> it from? The version of flush_to_ldisc() from drivers/char/tty_io.c from
> 2.17.4 got last Friday from kernel.org does this:

>From HEAD so it should make 2.6.18. Paul fixed this one.


Alan

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Andy Gay <andy@andynet.net> writes:
> Adapted from an earlier patch by Greg KH <gregkh@suse.de>.
> That patch added multiple read urbs and larger transfer buffers to allow
> data transfers at full EvDO speed.

Below are two more problems with the patch, one of which existed in the
original Greg's patch resulting in return with "Message too long"
(EMSGSIZE) from driver's open() function.

[...]

> +		/* something happened, so free up the memory for this urb /*

There should be '*/' at the end of this line, not '/*', otherwise the
driver even doesn't compile.

[...]
> +static int airprime_open(struct usb_serial_port *port, struct file *filp)
> +{
[...]
> +		usb_fill_bulk_urb(urb, serial->dev,
> +				  usb_rcvbulkpipe(serial->dev,
> +						  port->bulk_out_endpointAddress),

Here, it should obviously be port->bulk_in_endpointAddress, not
port->bulk_out_endpointAddress, otherwise devices that have endpoints
numeration like, say 0x01-out, 0x82-in (unlike more usual usual
0x01-out, 0x81-in), won't work returning -EMSGSIZE from open().

After these fixes, I've been able to run the driver with my own USB
device and achieved about 320 Kbytes/s read speed. That's still not very
exciting as I have another driver here in development that seems to be
able to do about 650 Kbytes/s with the same device.

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Andy Gay]

On Tue, 2006-07-11 at 22:31 +0400, Sergei Organov wrote:
> Andy Gay <andy@andynet.net> writes:
> > Adapted from an earlier patch by Greg KH <gregkh@suse.de>.
> > That patch added multiple read urbs and larger transfer buffers to allow
> > data transfers at full EvDO speed.
> 
> Below are two more problems with the patch, one of which existed in the
> original Greg's patch resulting in return with "Message too long"
> (EMSGSIZE) from driver's open() function.
> 
> [...]
> 
> > +		/* something happened, so free up the memory for this urb /*
> 
> There should be '*/' at the end of this line, not '/*', otherwise the
> driver even doesn't compile.

Sure. I posted an updated version including that fix -
http://lkml.org/lkml/2006/7/3/280

> 
> [...]
> > +static int airprime_open(struct usb_serial_port *port, struct file *filp)
> > +{
> [...]
> > +		usb_fill_bulk_urb(urb, serial->dev,
> > +				  usb_rcvbulkpipe(serial->dev,
> > +						  port->bulk_out_endpointAddress),
> 
> Here, it should obviously be port->bulk_in_endpointAddress, not
> port->bulk_out_endpointAddress, otherwise devices that have endpoints
> numeration like, say 0x01-out, 0x82-in (unlike more usual usual
> 0x01-out, 0x81-in), won't work returning -EMSGSIZE from open().

Sounds correct. Good catch!

> 
> After these fixes, I've been able to run the driver with my own USB
> device and achieved about 320 Kbytes/s read speed. That's still not very
> exciting as I have another driver here in development that seems to be
> able to do about 650 Kbytes/s with the same device.
> 
Nice. That's over 5Mbits/sec!

Is that on an EvDO network? I didn't know they could go that fast. The
EvDO network I'm testing on can only manage about 1Mbit/sec at best.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Andy Gay <andy@andynet.net> writes:
> On Tue, 2006-07-11 at 22:31 +0400, Sergei Organov wrote:
>> Andy Gay <andy@andynet.net> writes:

[...]

>> > +		/* something happened, so free up the memory for this urb /*
>> 
>> There should be '*/' at the end of this line, not '/*', otherwise the
>> driver even doesn't compile.
>
> Sure. I posted an updated version including that fix -
> http://lkml.org/lkml/2006/7/3/280

OK, I've forgot about that one, sorry.

[...]

>> 
>> After these fixes, I've been able to run the driver with my own USB
>> device and achieved about 320 Kbytes/s read speed. That's still not very
>> exciting as I have another driver here in development that seems to be
>> able to do about 650 Kbytes/s with the same device.
>> 
> Nice. That's over 5Mbits/sec!
>
> Is that on an EvDO network? I didn't know they could go that fast. The
> EvDO network I'm testing on can only manage about 1Mbit/sec at best.

No, it is not, -- that's my own device I write firmware for. In our
earlier discussion about high-speed bulk-to-tty converter Greg suggested
to try to use improved airprime driver to see if it's fast enough, and
probably then we may come up with high speed generic bulk-to-tty
converter. If we finally get it done, the airprime and other similar
drivers may directly use it for read/write/handshake operations and
manage only their specifics themselves.

Now to the difference in speed. I think the main reason airprime can't
match 5Mbits/sec is copying of data into tty in the read callback. I use
specially crafted ring buffer of chars, memory from which is directly
passed to the single read URB, then I immediately resubmit the URB (with
the next chunk of memory from the ring buffer) in the read callback and
schedule tasklet to copy from the ring buffer to tty. This solution is
there from the old days when USB core didn't like submissions of
multiple URBs. Now I think a better approach could be to have a FIFO of
URBs along with their buffers where you put URBs in the read callback,
then get them in the tasklet, copy to tty, and re-submit. I didn't have
time to play with the latter approach though.

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> Ar Llu, 2006-07-10 am 19:54 +0400, ysgrifennodd Sergei Organov:
>> Wow! The code you've quoted seems to be correct! But where did you get
>> it from? The version of flush_to_ldisc() from drivers/char/tty_io.c from
>> 2.17.4 got last Friday from kernel.org does this:
>
>> From HEAD so it should make 2.6.18. Paul fixed this one.

I've tested my driver with 2.6.18-rc1 and can confirm Paul changes
finally fixed this particular problem. However, this fix unveiled
different problem with the current tty buffering code.

The memory amount that could be used by tty buffers is
uncontrolled. I've arranged a test[1] that demonstrates that tty buffers
do indeed grow to the entire size of available memory at some conditions
resulting in kernel starting to kill random processes until the testing
process is killed.

This problem may occur with any tty driver that doesn't stop to insert
data into the tty buffers in throttled state. And yes, there are such
drivers in the tree. Before Paul's changes, the tty just dropped bytes
that aren't accepted by ldisc, so this problem had no chances to arise.

I think that either the amount of memory used by tty buffers should be
limited by a tty variable with a suitable default, or tty buffering
should be changed not to accept data from drivers in throttled state.
Alternatively, tty may rely on drivers not to insert data in the
throttled state, but then it's better to be written in big red letters
in the description of tty buffer interface routines. Though in the 2
latter cases drivers that insert too much data without pushing to ldisc
may cause similar problem. Anyway, you definitely know better what to do
about it.

[1] I have a USB device streaming data on its bulk endpoint. The device
is handled by improved airprime driver (usb-to-tty converter) that could
be found in Greg's patches. The driver attached the device to
/dev/ttyUSB0. The testing application just opened /dev/ttyUSB0 and went
to sleep, i.e., it just didn't read from the resulting fd. At these
conditions the slab memory reported in /proc/meminfo grew linearly in
time until in a few minutes kernel started to kill processes. The
testing process wasn't the first one the kernel killed. It killed X
server and some other applications until eventually it killed the
testing process at which point things returned to normal operation.

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Alan Cox]

On Iau, 2006-07-13 at 18:17 +0400, Sergei Organov wrote:
> This problem may occur with any tty driver that doesn't stop to insert
> data into the tty buffers in throttled state. And yes, there are such
> drivers in the tree. Before Paul's changes, the tty just dropped bytes
> that aren't accepted by ldisc, so this problem had no chances to arise.

You must honour throttle. That has always been the case. At all times
you should attempt to homour tty->receive_room and also ->throttle. If
you don't it breaks. There will always be some "reaction time" overruns.

> latter cases drivers that insert too much data without pushing to ldisc
> may cause similar problem. Anyway, you definitely know better what to do
> about it.

Might be a good idea to put a limiter in before 2.6.18 proper just to
trap any other drivers that have that bug. At least printk a warning and
refuse the allocation once there is say 64K queued. That way the driver
author gets a hint all is not well.

Alan

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> On Iau, 2006-07-13 at 18:17 +0400, Sergei Organov wrote:
>> This problem may occur with any tty driver that doesn't stop to insert
>> data into the tty buffers in throttled state. And yes, there are such
>> drivers in the tree. Before Paul's changes, the tty just dropped bytes
>> that aren't accepted by ldisc, so this problem had no chances to arise.
>
> You must honour throttle. 

I do, it's Greg who doesn't ;) BTW, isn't it OK to just check for
tty->throttled where appropriate if I don't have anything special to do
at unthrottled/throttled transition time?

> That has always been the case. At all times you should attempt to
> homour tty->receive_room and also ->throttle. If you don't it breaks.

Yes, but the difference is what "it" actually is. Loosing some
characters at some rare or "might never in fact happen conditions" is
one "it", and exhausting kernel memory at (even more) rare conditions is
a different "it", isn't it?

Besides, if the throttle() is that important and failure to handle it is
a big mistake, why is it optional then? I mean why struct tty_operations
with throttle field set to NULL is accepted in the first place? The same
question is applicable to the struct usb_serial_driver.

>> latter cases drivers that insert too much data without pushing to ldisc
>> may cause similar problem. Anyway, you definitely know better what to do
>> about it.
>
> Might be a good idea to put a limiter in before 2.6.18 proper just to
> trap any other drivers that have that bug. At least printk a warning and
> refuse the allocation once there is say 64K queued. That way the driver
> author gets a hint all is not well.

I'm afraid that the limit won't work well as a hint for driver
developers that didn't honour throttle, as real applications do usually
read from the files they open, and therefore the problem most probably
won't be noticed for a long time.

Provided the limiter is put, why not to make it a variable with 64K
default?  Driver writers that for whatever reason decide they need more
in buffers will be able to change that, but then it will be their
deliberate decision, not just underestimation of consequences of failure
to handle throttle() due to a lack of knowledge.

Actually I think that the first thing to decide is if memory usage by
tty should be bounded or not, and if yes, should it be per-tty limit, or
total memory usage by all the ttys limit, or both. Those decisions I'd
probably base on how other kernel subsystems behave (TCP stack is the
first that comes to mind, and AFAIK buffering for every socket is
limited). Due to lack of broad knowledge of the kernel, I won't try to
insist on any solution, even though my experience in embedded systems
programming cries for bounded model.

And at the end, I'm going to RTFM ;)

The comment to the throttle routine in the kernel tree says:

 * 	This routine notifies the tty driver that input buffers for
 * 	the line discipline are close to full, and it should somehow
 * 	signal that no more characters should be sent to the tty.

"Linux Device Drivers" 3-d edition says:

 The throttle function is called when the tty core’s input buffers are
 getting full. The tty driver should try to signal to the device that
 no more characters should be sent to it.

None of these two suggests there could be such a global consequences of
attempting to insert data to the throttled tty as exhausted kernel
memory. The kernel version reads more strict to me, but LDD one is
apparently how people indeed understand it.

BTW, I'm curious if Greg wasn't aware throttle must be handled, or just
decided that it's not worth to, as neither generic nor airprime
usb-serial drivers handle throttle. Besides, the example tiny_tty.c
driver from the LDD doesn't handle throttle either. If even most
experienced and involved kernel developers do ignore the throttle(),
what should be expected from random Joe driver writer?

-- 
Sergei.

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Greg KH]

On Thu, Jul 13, 2006 at 10:20:19PM +0400, Sergei Organov wrote:
> Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> > On Iau, 2006-07-13 at 18:17 +0400, Sergei Organov wrote:
> >> This problem may occur with any tty driver that doesn't stop to insert
> >> data into the tty buffers in throttled state. And yes, there are such
> >> drivers in the tree. Before Paul's changes, the tty just dropped bytes
> >> that aren't accepted by ldisc, so this problem had no chances to arise.
> >
> > You must honour throttle. 
> 
> I do, it's Greg who doesn't ;) BTW, isn't it OK to just check for
> tty->throttled where appropriate if I don't have anything special to do
> at unthrottled/throttled transition time?
> 
> > That has always been the case. At all times you should attempt to
> > homour tty->receive_room and also ->throttle. If you don't it breaks.
> 
> Yes, but the difference is what "it" actually is. Loosing some
> characters at some rare or "might never in fact happen conditions" is
> one "it", and exhausting kernel memory at (even more) rare conditions is
> a different "it", isn't it?
> 
> Besides, if the throttle() is that important and failure to handle it is
> a big mistake, why is it optional then? I mean why struct tty_operations
> with throttle field set to NULL is accepted in the first place? The same
> question is applicable to the struct usb_serial_driver.

Yes, I didn't realize it was required.  If it is, we should add this
change.

> >> latter cases drivers that insert too much data without pushing to ldisc
> >> may cause similar problem. Anyway, you definitely know better what to do
> >> about it.
> >
> > Might be a good idea to put a limiter in before 2.6.18 proper just to
> > trap any other drivers that have that bug. At least printk a warning and
> > refuse the allocation once there is say 64K queued. That way the driver
> > author gets a hint all is not well.
> 
> I'm afraid that the limit won't work well as a hint for driver
> developers that didn't honour throttle, as real applications do usually
> read from the files they open, and therefore the problem most probably
> won't be noticed for a long time.
> 
> Provided the limiter is put, why not to make it a variable with 64K
> default?  Driver writers that for whatever reason decide they need more
> in buffers will be able to change that, but then it will be their
> deliberate decision, not just underestimation of consequences of failure
> to handle throttle() due to a lack of knowledge.
> 
> Actually I think that the first thing to decide is if memory usage by
> tty should be bounded or not, and if yes, should it be per-tty limit, or
> total memory usage by all the ttys limit, or both. Those decisions I'd
> probably base on how other kernel subsystems behave (TCP stack is the
> first that comes to mind, and AFAIK buffering for every socket is
> limited). Due to lack of broad knowledge of the kernel, I won't try to
> insist on any solution, even though my experience in embedded systems
> programming cries for bounded model.
> 
> And at the end, I'm going to RTFM ;)
> 
> The comment to the throttle routine in the kernel tree says:
> 
>  * 	This routine notifies the tty driver that input buffers for
>  * 	the line discipline are close to full, and it should somehow
>  * 	signal that no more characters should be sent to the tty.
> 
> "Linux Device Drivers" 3-d edition says:
> 
>  The throttle function is called when the tty core???s input buffers are
>  getting full. The tty driver should try to signal to the device that
>  no more characters should be sent to it.
> 
> None of these two suggests there could be such a global consequences of
> attempting to insert data to the throttled tty as exhausted kernel
> memory. The kernel version reads more strict to me, but LDD one is
> apparently how people indeed understand it.

Well, as I wrote that chapter in LDD, that was how I understood it :)

> BTW, I'm curious if Greg wasn't aware throttle must be handled, or just
> decided that it's not worth to, as neither generic nor airprime
> usb-serial drivers handle throttle.

I wasn't aware that it was required.

> Besides, the example tiny_tty.c driver from the LDD doesn't handle
> throttle either.

I wrote that too :)

thanks,

greg k-h

Re: [PATCH] Airprime driver improvements to allow full speed EvDO transfers

[Sergei Organov]

Greg KH <gregkh@suse.de> writes:
> On Thu, Jul 13, 2006 at 10:20:19PM +0400, Sergei Organov wrote:
[...]
>> Besides, if the throttle() is that important and failure to handle it is
>> a big mistake, why is it optional then? I mean why struct tty_operations
>> with throttle field set to NULL is accepted in the first place? The same
>> question is applicable to the struct usb_serial_driver.
>
> Yes, I didn't realize it was required.

That was my point, -- people don't realize it is dangerous not to handle
throttle, and in fact it was not that dangerous before new tty buffering
has been implemented. I do handle throttle in my driver, but it has been
implemented as a part of work-around for the deficiencies of the old tty
buffering. I seriously doubt I'd realize it's a must to honour throttle
should I start to write the driver today.

-- 
Sergei.