[RESEND] [PATCH 1/2] ipaq.c bugfixes

[RESEND] [PATCH 1/2] ipaq.c bugfixes

[Frank Gevaerts]

This patch fixes several problems in the ipaq.c driver with connecting
and disconnecting pocketpc devices:
* The read urb stayed active if the connect failed, causing nullpointer
  dereferences later on.
* If a write failed, the driver continued as if nothing happened. Now it
  handles that case the same way as other usb serial devices (fix by
  Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>)

Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>

diff -urp linux-2.6.17-rc6/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c
--- linux-2.6.17-rc6/drivers/usb/serial/ipaq.c	2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
@@ -652,11 +652,6 @@ static int ipaq_open(struct usb_serial_p
 		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
 		      port->read_urb->transfer_buffer, port->read_urb->transfer_buffer_length,
 		      ipaq_read_bulk_callback, port);
-	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
-	if (result) {
-		err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
-		goto error;
-	}
 
 	/*
 	 * Send out control message observed in win98 sniffs. Not sure what
@@ -671,6 +666,11 @@ static int ipaq_open(struct usb_serial_p
 				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
 				0x1, 0, NULL, 0, 100);
 		if (result == 0) {
+			result = usb_submit_urb(port->read_urb, GFP_KERNEL);
+			if (result) {
+				err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
+				goto error;
+			}
 			return 0;
 		}
 	}
@@ -855,6 +855,7 @@ static void ipaq_write_bulk_callback(str
 	
 	if (urb->status) {
 		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, urb->status);
+		return;
 	}
 
 	spin_lock_irqsave(&write_list_lock, flags);

-- 
Frank Gevaerts                                 frank.gevaerts@fks.be
fks bvba - Formal and Knowledge Systems        http://www.fks.be/
Stationsstraat 108                             Tel:  ++32-(0)11-21 49 11
B-3570 ALKEN                                   Fax:  ++32-(0)11-22 04 19

[RESEND] [PATCH 2/2] ipaq.c timing parameters

[Frank Gevaerts]

Adds configurable waiting periods to the ipaq connection code. These are
not needed when the pocketpc device is running normally when plugged in,
but they need extra delays if they are physically connected while
rebooting.
There are two parameters :
* initial_wait : this is the delay before the driver attemts to start the
  connection. This is needed because the pocktpc device takes much
  longer to boot if the driver starts sending control packets too soon.
* connect_retries : this is the number of times the control urb is
  retried before finally giving up. The patch also adds a 1 second delay
  between retries.
I'm not sure if the cases where this patch is useful are general enough
to include this in the kernel.

Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>

diff -urp linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c
--- linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
+++ linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c	2006-06-14 16:06:44.000000000 +0200
@@ -71,6 +71,8 @@
 
 static __u16 product, vendor;
 static int debug;
+static int connect_retries = KP_RETRIES;
+static int initial_wait;
 
 /* Function prototypes for an ipaq */
 static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
