lkml.org 
[lkml]   [2006]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RESEND] [PATCH 1/2] ipaq.c bugfixes
    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
    -
    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/

    \
     
     \ /
      Last update: 2006-06-19 18:38    [W:0.038 / U:0.356 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site