Re: usb-serial ipaq kernel problem

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

On Tue, 30 May 2006 10:21:41 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| On Mon, May 29, 2006 at 07:33:30PM -0300, Luiz Fernando N. Capitulino wrote:
| > On Mon, 29 May 2006 22:47:24 +0200
| > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| > | 
| > | The panic was caused by the read urb being submitten in ipaq_open,
| > | regardless of success, and never killed in case of failure. What my
| > | patch basically does is to submit the urb only after succesfully sending
| > | the control message, and adding a sleep between tries. As long as this
| > | patch is not applied, we hardly get any other error because the kernel
| > | panics as soon as an ipaq reboots.
| > 
| >  I see.
| > 
| >  Did you try to just kill the read urb in the ipaq_open's error path?
| 
| Yes, that's what I did at first. It works, but with the long waits (we see
| waits up to 80-90 seconds right now) I was afraid that the urb might timeout
| before the control message succeeds.

 Hmmm, I see.

 My only (obvious) question is that if it's really safe to send the read
urb after the control message..

 Greg, are you following this thread? WDT?

 Anyways, if it's safe to do it, I do prefer the following (not tested)
version. Which is cleaner, IMO.

------------

diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
index 9a5c979..9333169 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
@@ -665,17 +654,31 @@ static int ipaq_open(struct usb_serial_p
 	 * through. Since this has a reasonably high failure rate, we retry
 	 * several times.
 	 */
-
 	while (retries--) {
 		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;
+	}
+	if (result) {
+		err("%s - failed doing control urb, error %d", __FUNCTION__,
+				result);
+		goto error;
 	}
-	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;


-- 
Luiz Fernando N. Capitulino