usb-serial ipaq kernel problem

usb-serial ipaq kernel problem

[Frank Gevaerts]

Hi, 

We are having problems with the usb-serial ipaq driver in 2.6.16 (debian
backports 2.6.16-1-686, but also reproducible with self-compiled
kernel.org kernel)

Sometimes, we get the following on disconnect:

Unable to handle kernel paging request at virtual address 723d4ec3
 printing eip:
b01ea93d
*pde = 00000000
Oops: 0000 [#1]
Modules linked in: ppp_async crc_ccitt ppp_generic slhc ipaq usbserial uhci_hcd ohci_hcd usbhid ehci_hcd usbcore 8139too mii sr_mod sbp2 scsi_mod ieee1394 psmouse ide_generic ide_cd cdrom genrtc ext3 jbd mbcache ide_disk generic via82cxxx ide_core evdev mousedev
CPU:    0
EIP:    0060:[<b01ea93d>]    Not tainted VLI
EFLAGS: 00010246   (2.6.16-1-686 #2)
EIP is at check_tty_count+0x33/0x7a
eax: 723d4e4f   ebx: 00000000   ecx: c97cb000   edx: c97cb170
esi: cbfe55a0   edi: 00000283   ebp: c97cb000   esp: cbe87f18
ds: 007b   es: 007b   ss: 0068
Process events/0 (pid: 4, threadinfo=cbe86000 task=cbfdfab0)
Stack: <0>c97cb000 b01eb558 c97cb000 b029719d 00000000 00000000 00000000 c97cb13c
       cbfe55a0 00000283 c97cb000 b012277d c97cb000 b01eb507 cbe86000 cbe87f84
       cbe87fa4 cbfe55a0 b01228a9 cbfe55a0 ffffffff ffffffff 00000001 00000000
Call Trace:
 [<b01eb558>] do_tty_hangup+0x51/0x294
 [<b012277d>] run_workqueue+0x6e/0xa2
 [<b01eb507>] do_tty_hangup+0x0/0x294
 [<b01228a9>] worker_thread+0xf8/0x12a
 [<b01138c3>] default_wake_function+0x0/0x12
 [<b026dabd>] schedule+0x45f/0x4cd
 [<b01138c3>] default_wake_function+0x0/0x12
 [<b01227b1>] worker_thread+0x0/0x12a
 [<b0124ece>] kthread+0x79/0xa3
 [<b0124e55>] kthread+0x0/0xa3
 [<b01012cd>] kernel_thread_helper+0x5/0xb
Code: 91 70 01 00 00 8b 02 0f 18 00 90 8d 81 70 01 00 00 39 c2 74 13 8b 12 43 8b 02 0f 18 00 90 8d 81 70 01 00 00 39 c2 75 ed 8b 41 04 <81> 78 74 04 00 02 00 75 17 8b 91 cc 00 00 00 85 d2 74 0d 83 ba

I don't know enough about the tty system to find the problem, but it
seems to me that the tty structure might be released too soon.
We have a hotplug script that starts ppp on connect, and we can
reproduce this by repeatedly plugging and unplugging the ipaq.

Frank

-- 
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

Re: usb-serial ipaq kernel problem

[Greg KH]

On Mon, May 22, 2006 at 04:30:48PM +0200, Frank Gevaerts wrote:
> Hi, 
> 
> We are having problems with the usb-serial ipaq driver in 2.6.16 (debian
> backports 2.6.16-1-686, but also reproducible with self-compiled
> kernel.org kernel)
> 
> Sometimes, we get the following on disconnect:

<snip>

Can you duplicate this on 2.6.17-rc4?  A number of tty changes went into
that release that should have fixed this issue.

thanks,

greg k-h

[PATCH] Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Mon, May 22, 2006 at 02:44:03PM -0700, Greg KH wrote:
> On Mon, May 22, 2006 at 04:30:48PM +0200, Frank Gevaerts wrote:
> > Hi, 
> > 
> > We are having problems with the usb-serial ipaq driver in 2.6.16 (debian
> > backports 2.6.16-1-686, but also reproducible with self-compiled
> > kernel.org kernel)
> > 
> > Sometimes, we get the following on disconnect:
> 
> <snip>
> 
> Can you duplicate this on 2.6.17-rc4?  A number of tty changes went into
> that release that should have fixed this issue.

I'll try it in the morning. In the meantime, we found some other
problems in ipaq.c : apparently pocketpc accepts usb enumeration long
before it accepts usb-serial commands (sometimes 50 or more seconds),
which makes ipaq_open fail. When it fails, the read urb is not killed,
while the associated structures are freed, which gives a panic when
the urb completes. The following patch solves that :
Since changing this, I also have not seen the original problem anymore,
but I will do some more testing tomorrow.

Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>

--- linux-2.6.16/drivers/usb/serial/ipaq.c      2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.16.ipaq/drivers/usb/serial/ipaq.c 2006-05-23 00:00:33.000000000 +0200
@@ -59,7 +59,7 @@
 #include "usb-serial.h"
 #include "ipaq.h"
 
-#define KP_RETRIES     100
+#define KP_RETRIES     1000
 
 /*
  * Version Information
@@ -652,12 +652,6 @@
                      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
         * it does, but from empirical observations, it seems that the device
@@ -671,8 +665,15 @@
                                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;
                }
+                msleep(100);
        }
        err("%s - failed doing control urb, error %d", __FUNCTION__, result);
        goto error;

> 
> thanks,
> 
> greg k-h
> 

-- 
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

Re: [PATCH] Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Tue, May 23, 2006 at 12:04:59AM +0200, Frank Gevaerts wrote:
> On Mon, May 22, 2006 at 02:44:03PM -0700, Greg KH wrote:
> > On Mon, May 22, 2006 at 04:30:48PM +0200, Frank Gevaerts wrote:
> > > Hi, 
> > > 
> > > We are having problems with the usb-serial ipaq driver in 2.6.16 (debian
> > > backports 2.6.16-1-686, but also reproducible with self-compiled
> > > kernel.org kernel)
> > > 
> > > Sometimes, we get the following on disconnect:
> > 
> > <snip>
> > 
> > Can you duplicate this on 2.6.17-rc4?  A number of tty changes went into
> > that release that should have fixed this issue.
> 
> I'll try it in the morning. In the meantime, we found some other
> problems in ipaq.c : apparently pocketpc accepts usb enumeration long
> before it accepts usb-serial commands (sometimes 50 or more seconds),
> which makes ipaq_open fail. When it fails, the read urb is not killed,
> while the associated structures are freed, which gives a panic when
> the urb completes. The following patch solves that :
> Since changing this, I also have not seen the original problem anymore,
> but I will do some more testing tomorrow.

I cannot reproduce the original problem anymore with my patch applied

Frank

> 
> Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>
> 
> --- linux-2.6.16/drivers/usb/serial/ipaq.c      2006-03-20 06:53:29.000000000 +0100
> +++ linux-2.6.16.ipaq/drivers/usb/serial/ipaq.c 2006-05-23 00:00:33.000000000 +0200
> @@ -59,7 +59,7 @@
>  #include "usb-serial.h"
>  #include "ipaq.h"
>  
> -#define KP_RETRIES     100
> +#define KP_RETRIES     1000
>  
>  /*
>   * Version Information
> @@ -652,12 +652,6 @@
>                       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
>          * it does, but from empirical observations, it seems that the device
> @@ -671,8 +665,15 @@
>                                 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;
>                 }
> +                msleep(100);
>         }
>         err("%s - failed doing control urb, error %d", __FUNCTION__, result);
>         goto error;
> 
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> -- 
> 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

-- 
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

usb-serial ipaq kernel problem

[Frank Gevaerts]

Hi,

I got this when disconnecting an ipaq with 2,6,17rc4.

Frank

usb 1-4.5.7: USB disconnect, address 79
------------[ cut here ]------------
kernel BUG at kernel/workqueue.c:110!
invalid opcode: 0000 [#1]
Modules linked in: uhci_hcd ohci_hcd ehci_hcd ppp_deflate zlib_deflate bsd_comp ppp_async crc_ccitt ppp_generic slhc usbhid ipaq usbserial usbcore 8139too mii sr_mod sbp2 scsi_mod ieee1394 psmouse ide_generic ide_cd cdrom genrtc ext3 jbd mbcache ide_disk generic via82cxxx ide_core evdev mousedev
CPU:    0
EIP:    0060:[<b0121c03>]    Not tainted VLI
EFLAGS: 00010213   (2.6.17-rc4 #3) 
EIP is at queue_work+0x17/0x2f
eax: c1e5193c   ebx: b13f2a40   ecx: 00000000   edx: c1e51938
esi: c7104160   edi: b9c90a14   ebp: 00000000   esp: c92bbeb8
ds: 007b   es: 007b   ss: 0068
Process khubd (pid: 1559, threadinfo=c92ba000 task=cbf6c050)
Stack: <0>c7104160 cc985ace c1e51800 b9c90a00 cc9cd980 cc9cd9a4 b9c90a14 cc9dd838 
       b9c90a00 b9c90a7c b9c90a14 b01fb254 b9c90a14 b9c90a14 00000000 cc9f0ba0 
       b01fb419 b9c90a14 b01fac3d b9c90a14 b9c90a5c b9c90a14 c8913058 00000000 
Call Trace:
 <cc985ace> usb_serial_disconnect+0x59/0xa1 [usbserial]   <cc9dd838> usb_unbind_interface+0x36/0x6f [usbcore]
 <b01fb254> __device_release_driver+0x5c/0x72   <b01fb419> device_release_driver+0x18/0x26
 <b01fac3d> bus_remove_device+0x74/0x8c   <b01fa0cc> device_del+0x39/0x65
 <cc9dcaa1> usb_disable_device+0x6a/0xd4 [usbcore]   <cc9d9225> usb_disconnect+0x7c/0xc9 [usbcore]
 <cc9d9f3d> hub_thread+0x35b/0x9eb [usbcore]   <b0123f84> autoremove_wake_function+0x0/0x3a
 <b0123f36> kthread+0x80/0xc1   <cc9d9be2> hub_thread+0x0/0x9eb [usbcore]
 <b0123f4a> kthread+0x94/0xc1   <b0123eb6> kthread+0x0/0xc1
 <b0101005> kernel_thread_helper+0x5/0xb  
Code: 89 d8 5b 5e 5f c3 89 d1 89 c2 a1 f4 71 32 b0 e9 86 ff ff ff 53 89 c3 0f ba 2a 00 19 c0 31 c9 85 c0 75 1c 8d 42 04 39 42 04 74 08 <0f> 0b 6e 00 64 61 27 b0 8b 03 e8 4a fc ff ff b9 01 00 00 00 5b 
EIP: [<b0121c03>] queue_work+0x17/0x2f SS:ESP 0068:c92bbeb8

-- 
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

Re: usb-serial ipaq kernel problem

[Pete Zaitcev]

On Fri, 26 May 2006 20:22:17 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:

> usb 1-4.5.7: USB disconnect, address 79
> ------------[ cut here ]------------
> kernel BUG at kernel/workqueue.c:110!

Please let me know if this helps:

--- linux-2.6.17-rc2/drivers/usb/serial/usb-serial.c	2006-04-23 21:06:18.000000000 -0700
+++ linux-2.6.17-rc2-lem/drivers/usb/serial/usb-serial.c	2006-05-22 21:23:29.000000000 -0700
@@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
 		}
 	}
 
+	flush_scheduled_work();		/* port->work */
+
 	usb_put_dev(serial->dev);
 
 	/* free up any memory that we allocated */

-- Pete

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Fri, May 26, 2006 at 01:34:10PM -0700, Pete Zaitcev wrote:
> On Fri, 26 May 2006 20:22:17 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> > usb 1-4.5.7: USB disconnect, address 79
> > ------------[ cut here ]------------
> > kernel BUG at kernel/workqueue.c:110!
> 
> Please let me know if this helps:

Thanks. It's now running with this applied. I'll let you know if it's
still running in a few days (I got the last one after running +- 2 days)

Frank

> 
> --- linux-2.6.17-rc2/drivers/usb/serial/usb-serial.c	2006-04-23 21:06:18.000000000 -0700
> +++ linux-2.6.17-rc2-lem/drivers/usb/serial/usb-serial.c	2006-05-22 21:23:29.000000000 -0700
> @@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
>  		}
>  	}
>  
> +	flush_scheduled_work();		/* port->work */
> +
>  	usb_put_dev(serial->dev);
>  
>  	/* free up any memory that we allocated */
> 
> -- Pete

