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

["Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>]

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