Messages in this thread | | | Date | Mon, 19 Jun 2006 19:25:36 +0200 | From | Frank Gevaerts <> | Subject | Re: [RESEND] [PATCH 1/2] ipaq.c bugfixes |
| |
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 - 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/
| |