-- 
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

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Fri, May 26, 2006 at 01:34:10PM -0700, Pete Zaitcev wrote:
> On Fri, 26 May 2006 20:22:17 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> > usb 1-4.5.7: USB disconnect, address 79
> > ------------[ cut here ]------------
> > kernel BUG at kernel/workqueue.c:110!
> 
> Please let me know if this helps:

It didn't help, I still get the error.

Frank

> 
> --- linux-2.6.17-rc2/drivers/usb/serial/usb-serial.c	2006-04-23 21:06:18.000000000 -0700
> +++ linux-2.6.17-rc2-lem/drivers/usb/serial/usb-serial.c	2006-05-22 21:23:29.000000000 -0700
> @@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
>  		}
>  	}
>  
> +	flush_scheduled_work();		/* port->work */
> +
>  	usb_put_dev(serial->dev);
>  
>  	/* free up any memory that we allocated */
> 
> -- Pete

-- 
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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

 Hi Pete,

On Fri, 26 May 2006 13:34:10 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| On Fri, 26 May 2006 20:22:17 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| 
| > usb 1-4.5.7: USB disconnect, address 79
| > ------------[ cut here ]------------
| > kernel BUG at kernel/workqueue.c:110!
| 
| Please let me know if this helps:
| 
| --- linux-2.6.17-rc2/drivers/usb/serial/usb-serial.c	2006-04-23 21:06:18.000000000 -0700
| +++ linux-2.6.17-rc2-lem/drivers/usb/serial/usb-serial.c	2006-05-22 21:23:29.000000000 -0700
| @@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
|  		}
|  	}
|  
| +	flush_scheduled_work();		/* port->work */
| +
|  	usb_put_dev(serial->dev);
|  
|  	/* free up any memory that we allocated */

 IIUC, the problem occurred before the call to destroy_serial(),
