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 <[email protected]> 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 <[email protected]>)
> |
> | Signed-off-by: Frank Gevaerts <[email protected]>
> |
> | 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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
