On Tue, May 30, 2006 at 11:38:01AM -0300, Luiz Fernando N. Capitulino wrote:
> On Tue, 30 May 2006 10:21:41 +0200
> Frank Gevaerts <[email protected]> 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
> | > 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..
Since it is bulk, it is not guaranteed to start before the control
message anyway, so it should be safe.
The patch looks correct to me, but I would still like to increase
KP_RETRIES a bit. If I read the code correctly, the current setup allows
for 10 seconds between usb connect and acknowledging the control
message. This is enough if the device is only connected when booted
(which is the normal use case). However, in our case, we do
software-initiated reboots of the ipaq while it is attached to the usb
bus, which can take much longer, so for us KP_RETRIES should be at least
1000, maybe 2000. Of course, we can always run a patched kernel for this.
I'll test the patch later today.
Anyway, we have not seen the usb_serial_disconnect bug since applying
your patch, so that bug is also probably gone (we have had nearly 1000
successfull connects/disconnects since then)
Frank
> 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
--
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]