otherwise it should be in the backtrace.

 It seems that 'port->work' is becoming NULL when the device is
disconnected, but the ipaq_write_bulk_callback() is executing after
that.

 I'm checking this also.

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Mon, 29 May 2006 12:01:02 -0300
"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

| 
|  Hi Pete,
| 
| On Fri, 26 May 2006 13:34:10 -0700
| Pete Zaitcev <zaitcev@redhat.com> wrote:
| 
| | On Fri, 26 May 2006 20:22:17 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| | 
| | > usb 1-4.5.7: USB disconnect, address 79
| | > ------------[ cut here ]------------
| | > kernel BUG at kernel/workqueue.c:110!
| | 
| | Please let me know if this helps:
| | 
| | --- linux-2.6.17-rc2/drivers/usb/serial/usb-serial.c	2006-04-23 21:06:18.000000000 -0700
| | +++ linux-2.6.17-rc2-lem/drivers/usb/serial/usb-serial.c	2006-05-22 21:23:29.000000000 -0700
| | @@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
| |  		}
| |  	}
| |  
| | +	flush_scheduled_work();		/* port->work */
| | +
| |  	usb_put_dev(serial->dev);
| |  
| |  	/* free up any memory that we allocated */
| 
|  IIUC, the problem occurred before the call to destroy_serial(),
| otherwise it should be in the backtrace.
| 
|  It seems that 'port->work' is becoming NULL when the device is
| disconnected, but the ipaq_write_bulk_callback() is executing after
| that.

 Err, I meant 'port->work->entry' is empty, of course.

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Mon, 29 May 2006 13:25:53 -0300
"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

| On Mon, 29 May 2006 12:01:02 -0300
| "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| 
| | 
| |  Hi Pete,
| | 
| | On Fri, 26 May 2006 13:34:10 -0700
| | Pete Zaitcev <zaitcev@redhat.com> wrote:
| | 
| | | On Fri, 26 May 2006 20:22:17 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| | | 
| | | > usb 1-4.5.7: USB disconnect, address 79
| | | > ------------[ cut here ]------------
| | | > kernel BUG at kernel/workqueue.c:110!
| | | 
| | | Please let me know if this helps:
| | | 
| | | --- linux-2.6.17-rc2/drivers/usb/serial/usb-serial.c	2006-04-23 21:06:18.000000000 -0700
| | | +++ linux-2.6.17-rc2-lem/drivers/usb/serial/usb-serial.c	2006-05-22 21:23:29.000000000 -0700
| | | @@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
| | |  		}
| | |  	}
| | |  
| | | +	flush_scheduled_work();		/* port->work */
| | | +
| | |  	usb_put_dev(serial->dev);
| | |  
| | |  	/* free up any memory that we allocated */
| | 
| |  IIUC, the problem occurred before the call to destroy_serial(),
| | otherwise it should be in the backtrace.
| | 
| |  It seems that 'port->work' is becoming NULL when the device is
| | disconnected, but the ipaq_write_bulk_callback() is executing after
| | that.
| 
|  Err, I meant 'port->work->entry' is empty, of course.

 Frank, could you try this one please?

 I have no sure whether this makes sense, but every USB-Serial driver
I know exits in the write URB callback if the URB got an error.

-------------------

usbserial: ipaq: Don't handle URB on errors.

ipaq_write_bulk_callback() should exit if the URB got an error, otherwise
we can get weird problems.

Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>

---

 drivers/usb/serial/ipaq.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

6ad106187344769a4722f9e6d6f265529844d568
diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
index 9a5c979..d263a5e 100644
--- a/drivers/usb/serial/ipaq.c
+++ b/drivers/usb/serial/ipaq.c
@@ -855,6 +855,7 @@ static void ipaq_write_bulk_callback(str
 	
 	if (urb->status) {
 		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, urb->status);
+		return;
 	}
 
 	spin_lock_irqsave(&write_list_lock, flags);
