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

From: Luiz Fernando N. Capitulino
Date: Mon Jun 19 2006 - 12:34:25 EST


On Mon, 19 Jun 2006 10:44:47 +0200
Frank Gevaerts <frank.gevaerts@xxxxxx> 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@xxxxxxxxxxxxxxx>)
|
| Signed-off-by: Frank Gevaerts <frank.gevaerts@xxxxxx>
|
| 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/