@@ -583,7 +585,7 @@ static int ipaq_open(struct usb_serial_p
 	struct ipaq_private	*priv;
 	struct ipaq_packet	*pkt;
 	int			i, result = 0;
-	int			retries = KP_RETRIES;
+	int			retries = connect_retries;
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
@@ -647,6 +649,7 @@ static int ipaq_open(struct usb_serial_p
 	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
 	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
 	
+	msleep(1000*initial_wait);
 	/* Start reading from the device */
 	usb_fill_bulk_urb(port->read_urb, serial->dev, 
 		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
@@ -673,6 +676,7 @@ static int ipaq_open(struct usb_serial_p
 			}
 			return 0;
 		}
+		msleep(1000);
 	}
 	err("%s - failed doing control urb, error %d", __FUNCTION__, result);
 	goto error;
@@ -968,3 +972,9 @@ MODULE_PARM_DESC(vendor, "User specified
 
 module_param(product, ushort, 0);
 MODULE_PARM_DESC(product, "User specified USB idProduct");
+
+module_param(connect_retries, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(connect_retries, "Maximum number of connect retries (100ms each)");
+
+module_param(initial_wait, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(initial_wait, "Time to wait before attempting a connection (in seconds)");

-- 
Frank Gevaerts                                 frank.gevaerts@fks.be
fks bvba - Formal and Knowledge Systems        http://www.fks.be/
Stationsstraat 108                             Tel:  ++32-(0)11-21 49 11
B-3570 ALKEN                                   Fax:  ++32-(0)11-22 04 19

Re: [RESEND] [PATCH 2/2] ipaq.c timing parameters

[Luiz Fernando N. Capitulino]

On Mon, 19 Jun 2006 10:46:19 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| Adds configurable waiting periods to the ipaq connection code. These are
| not needed when the pocketpc device is running normally when plugged in,
| but they need extra delays if they are physically connected while
| rebooting.
| There are two parameters :
| * initial_wait : this is the delay before the driver attemts to start the
|   connection. This is needed because the pocktpc device takes much
|   longer to boot if the driver starts sending control packets too soon.
| * connect_retries : this is the number of times the control urb is
|   retried before finally giving up. The patch also adds a 1 second delay
|   between retries.
| I'm not sure if the cases where this patch is useful are general enough
| to include this in the kernel.
| 
| Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>
| 
| diff -urp linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c
| --- linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
| +++ linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c	2006-06-14 16:06:44.000000000 +0200
| @@ -71,6 +71,8 @@
|  
|  static __u16 product, vendor;
|  static int debug;
| +static int connect_retries = KP_RETRIES;
| +static int initial_wait;
|  
|  /* Function prototypes for an ipaq */
|  static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
| @@ -583,7 +585,7 @@ static int ipaq_open(struct usb_serial_p
|  	struct ipaq_private	*priv;
|  	struct ipaq_packet	*pkt;
|  	int			i, result = 0;
| -	int			retries = KP_RETRIES;
| +	int			retries = connect_retries;
|  
|  	dbg("%s - port %d", __FUNCTION__, port->number);
|  
| @@ -647,6 +649,7 @@ static int ipaq_open(struct usb_serial_p
|  	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
|  	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
|  	
| +	msleep(1000*initial_wait);

 I was going to say you should use ssleep() here, but I can't find a
ssleep_interruptible(). Then either: use msleep_interruptible() or
creates a new ssleep_interruptible().

|  	/* Start reading from the device */
|  	usb_fill_bulk_urb(port->read_urb, serial->dev, 
|  		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
| @@ -673,6 +676,7 @@ static int ipaq_open(struct usb_serial_p
|  			}
|  			return 0;
|  		}
| +		msleep(1000);
|  	}

 Don't you want msleep(100); here?

-- 
Luiz Fernando N. Capitulino

Re: [RESEND] [PATCH 2/2] ipaq.c timing parameters

[Frank Gevaerts]

On Mon, Jun 19, 2006 at 01:42:40PM -0300, Luiz Fernando N. Capitulino wrote:
> On Mon, 19 Jun 2006 10:46:19 +0200
> Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> | Adds configurable waiting periods to the ipaq connection code. These are
> | not needed when the pocketpc device is running normally when plugged in,
> | but they need extra delays if they are physically connected while
> | rebooting.
> | There are two parameters :
> | * initial_wait : this is the delay before the driver attemts to start the
> |   connection. This is needed because the pocktpc device takes much
> |   longer to boot if the driver starts sending control packets too soon.
> | * connect_retries : this is the number of times the control urb is
> |   retried before finally giving up. The patch also adds a 1 second delay
> |   between retries.
> | I'm not sure if the cases where this patch is useful are general enough
> | to include this in the kernel.
> | 
> | Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>
> | 
> | diff -urp linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c
> | --- linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
> | +++ linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c	2006-06-14 16:06:44.000000000 +0200
> | @@ -71,6 +71,8 @@
> |  
> |  static __u16 product, vendor;
> |  static int debug;
> | +static int connect_retries = KP_RETRIES;
> | +static int initial_wait;
> |  
> |  /* Function prototypes for an ipaq */
> |  static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
> | @@ -583,7 +585,7 @@ static int ipaq_open(struct usb_serial_p
> |  	struct ipaq_private	*priv;
> |  	struct ipaq_packet	*pkt;
> |  	int			i, result = 0;
> | -	int			retries = KP_RETRIES;
> | +	int			retries = connect_retries;
> |  
> |  	dbg("%s - port %d", __FUNCTION__, port->number);
> |  
> | @@ -647,6 +649,7 @@ static int ipaq_open(struct usb_serial_p
> |  	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
> |  	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
> |  	
> | +	msleep(1000*initial_wait);
> 
>  I was going to say you should use ssleep() here, but I can't find a
> ssleep_interruptible(). Then either: use msleep_interruptible() or
> creates a new ssleep_interruptible().

I wasn't sure if that was safe here, so I used the non-interruptible
version. I'll change it when I redo the patch.
Is it worth it creating ssleep_interruptible() just for this one call?

> |  	/* Start reading from the device */
> |  	usb_fill_bulk_urb(port->read_urb, serial->dev, 
> |  		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
> | @@ -673,6 +676,7 @@ static int ipaq_open(struct usb_serial_p
> |  			}
> |  			return 0;
> |  		}
> | +		msleep(1000);
> |  	}
> 
>  Don't you want msleep(100); here?

The currently running version has 1000. 100 is probably better.

I'll submit a new patch once it's clear which version of the first patch
goes in.

> 
> -- 
> Luiz Fernando N. Capitulino

-- 
Frank Gevaerts                                 frank.gevaerts@fks.be
fks bvba - Formal and Knowledge Systems        http://www.fks.be/
Stationsstraat 108                             Tel:  ++32-(0)11-21 49 11
B-3570 ALKEN                                   Fax:  ++32-(0)11-22 04 19

Re: [RESEND] [PATCH 2/2] ipaq.c timing parameters

[Luiz Fernando N. Capitulino]

On Mon, 19 Jun 2006 19:34:28 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| On Mon, Jun 19, 2006 at 01:42:40PM -0300, Luiz Fernando N. Capitulino wrote:
| > On Mon, 19 Jun 2006 10:46:19 +0200
| > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| > 
| > | Adds configurable waiting periods to the ipaq connection code. These are
| > | not needed when the pocketpc device is running normally when plugged in,
| > | but they need extra delays if they are physically connected while
| > | rebooting.
| > | There are two parameters :
| > | * initial_wait : this is the delay before the driver attemts to start the
| > |   connection. This is needed because the pocktpc device takes much
| > |   longer to boot if the driver starts sending control packets too soon.
| > | * connect_retries : this is the number of times the control urb is
| > |   retried before finally giving up. The patch also adds a 1 second delay
| > |   between retries.
| > | I'm not sure if the cases where this patch is useful are general enough
| > | to include this in the kernel.
| > | 
| > | Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>
| > | 
| > | diff -urp linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c
| > | --- linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
| > | +++ linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c	2006-06-14 16:06:44.000000000 +0200
| > | @@ -71,6 +71,8 @@
| > |  
| > |  static __u16 product, vendor;
| > |  static int debug;
| > | +static int connect_retries = KP_RETRIES;
| > | +static int initial_wait;
| > |  
| > |  /* Function prototypes for an ipaq */
| > |  static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
| > | @@ -583,7 +585,7 @@ static int ipaq_open(struct usb_serial_p
| > |  	struct ipaq_private	*priv;
| > |  	struct ipaq_packet	*pkt;
| > |  	int			i, result = 0;
| > | -	int			retries = KP_RETRIES;
| > | +	int			retries = connect_retries;
| > |  
| > |  	dbg("%s - port %d", __FUNCTION__, port->number);
| > |  
| > | @@ -647,6 +649,7 @@ static int ipaq_open(struct usb_serial_p
| > |  	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
| > |  	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
| > |  	
| > | +	msleep(1000*initial_wait);
| > 
| >  I was going to say you should use ssleep() here, but I can't find a
| > ssleep_interruptible(). Then either: use msleep_interruptible() or
| > creates a new ssleep_interruptible().
| 
| I wasn't sure if that was safe here, so I used the non-interruptible
| version. I'll change it when I redo the patch.
| Is it worth it creating ssleep_interruptible() just for this one call?

 You could grep around to check it.

-- 
Luiz Fernando N. Capitulino

Re: [RESEND] [PATCH 2/2] ipaq.c timing parameters

[Andrew Morton]

On Mon, 19 Jun 2006 10:46:19 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

> Adds configurable waiting periods to the ipaq connection code. These are
> not needed when the pocketpc device is running normally when plugged in,
> but they need extra delays if they are physically connected while
> rebooting.
> There are two parameters :
> * initial_wait : this is the delay before the driver attemts to start the
>   connection. This is needed because the pocktpc device takes much
>   longer to boot if the driver starts sending control packets too soon.
> * connect_retries : this is the number of times the control urb is
>   retried before finally giving up. The patch also adds a 1 second delay
>   between retries.
> I'm not sure if the cases where this patch is useful are general enough
> to include this in the kernel.
> 
> ..
>
> +module_param(connect_retries, int, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(connect_retries, "Maximum number of connect retries (100ms each)");

1000ms, methinks.

Re: [RESEND] [PATCH 1/2] ipaq.c bugfixes

[Luiz Fernando N. Capitulino]

On Mon, 19 Jun 2006 10:44:47 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| This patch fixes several problems in the ipaq.c driver with connecting
| and disconnecting pocketpc devices:
| * The read urb stayed active if the connect failed, causing nullpointer
|   dereferences later on.
| * If a write failed, the driver continued as if nothing happened. Now it
|   handles that case the same way as other usb serial devices (fix by
|   Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>)
| 
| Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>
| 
| diff -urp linux-2.6.17-rc6/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c
| --- linux-2.6.17-rc6/drivers/usb/serial/ipaq.c	2006-03-20 06:53:29.000000000 +0100
| +++ linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
| @@ -652,11 +652,6 @@ static int ipaq_open(struct usb_serial_p
|  		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
|  		      port->read_urb->transfer_buffer, port->read_urb->transfer_buffer_length,
|  		      ipaq_read_bulk_callback, port);
| -	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
| -	if (result) {
| -		err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
| -		goto error;
| -	}
|  
|  	/*
|  	 * Send out control message observed in win98 sniffs. Not sure what
| @@ -671,6 +666,11 @@ static int ipaq_open(struct usb_serial_p
|  				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
|  				0x1, 0, NULL, 0, 100);
|  		if (result == 0) {
| +			result = usb_submit_urb(port->read_urb, GFP_KERNEL);
| +			if (result) {
| +				err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
| +				goto error;
| +			}
|  			return 0;
|  		}
|  	}

 What do you think about this (not compiled and may be wrong):

diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
index 9a5c979..96a6550 100644
--- a/drivers/usb/serial/ipaq.c
+++ b/drivers/usb/serial/ipaq.c
@@ -646,17 +646,6 @@ static int ipaq_open(struct usb_serial_p
 	port->write_urb->transfer_buffer = port->bulk_out_buffer;
 	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
 	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
-	
-	/* Start reading from the device */
-	usb_fill_bulk_urb(port->read_urb, serial->dev, 
-		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
-		      port->read_urb->transfer_buffer, port->read_urb->transfer_buffer_length,
-		      ipaq_read_bulk_callback, port);
-	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
-	if (result) {
-		err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
-		goto error;
-	}
 
 	/*
 	 * Send out control message observed in win98 sniffs. Not sure what
@@ -670,12 +659,27 @@ static int ipaq_open(struct usb_serial_p
 		result = usb_control_msg(serial->dev,
 				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
 				0x1, 0, NULL, 0, 100);
-		if (result == 0) {
-			return 0;
-		}
+		if (!result)
+			break;
 	}
-	err("%s - failed doing control urb, error %d", __FUNCTION__, result);
-	goto error;
+	if (result) {
+		err("%s - failed doing control urb, error %d", __FUNCTION__,
+		    result);
+		goto error;
+	}
+
+	/* Start reading from the device */
+	usb_fill_bulk_urb(port->read_urb, serial->dev, 
+		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
+		      port->read_urb->transfer_buffer, port->read_urb->transfer_buffer_length,
+		      ipaq_read_bulk_callback, port);
+	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
+	if (result) {
+		err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
+		goto error;
+	}
+
+	return 0;
 
 enomem:
 	result = -ENOMEM;

 This makes the code more readable than your version, IMHO.

 Greg, what do you think about this patch? I think it makes sense
because besides Frank's tests there's a comment stating that the
device only starts the chat sequence after one of these control
messages gets through.

-- 
Luiz Fernando N. Capitulino

Re: [RESEND] [PATCH 1/2] ipaq.c bugfixes

[Frank Gevaerts]

On Mon, Jun 19, 2006 at 01:35:31PM -0300, Luiz Fernando N. Capitulino wrote:
> On Mon, 19 Jun 2006 10:44:47 +0200
> Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> | This patch fixes several problems in the ipaq.c driver with connecting
> | and disconnecting pocketpc devices:
> | * The read urb stayed active if the connect failed, causing nullpointer
> |   dereferences later on.
> | * If a write failed, the driver continued as if nothing happened. Now it
> |   handles that case the same way as other usb serial devices (fix by
> |   Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>)
> | 
> | Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>
> | 
> | diff -urp linux-2.6.17-rc6/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c
> | --- linux-2.6.17-rc6/drivers/usb/serial/ipaq.c	2006-03-20 06:53:29.000000000 +0100
> | +++ linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
> | @@ -652,11 +652,6 @@ static int ipaq_open(struct usb_serial_p
> |  		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
> |  		      port->read_urb->transfer_buffer, port->read_urb->transfer_buffer_length,
> |  		      ipaq_read_bulk_callback, port);
> | -	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
> | -	if (result) {
> | -		err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
> | -		goto error;
> | -	}
> |  
> |  	/*
> |  	 * Send out control message observed in win98 sniffs. Not sure what
> | @@ -671,6 +666,11 @@ static int ipaq_open(struct usb_serial_p
> |  				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
> |  				0x1, 0, NULL, 0, 100);
> |  		if (result == 0) {
> | +			result = usb_submit_urb(port->read_urb, GFP_KERNEL);
> | +			if (result) {
> | +				err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
> | +				goto error;
> | +			}
> |  			return 0;
> |  		}
> |  	}
> 
>  What do you think about this (not compiled and may be wrong):
> 
> diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
> index 9a5c979..96a6550 100644
> --- a/drivers/usb/serial/ipaq.c
> +++ b/drivers/usb/serial/ipaq.c
> @@ -646,17 +646,6 @@ static int ipaq_open(struct usb_serial_p
>  	port->write_urb->transfer_buffer = port->bulk_out_buffer;
>  	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
>  	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
> -	
> -	/* Start reading from the device */
> -	usb_fill_bulk_urb(port->read_urb, serial->dev, 
> -		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
> -		      port->read_urb->transfer_buffer, port->read_urb->transfer_buffer_length,
> -		      ipaq_read_bulk_callback, port);
> -	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
> -	if (result) {
> -		err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
> -		goto error;
> -	}
>  
>  	/*
>  	 * Send out control message observed in win98 sniffs. Not sure what
> @@ -670,12 +659,27 @@ static int ipaq_open(struct usb_serial_p
>  		result = usb_control_msg(serial->dev,
>  				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
>  				0x1, 0, NULL, 0, 100);
> -		if (result == 0) {
> -			return 0;
> -		}
> +		if (!result)
> +			break;
>  	}
> -	err("%s - failed doing control urb, error %d", __FUNCTION__, result);
> -	goto error;
> +	if (result) {
> +		err("%s - failed doing control urb, error %d", __FUNCTION__,
> +		    result);
> +		goto error;
> +	}
> +
> +	/* Start reading from the device */
> +	usb_fill_bulk_urb(port->read_urb, serial->dev, 
> +		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
> +		      port->read_urb->transfer_buffer, port->read_urb->transfer_buffer_length,
> +		      ipaq_read_bulk_callback, port);
> +	result = usb_submit_urb(port->read_urb, GFP_KERNEL);
> +	if (result) {
> +		err("%s - failed submitting read urb, error %d", __FUNCTION__, result);
> +		goto error;
> +	}
> +
> +	return 0;
>  
>  enomem:
>  	result = -ENOMEM;
> 
>  This makes the code more readable than your version, IMHO.

It is more readable. It compiles, and it looks equivalent to me.
Unfortunately, I don't have easy access to the test setup anymore
(everything is now at the customer site), so I'm not sure if I can test
this anytime soon.

Frank

>  Greg, what do you think about this patch? I think it makes sense
> because besides Frank's tests there's a comment stating that the
> device only starts the chat sequence after one of these control
> messages gets through.
> 
> -- 
> Luiz Fernando N. Capitulino

-- 
Frank Gevaerts                                 frank.gevaerts@fks.be
fks bvba - Formal and Knowledge Systems        http://www.fks.be/
Stationsstraat 108                             Tel:  ++32-(0)11-21 49 11
B-3570 ALKEN                                   Fax:  ++32-(0)11-22 04 19