-- 
1.3.2



-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Mon, May 29, 2006 at 02:11:10PM -0300, Luiz Fernando N. Capitulino wrote:
> 
>  Frank, could you try this one please?
> 
>  I have no sure whether this makes sense, but every USB-Serial driver
> I know exits in the write URB callback if the URB got an error.

It looks sane to me at least.
The machine is now running with this patch (and my ipaq_open patch, see
http://www.ussg.iu.edu/hypermail/linux/kernel/0605.2/1901.html).
If the problem is still there, it should occur within 24 hours in our
usage mode (25 ipaqs rebooting every 15 minutes to provide lots of
connects and disconnects).  I'll let you know the results.

Frank

> 
> -------------------
> 
> usbserial: ipaq: Don't handle URB on errors.
> 
> ipaq_write_bulk_callback() should exit if the URB got an error, otherwise
> we can get weird problems.
> 
> Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
> 
> ---
> 
>  drivers/usb/serial/ipaq.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> 6ad106187344769a4722f9e6d6f265529844d568
> diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
> index 9a5c979..d263a5e 100644
> --- a/drivers/usb/serial/ipaq.c
> +++ b/drivers/usb/serial/ipaq.c
> @@ -855,6 +855,7 @@ static void ipaq_write_bulk_callback(str
>  	
>  	if (urb->status) {
>  		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, urb->status);
> +		return;
>  	}
>  
>  	spin_lock_irqsave(&write_list_lock, flags);
> -- 
> 1.3.2
> 
> 
> 
> -- 
> 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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Mon, 29 May 2006 21:43:35 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| On Mon, May 29, 2006 at 02:11:10PM -0300, Luiz Fernando N. Capitulino wrote:
| > 
| >  Frank, could you try this one please?
| > 
| >  I have no sure whether this makes sense, but every USB-Serial driver
| > I know exits in the write URB callback if the URB got an error.
| 
| It looks sane to me at least.
| The machine is now running with this patch (and my ipaq_open patch, see
| http://www.ussg.iu.edu/hypermail/linux/kernel/0605.2/1901.html).

 Hmmm. Then does the workqueue problem began to happen _after_ you applied
your patch?

 Are you sure your patch is the right thing to do? Does it look reasonable
to submit that urb 1000 times that way?

 At first, it seems something else.

 Couldn't you run your test-case in a kernel previous to the TTY layer
buffering revamp change?

| If the problem is still there, it should occur within 24 hours in our
| usage mode (25 ipaqs rebooting every 15 minutes to provide lots of
| connects and disconnects).  I'll let you know the results.

 Wow, nice.

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Mon, May 29, 2006 at 05:24:10PM -0300, Luiz Fernando N. Capitulino wrote:
> On Mon, 29 May 2006 21:43:35 +0200
> Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> | On Mon, May 29, 2006 at 02:11:10PM -0300, Luiz Fernando N. Capitulino wrote:
> | > 
> | >  Frank, could you try this one please?
> | > 
> | >  I have no sure whether this makes sense, but every USB-Serial driver
> | > I know exits in the write URB callback if the URB got an error.
> | 
> | It looks sane to me at least.
> | The machine is now running with this patch (and my ipaq_open patch, see
> | http://www.ussg.iu.edu/hypermail/linux/kernel/0605.2/1901.html).
> 
>  Hmmm. Then does the workqueue problem began to happen _after_ you applied
> your patch?

No. I saw it a few times before that as well. Here is the oldest one I found (using 2.6.15)

May  8 13:11:17 localhost kernel: kernel BUG at kernel/workqueue.c:109!
May  8 13:11:17 localhost kernel: invalid operand: 0000 [#1]
May  8 13:11:17 localhost kernel: Modules linked in: ppp_generic slhc ipaq usbserial sd_mod uhci_hcd yealink usb_storage usbhid ohci_hcd ehci_hcd usbcore tun 8139too mii sr_mod sbp2 scsi_mod ieee1394 psmouse ide_generic ide_cd cdrom genrtc ext3 jbd mbcache ide_disk generic via82cxxx ide_core evdev mousedev
May  8 13:11:17 localhost kernel: CPU:    0
May  8 13:11:17 localhost kernel: EIP:    0060:[queue_work+23/50]    Not tainted VLI
May  8 13:11:17 localhost kernel: EFLAGS: 00010213   (2.6.15-1-686)
May  8 13:11:17 localhost kernel: EIP is at queue_work+0x17/0x32
May  8 13:11:17 localhost kernel: eax: d7fcf944   ebx: dbec6680   ecx: 00000000   edx: d7fcf940
May  8 13:11:17 localhost kernel: esi: 00000000   edi: d9b6aa14   ebp: d9b6aa14   esp: d7a03e18
May  8 13:11:17 localhost kernel: ds: 007b   es: 007b   ss: 0068
May  8 13:11:17 localhost kernel: Process rmmod (pid: 4340, threadinfo=d7a02000 task=d77fb050)
May  8 13:11:17 localhost kernel: Stack: d9ab4f20 dca3a9df d7fcf000 d9b6aa00 dca36740 dca36760 dc9e70ea d9b6aa00
May  8 13:11:17 localhost kernel:        d9b6aa7c d9b6aa14 c0202e2a d9b6aa14 d9b6aa14 d9fe1068 d9fe1000 c0202e5c
May  8 13:11:17 localhost kernel:        d9b6aa14 d9b6aa14 c02027f9 d9b6aa14 d9b6aa14 c0201ae9 d9b6aa14 00000000
May  8 13:11:17 localhost kernel: Call Trace:
May  8 13:11:17 localhost kernel:  [pg0+476928479/1070175232] usb_serial_disconnect+0x5b/0x9f [usbserial]
May  8 13:11:17 localhost kernel:  [pg0+476586218/1070175232] usb_unbind_interface+0x36/0x6f [usbcore]
May  8 13:11:17 localhost kernel:  [__device_release_driver+72/99] __device_release_driver+0x48/0x63
May  8 13:11:17 localhost kernel:  [device_release_driver+23/38] device_release_driver+0x17/0x26
May  8 13:11:17 localhost kernel:  [bus_remove_device+82/101] bus_remove_device+0x52/0x65
May  8 13:11:17 localhost kernel:  [device_del+57/101] device_del+0x39/0x65
May  8 13:11:17 localhost kernel:  [pg0+476612208/1070175232] usb_disable_device+0x73/0xe7 [usbcore]
May  8 13:11:17 localhost kernel:  [pg0+476594141/1070175232] usb_disconnect+0x93/0xec [usbcore]
May  8 13:11:17 localhost kernel:  [pg0+476594123/1070175232] usb_disconnect+0x81/0xec [usbcore]
May  8 13:11:17 localhost kernel:  [pg0+476594123/1070175232] usb_disconnect+0x81/0xec [usbcore]
May  8 13:11:17 localhost kernel:  [pg0+476607388/1070175232] usb_remove_hcd+0x58/0xa3 [usbcore]
May  8 13:11:17 localhost kernel:  [pg0+476632530/1070175232] usb_hcd_pci_remove+0x16/0x77 [usbcore]
May  8 13:11:17 localhost kernel:  [pci_device_remove+25/44] pci_device_remove+0x19/0x2c
May  8 13:11:17 localhost kernel:  [__device_release_driver+72/99] __device_release_driver+0x48/0x63
May  8 13:11:17 localhost kernel:  [driver_detach+54/76] driver_detach+0x36/0x4c
May  8 13:11:17 localhost kernel:  [bus_remove_driver+60/93] bus_remove_driver+0x3c/0x5d
May  8 13:11:17 localhost kernel:  [driver_unregister+11/21] driver_unregister+0xb/0x15
May  8 13:11:17 localhost kernel:  [pci_unregister_driver+14/25] pci_unregister_driver+0xe/0x19
May  8 13:11:17 localhost kernel:  [pg0+476326132/1070175232] ehci_hcd_pci_cleanup+0xa/0xc [ehci_hcd]
May  8 13:11:17 localhost kernel:  [sys_delete_module+304/347] sys_delete_module+0x130/0x15b
May  8 13:11:17 localhost kernel:  [do_munmap+223/235] do_munmap+0xdf/0xeb
May  8 13:11:17 localhost kernel:  [sys_munmap+58/85] sys_munmap+0x3a/0x55
May  8 13:11:17 localhost kernel:  [sysenter_past_esp+84/117] sysenter_past_esp+0x54/0x75
May  8 13:11:17 localhost kernel: Code: ff 40 04 83 c0 10 6a 00 e8 fb 17 ff ff 58 57 9d 5b 5e 5f c3 53 31 c9 89 c3 0f ba 2a 00 19 c0 85 c0 75 1f 8d 42 04 39 42 04 74 08 <0f> 0b 6d 00 65 5f 28 c0 52 ff 33 e8 96 ff ff ff 5a b9 01 00 00
May  8 13:11:17 localhost kernel:  <6>ohci_hcd 0000:00:0a.1: remove, state 1

>  Are you sure your patch is the right thing to do? Does it look reasonable
> to submit that urb 1000 times that way?

It only submits it once, just after the control message has succeeded.
The loop is needed because sometimes the ipaq takes a very long time
(more than a minute) before it starts accepting the control message.

>  At first, it seems something else.
> 
>  Couldn't you run your test-case in a kernel previous to the TTY layer
> buffering revamp change?

We first used 2.6.15. We got different types of error : a panic in
ipaq_read_bulk_callback(), the bug I mentionned in
http://www.ussg.iu.edu/hypermail/linux/kernel/0605.2/1770.html and the
current problem. We first tried upgrading to 2.6.16, which did not help.

The panic was caused by the read urb being submitten in ipaq_open,
regardless of success, and never killed in case of failure. What my
patch basically does is to submit the urb only after succesfully sending
the control message, and adding a sleep between tries. As long as this
patch is not applied, we hardly get any other error because the kernel
panics as soon as an ipaq reboots.

After changing ipaq_open, we did not get the panic any more, and the
first error (in do_tty_hangup) seems to have gone at the same time, but
the usb_serial_disconnect bug was still there.

Frank

> | If the problem is still there, it should occur within 24 hours in our
> | usage mode (25 ipaqs rebooting every 15 minutes to provide lots of
> | connects and disconnects).  I'll let you know the results.
> 
>  Wow, nice.
> 
> -- 
> 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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Mon, 29 May 2006 22:47:24 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| On Mon, May 29, 2006 at 05:24:10PM -0300, Luiz Fernando N. Capitulino wrote:
| > On Mon, 29 May 2006 21:43:35 +0200
| > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| > 
| > | On Mon, May 29, 2006 at 02:11:10PM -0300, Luiz Fernando N. Capitulino wrote:
| > | > 
| > | >  Frank, could you try this one please?
| > | > 
| > | >  I have no sure whether this makes sense, but every USB-Serial driver
| > | > I know exits in the write URB callback if the URB got an error.
| > | 
| > | It looks sane to me at least.
| > | The machine is now running with this patch (and my ipaq_open patch, see
| > | http://www.ussg.iu.edu/hypermail/linux/kernel/0605.2/1901.html).
| > 
| >  Hmmm. Then does the workqueue problem began to happen _after_ you applied
| > your patch?
| 
| No. I saw it a few times before that as well. Here is the oldest one I found (using 2.6.15)

 Okay.

| >  Are you sure your patch is the right thing to do? Does it look reasonable
| > to submit that urb 1000 times that way?
| 
| It only submits it once, just after the control message has succeeded.

 Oh, that's right. I didn't see the return statement.

| The loop is needed because sometimes the ipaq takes a very long time
| (more than a minute) before it starts accepting the control message.

 Ok.

| >  At first, it seems something else.
| > 
| >  Couldn't you run your test-case in a kernel previous to the TTY layer
| > buffering revamp change?
| 
| We first used 2.6.15. We got different types of error : a panic in
| ipaq_read_bulk_callback(), the bug I mentionned in
| http://www.ussg.iu.edu/hypermail/linux/kernel/0605.2/1770.html and the
| current problem. We first tried upgrading to 2.6.16, which did not help.
| 
| The panic was caused by the read urb being submitten in ipaq_open,
| regardless of success, and never killed in case of failure. What my
| patch basically does is to submit the urb only after succesfully sending
| the control message, and adding a sleep between tries. As long as this
| patch is not applied, we hardly get any other error because the kernel
| panics as soon as an ipaq reboots.

 I see.

 Did you try to just kill the read urb in the ipaq_open's error path?

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Mon, May 29, 2006 at 07:33:30PM -0300, Luiz Fernando N. Capitulino wrote:
> On Mon, 29 May 2006 22:47:24 +0200
> Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> | 
> | The panic was caused by the read urb being submitten in ipaq_open,
> | regardless of success, and never killed in case of failure. What my
> | patch basically does is to submit the urb only after succesfully sending
> | the control message, and adding a sleep between tries. As long as this
> | patch is not applied, we hardly get any other error because the kernel
> | panics as soon as an ipaq reboots.
> 
>  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.

Frank

> 
> -- 
> 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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Tue, 30 May 2006 10:21:41 +0200
Frank Gevaerts <frank.gevaerts@fks.be> 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
| > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| > | 
| > | The panic was caused by the read urb being submitten in ipaq_open,
| > | regardless of success, and never killed in case of failure. What my
| > | patch basically does is to submit the urb only after succesfully sending
| > | the control message, and adding a sleep between tries. As long as this
| > | patch is not applied, we hardly get any other error because the kernel
| > | panics as soon as an ipaq reboots.
| > 
| >  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..

 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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Tue, 30 May 2006 11:38:01 -0300
"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

| On Tue, 30 May 2006 10:21:41 +0200
| Frank Gevaerts <frank.gevaerts@fks.be> 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
| | > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| | > | 
| | > | The panic was caused by the read urb being submitten in ipaq_open,
| | > | regardless of success, and never killed in case of failure. What my
| | > | patch basically does is to submit the urb only after succesfully sending
| | > | the control message, and adding a sleep between tries. As long as this
| | > | patch is not applied, we hardly get any other error because the kernel
| | > | panics as soon as an ipaq reboots.
| | > 
| | >  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.

 Thinking about this again, are you sure the read urb depends on the
control message? It's quite easy to test, just a add a long timeout after
the read URB was sent (say, five minutes) and waits for the read urb
callback to run.

 If it ran _before_ the timeout expires with no timeout error it does not
depend. Then we can do the simpler solution: just kill the read urb in the
ipaq_open's error path.

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Tue, May 30, 2006 at 11:53:29AM -0300, Luiz Fernando N. Capitulino wrote:
> On Tue, 30 May 2006 11:38:01 -0300
> "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
> 
> | On Tue, 30 May 2006 10:21:41 +0200
> | Frank Gevaerts <frank.gevaerts@fks.be> 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
> | | > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> | | > | 
> | | > | The panic was caused by the read urb being submitten in ipaq_open,
> | | > | regardless of success, and never killed in case of failure. What my
> | | > | patch basically does is to submit the urb only after succesfully sending
> | | > | the control message, and adding a sleep between tries. As long as this
> | | > | patch is not applied, we hardly get any other error because the kernel
> | | > | panics as soon as an ipaq reboots.
> | | > 
> | | >  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.
> 
>  Thinking about this again, are you sure the read urb depends on the
> control message? It's quite easy to test, just a add a long timeout after
> the read URB was sent (say, five minutes) and waits for the read urb
> callback to run.
> 
>  If it ran _before_ the timeout expires with no timeout error it does not
> depend. Then we can do the simpler solution: just kill the read urb in the
> ipaq_open's error path.

I'll try it sometime today.

Frank

> 
> -- 
> 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

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Tue, May 30, 2006 at 11:53:29AM -0300, Luiz Fernando N. Capitulino wrote:
> On Tue, 30 May 2006 11:38:01 -0300
> "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
> 
>  If it ran _before_ the timeout expires with no timeout error it does not
> depend. Then we can do the simpler solution: just kill the read urb in the
> ipaq_open's error path.

That seems to work.
I also found that both the return in ipaq_write_bulk_callback and the
flush_scheduled_work() in destroy_serial() are needed to get rid of the
usb_serial_disconnect() bug.

It's now running with the following patch:

Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>

diff -pur linux-2.6.17-rc4/drivers/usb/serial/ipaq.c linux-2.6.17-rc4.test/drivers/usb/serial/ipaq.c
--- linux-2.6.17-rc4/drivers/usb/serial/ipaq.c	2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.17-rc4.test/drivers/usb/serial/ipaq.c	2006-05-30 19:41:19.000000000 +0200
@@ -71,6 +71,7 @@
 
 static __u16 product, vendor;
 static int debug;
+static int connect_retries;
 
 /* Function prototypes for an ipaq */
 static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
@@ -583,7 +584,7 @@ static int ipaq_open(struct usb_serial_p
 	struct ipaq_private	*priv;
 	struct ipaq_packet	*pkt;
 	int			i, result = 0;
-	int			retries = KP_RETRIES;
+	int			retries = connect_retries;
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
@@ -681,6 +682,7 @@ enomem:
 	result = -ENOMEM;
 	err("%s - Out of memory", __FUNCTION__);
 error:
+	usb_kill_urb(port->read_urb);
 	ipaq_destroy_lists(port);
 	kfree(priv);
 	return result;
@@ -692,6 +694,7 @@ static void ipaq_close(struct usb_serial
 	struct ipaq_private	*priv = usb_get_serial_port_data(port);
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
+
 			 
 	/*
 	 * shut down bulk read and write
@@ -855,6 +858,7 @@ static void ipaq_write_bulk_callback(str
 	
 	if (urb->status) {
 		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, urb->status);
+		return;
 	}
 
 	spin_lock_irqsave(&write_list_lock, flags);
@@ -967,3 +971,6 @@ MODULE_PARM_DESC(vendor, "User specified
 
 module_param(product, ushort, 0);
 MODULE_PARM_DESC(product, "User specified USB idProduct");
+
+module_param(connect_retries, int, KP_RETRIES);
+MODULE_PARM_DESC(product, "Maximum number of connect retries (100ms each)");
diff -pur linux-2.6.17-rc4/drivers/usb/serial/usb-serial.c linux-2.6.17-rc4.test/drivers/usb/serial/usb-serial.c
--- linux-2.6.17-rc4/drivers/usb/serial/usb-serial.c	2006-05-30 19:01:16.000000000 +0200
+++ linux-2.6.17-rc4.test/drivers/usb/serial/usb-serial.c	2006-05-30 19:01:24.000000000 +0200
@@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
 		}
 	}
 
+	flush_scheduled_work();		/* port->work */
+
 	usb_put_dev(serial->dev);
 
 	/* free up any memory that we allocated */

Frank

-- 
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

Re: usb-serial ipaq kernel problem

[Pete Zaitcev]

On Tue, 30 May 2006 19:48:21 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:
+0100
> +++ linux-2.6.17-rc4.test/drivers/usb/serial/ipaq.c	2006-05-30 19:41:19.000000000 +0200
> @@ -692,6 +694,7 @@ static void ipaq_close(struct usb_serial
>  	struct ipaq_private	*priv = usb_get_serial_port_data(port);
>  
>  	dbg("%s - port %d", __FUNCTION__, port->number);
> +
>  			 
>  	/*
>  	 * shut down bulk read and write

Please get rid of the above.

> @@ -967,3 +971,6 @@ MODULE_PARM_DESC(vendor, "User specified
>  
>  module_param(product, ushort, 0);
>  MODULE_PARM_DESC(product, "User specified USB idProduct");
> +
> +module_param(connect_retries, int, KP_RETRIES);
> +MODULE_PARM_DESC(product, "Maximum number of connect retries (100ms each)");

Personally, I'm not keen on adding knobs.

-- Pete

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Tue, May 30, 2006 at 11:33:27AM -0700, Pete Zaitcev wrote:
> On Tue, 30 May 2006 19:48:21 +0200, Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> +0100
> > +++ linux-2.6.17-rc4.test/drivers/usb/serial/ipaq.c	2006-05-30 19:41:19.000000000 +0200
> > @@ -692,6 +694,7 @@ static void ipaq_close(struct usb_serial
> >  	struct ipaq_private	*priv = usb_get_serial_port_data(port);
> >  
> >  	dbg("%s - port %d", __FUNCTION__, port->number);
> > +
> >  			 
> >  	/*
> >  	 * shut down bulk read and write
> 
> Please get rid of the above.

OK. I missed one while cleaning up.

> > @@ -967,3 +971,6 @@ MODULE_PARM_DESC(vendor, "User specified
> >  
> >  module_param(product, ushort, 0);
> >  MODULE_PARM_DESC(product, "User specified USB idProduct");
> > +
> > +module_param(connect_retries, int, KP_RETRIES);
> > +MODULE_PARM_DESC(product, "Maximum number of connect retries (100ms each)");
> 
> Personally, I'm not keen on adding knobs.

As far as I can see, the alternatives are that either it does not work
without patches for scenarios where the ipaq is rebooted while connected
(like we do), since these need a large number of retries (up to 3500 in
our tests today, about 6 minutes), or the default value is much higher
than the current 100 (which gives a 10 seconds timeout).

I'm not sure what the best solution is.

Frank

> 
> -- Pete

-- 
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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Tue, 30 May 2006 11:33:27 -0700
Pete Zaitcev <zaitcev@redhat.com> wrote:

| > @@ -967,3 +971,6 @@ MODULE_PARM_DESC(vendor, "User specified
| >  
| >  module_param(product, ushort, 0);
| >  MODULE_PARM_DESC(product, "User specified USB idProduct");
| > +
| > +module_param(connect_retries, int, KP_RETRIES);
| > +MODULE_PARM_DESC(product, "Maximum number of connect retries (100ms each)");
| 
| Personally, I'm not keen on adding knobs.

 We have a device-dependent parameter here, I can't think in anything
better..

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Tue, May 30, 2006 at 11:33:27AM -0700, Pete Zaitcev wrote:
> 
> Please get rid of the above.
> >  	 * shut down bulk read and write

OK, So here's the corrected patch:

Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>


diff -pur linux-2.6.17-rc4/drivers/usb/serial/ipaq.c linux-2.6.17-rc4.test/drivers/usb/serial/ipaq.c
--- linux-2.6.17-rc4/drivers/usb/serial/ipaq.c	2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.17-rc4.test/drivers/usb/serial/ipaq.c	2006-05-30 20:46:23.000000000 +0200
@@ -71,6 +71,7 @@
 
 static __u16 product, vendor;
 static int debug;
+static int connect_retries;
 
 /* Function prototypes for an ipaq */
 static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
@@ -583,7 +584,7 @@ static int ipaq_open(struct usb_serial_p
 	struct ipaq_private	*priv;
 	struct ipaq_packet	*pkt;
 	int			i, result = 0;
-	int			retries = KP_RETRIES;
+	int			retries = connect_retries;
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
@@ -681,6 +682,7 @@ enomem:
 	result = -ENOMEM;
 	err("%s - Out of memory", __FUNCTION__);
 error:
+	usb_kill_urb(port->read_urb);
 	ipaq_destroy_lists(port);
 	kfree(priv);
 	return result;
@@ -855,6 +857,7 @@ static void ipaq_write_bulk_callback(str
 	
 	if (urb->status) {
 		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, urb->status);
+		return;
 	}
 
 	spin_lock_irqsave(&write_list_lock, flags);
@@ -967,3 +970,6 @@ MODULE_PARM_DESC(vendor, "User specified
 
 module_param(product, ushort, 0);
 MODULE_PARM_DESC(product, "User specified USB idProduct");
+
+module_param(connect_retries, int, KP_RETRIES);
+MODULE_PARM_DESC(product, "Maximum number of connect retries (100ms each)");
diff -pur linux-2.6.17-rc4/drivers/usb/serial/usb-serial.c linux-2.6.17-rc4.test/drivers/usb/serial/usb-serial.c
--- linux-2.6.17-rc4/drivers/usb/serial/usb-serial.c	2006-05-30 19:01:16.000000000 +0200
+++ linux-2.6.17-rc4.test/drivers/usb/serial/usb-serial.c	2006-05-30 19:01:24.000000000 +0200
@@ -162,6 +162,8 @@ static void destroy_serial(struct kref *
 		}
 	}
 
+	flush_scheduled_work();		/* port->work */
+
 	usb_put_dev(serial->dev);
 
 	/* free up any memory that we allocated */



-- 
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

Re: usb-serial ipaq kernel problem

[Greg KH]

On Wed, May 31, 2006 at 11:38:28PM +0200, Frank Gevaerts wrote:
> On Tue, May 30, 2006 at 11:33:27AM -0700, Pete Zaitcev wrote:
> > 
> > Please get rid of the above.
> > >  	 * shut down bulk read and write
> 
> OK, So here's the corrected patch:
> 
> Signed-off-by: Frank Gevaerts <frank.gevaerts@fks.be>

Care to send it with a proper changelog description?  And not the
usb-serial.c fix as that's already in my tree.

thanks,

greg k-h

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Tue, 30 May 2006 19:48:21 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| On Tue, May 30, 2006 at 11:53:29AM -0300, Luiz Fernando N. Capitulino wrote:
| > On Tue, 30 May 2006 11:38:01 -0300
| > "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| > 
| >  If it ran _before_ the timeout expires with no timeout error it does not
| > depend. Then we can do the simpler solution: just kill the read urb in the
| > ipaq_open's error path.
| 
| That seems to work.
| I also found that both the return in ipaq_write_bulk_callback and the
| flush_scheduled_work() in destroy_serial() are needed to get rid of the
| usb_serial_disconnect() bug.

 Then did you hit it with my patch?

 I'm just worried with the fact that you're hitting it with every
proposed fix. Maybe it's something else.

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Tue, May 30, 2006 at 05:52:08PM -0300, Luiz Fernando N. Capitulino wrote:
> On Tue, 30 May 2006 19:48:21 +0200
> Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> | On Tue, May 30, 2006 at 11:53:29AM -0300, Luiz Fernando N. Capitulino wrote:
> | > On Tue, 30 May 2006 11:38:01 -0300
> | > "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
> | > 
> | >  If it ran _before_ the timeout expires with no timeout error it does not
> | > depend. Then we can do the simpler solution: just kill the read urb in the
> | > ipaq_open's error path.
> | 
> | That seems to work.
> | I also found that both the return in ipaq_write_bulk_callback and the
> | flush_scheduled_work() in destroy_serial() are needed to get rid of the
> | usb_serial_disconnect() bug.
> 
>  Then did you hit it with my patch?
> 
>  I'm just worried with the fact that you're hitting it with every
> proposed fix. Maybe it's something else.

I'm hitting it with either of the proposed fixes, but not when both are
applied.

Frank

> 
> -- 
> 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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Tue, 30 May 2006 23:36:35 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| On Tue, May 30, 2006 at 05:52:08PM -0300, Luiz Fernando N. Capitulino wrote:
| > On Tue, 30 May 2006 19:48:21 +0200
| > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
| > 
| > | On Tue, May 30, 2006 at 11:53:29AM -0300, Luiz Fernando N. Capitulino wrote:
| > | > On Tue, 30 May 2006 11:38:01 -0300
| > | > "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
| > | > 
| > | >  If it ran _before_ the timeout expires with no timeout error it does not
| > | > depend. Then we can do the simpler solution: just kill the read urb in the
| > | > ipaq_open's error path.
| > | 
| > | That seems to work.
| > | I also found that both the return in ipaq_write_bulk_callback and the
| > | flush_scheduled_work() in destroy_serial() are needed to get rid of the
| > | usb_serial_disconnect() bug.
| > 
| >  Then did you hit it with my patch?
| > 
| >  I'm just worried with the fact that you're hitting it with every
| > proposed fix. Maybe it's something else.
| 
| I'm hitting it with either of the proposed fixes, but not when both are
| applied.

 Is this still true? :)

-- 
Luiz Fernando N. Capitulino

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

On Wed, May 31, 2006 at 06:10:42PM -0300, Luiz Fernando N. Capitulino wrote:
> On Tue, 30 May 2006 23:36:35 +0200
> Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> | On Tue, May 30, 2006 at 05:52:08PM -0300, Luiz Fernando N. Capitulino wrote:
> | > On Tue, 30 May 2006 19:48:21 +0200
> | > Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> | > 
> | > | On Tue, May 30, 2006 at 11:53:29AM -0300, Luiz Fernando N. Capitulino wrote:
> | > | > On Tue, 30 May 2006 11:38:01 -0300
> | > | > "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:
> | > | > 
> | > | >  If it ran _before_ the timeout expires with no timeout error it does not
> | > | > depend. Then we can do the simpler solution: just kill the read urb in the
> | > | > ipaq_open's error path.
> | > | 
> | > | That seems to work.
> | > | I also found that both the return in ipaq_write_bulk_callback and the
> | > | flush_scheduled_work() in destroy_serial() are needed to get rid of the
> | > | usb_serial_disconnect() bug.
> | > 
> | >  Then did you hit it with my patch?
> | > 
> | >  I'm just worried with the fact that you're hitting it with every
> | > proposed fix. Maybe it's something else.
> | 
> | I'm hitting it with either of the proposed fixes, but not when both are
> | applied.
> 
>  Is this still true? :)

Yes. It's still running, with about 2000 disconnects since the last
boot.

Frank

> 
> -- 
> 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

Re: usb-serial ipaq kernel problem

[Frank Gevaerts]

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 <frank.gevaerts@fks.be> 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                                 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

Re: usb-serial ipaq kernel problem

[Luiz Fernando N. Capitulino]

On Tue, 30 May 2006 17:06:27 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| 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 <frank.gevaerts@fks.be> 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.

 Hmmm, what do you think about keeping the current default value and
adding a module parameter to change it?

| 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)

 Nice to know.

-- 
Luiz Fernando N. Capitulino