usb-serial ipaq kernel problem

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

[PATCH] ipaq.c bugfixes

[Frank Gevaerts]

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

The connect_retries parameter is added because if a pocketpc device is
connected while it is rebooting, it can take a long time after the USB
connect (sometimes several minutes) before it starts accepting the
control packet that starts the serial connection. Since this is not the
normal usecase, it is probably better to leave the default number of
retries as-is.

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

-- 
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] ipaq.c bugfixes

[Greg KH]

On Thu, Jun 01, 2006 at 12:42:45AM +0200, Frank Gevaerts 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>)
> 
> The connect_retries parameter is added because if a pocketpc device is
> connected while it is rebooting, it can take a long time after the USB
> connect (sometimes several minutes) before it starts accepting the
> control packet that starts the serial connection. Since this is not the
> normal usecase, it is probably better to leave the default number of
> retries as-is.
> 
> 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);

I really do not think that you want KP_RETRIES as a mode value in sysfs
:)

This is not how you pre-initialize a module parameter...

thanks,

greg k-h

Re: [PATCH] ipaq.c bugfixes

[Frank Gevaerts]

On Wed, May 31, 2006 at 03:46:24PM -0700, Greg KH wrote:
> On Thu, Jun 01, 2006 at 12:42:45AM +0200, Frank Gevaerts wrote:
> > +
> > +module_param(connect_retries, int, KP_RETRIES);
> 
> I really do not think that you want KP_RETRIES as a mode value in sysfs
> :)
> 
> This is not how you pre-initialize a module parameter...

Thanks. That should teach me not to try to fix kernel code without
reading documentation. I fixed it here, but I won't resubmit yet because
there are some 100% reproducible bugs left. 

Frank

> 
> 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] ipaq.c bugfixes

[Frank Gevaerts]

On Thu, Jun 01, 2006 at 09:18:40PM +0200, Frank Gevaerts wrote:
> On Wed, May 31, 2006 at 03:46:24PM -0700, Greg KH wrote:
> > This is not how you pre-initialize a module parameter...
> 
> Thanks. That should teach me not to try to fix kernel code without
> reading documentation. I fixed it here, but I won't resubmit yet because
> there are some 100% reproducible bugs left. 

Everything now runs for more than a week without problems, so I guess I
now have all bugs. I have split out the extra parameters to a separate
patch, because they might not be needed for the standard kernel.
The fixes to usb-serial.c (in a separate mail/thread) are also needed
for bug-free operation.


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 13:22:57.000000000 +0200
@@ -652,7 +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;
@@ -671,6 +670,7 @@ 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);
 			return 0;
 		}
 	}
@@ -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);

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

[PATCH] ipaq.c connection open timing parameters

[Frank Gevaerts]

Adds configurable waiting periods to the ipaq connection code. These are
not needed when the pocketpc device is running normally when plugged in,
but they need extra delays if they are physically connected while
rebooting.
There are two parameters :
* initial_wait : this is the delay before the driver attemts to start the
  connection. This is needed because the pocktpc device takes much
  longer to boot if the driver starts sending control packets too soon.
* connect_retries : this is the number of times the control urb is
  retried before finally giving up. The patch also adds a 1 second delay
  between retries.
I'm not sure if the cases where this patch is useful are general enough
to include this in the kernel.

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

diff -urp linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c
--- linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 13:22:57.000000000 +0200
+++ linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c	2006-06-14 13:29:40.000000000 +0200
@@ -71,6 +71,8 @@
 
 static __u16 product, vendor;
 static int debug;
+static int connect_retries = KP_RETRIES;
+static int initial_wait;
 
 /* Function prototypes for an ipaq */
 static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
@@ -583,7 +585,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);
 
@@ -647,6 +649,7 @@ static int ipaq_open(struct usb_serial_p
 	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
 	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
 	
+	msleep(1000*initial_wait);
 	/* Start reading from the device */
 	usb_fill_bulk_urb(port->read_urb, serial->dev, 
 		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
@@ -673,6 +676,7 @@ static int ipaq_open(struct usb_serial_p
 			result = usb_submit_urb(port->read_urb, GFP_KERNEL);
 			return 0;
 		}
+		msleep(1000);
 	}
 	err("%s - failed doing control urb, error %d", __FUNCTION__, result);
 	goto error;
@@ -968,3 +972,9 @@ MODULE_PARM_DESC(vendor, "User specified
 
 module_param(product, ushort, 0);
 MODULE_PARM_DESC(product, "User specified USB idProduct");
+
+module_param(connect_retries, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(connect_retries, "Maximum number of connect retries (1 second each)");
+
+module_param(initial_wait, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(initial_wait, "Time to wait before attempting a connection (in seconds)");

-- 
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] ipaq.c connection open timing parameters

[Frank Gevaerts]

On Wed, Jun 14, 2006 at 02:05:52PM +0200, Frank Gevaerts wrote:
> Adds configurable waiting periods to the ipaq connection code. These are
> not needed when the pocketpc device is running normally when plugged in,
> but they need extra delays if they are physically connected while
> rebooting.

This won't apply cleanly because of the mistake I made in the
correctness patch. Here's the new version:

Adds configurable waiting periods to the ipaq connection code. These are
not needed when the pocketpc device is running normally when plugged in,
but they need extra delays if they are physically connected while
rebooting.
There are two parameters :
* initial_wait : this is the delay before the driver attemts to start the
  connection. This is needed because the pocktpc device takes much
  longer to boot if the driver starts sending control packets too soon.
* connect_retries : this is the number of times the control urb is
  retried before finally giving up. The patch also adds a 1 second delay
  between retries.
I'm not sure if the cases where this patch is useful are general enough
to include this in the kernel.

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

diff -urp linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c
--- linux-2.6.17-rc6.a/drivers/usb/serial/ipaq.c	2006-06-14 16:02:03.000000000 +0200
+++ linux-2.6.17-rc6.b/drivers/usb/serial/ipaq.c	2006-06-14 16:06:44.000000000 +0200
@@ -71,6 +71,8 @@
 
 static __u16 product, vendor;
 static int debug;
+static int connect_retries = KP_RETRIES;
+static int initial_wait;
 
 /* Function prototypes for an ipaq */
 static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
@@ -583,7 +585,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);
 
@@ -647,6 +649,7 @@ static int ipaq_open(struct usb_serial_p
 	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
 	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
 	
+	msleep(1000*initial_wait);
 	/* Start reading from the device */
 	usb_fill_bulk_urb(port->read_urb, serial->dev, 
 		      usb_rcvbulkpipe(serial->dev, port->bulk_in_endpointAddress),
@@ -673,6 +676,7 @@ static int ipaq_open(struct usb_serial_p
 			}
 			return 0;
 		}
+		msleep(1000);
 	}
 	err("%s - failed doing control urb, error %d", __FUNCTION__, result);
 	goto error;
@@ -968,3 +972,9 @@ MODULE_PARM_DESC(vendor, "User specified
 
 module_param(product, ushort, 0);
 MODULE_PARM_DESC(product, "User specified USB idProduct");
+
+module_param(connect_retries, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(connect_retries, "Maximum number of connect retries (100ms each)");
+
+module_param(initial_wait, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(initial_wait, "Time to wait before attempting a connection (in seconds)");


-- 
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] ipaq.c bugfixes

[Luiz Fernando N. Capitulino]

On Wed, 14 Jun 2006 13:58:22 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| On Thu, Jun 01, 2006 at 09:18:40PM +0200, Frank Gevaerts wrote:
| > On Wed, May 31, 2006 at 03:46:24PM -0700, Greg KH wrote:
| > > This is not how you pre-initialize a module parameter...
| > 
| > Thanks. That should teach me not to try to fix kernel code without
| > reading documentation. I fixed it here, but I won't resubmit yet because
| > there are some 100% reproducible bugs left. 
| 
| Everything now runs for more than a week without problems, so I guess I
| now have all bugs. I have split out the extra parameters to a separate
| patch, because they might not be needed for the standard kernel.
| The fixes to usb-serial.c (in a separate mail/thread) are also needed
| for bug-free operation.
| 
| 
| 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 13:22:57.000000000 +0200
| @@ -652,7 +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;
| @@ -671,6 +670,7 @@ 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);
|  			return 0;
|  		}
|  	}

 I think you forgot to move the 'result' checking after the urb is
submitted.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] ipaq.c bugfixes

[Frank Gevaerts]

On Wed, Jun 14, 2006 at 10:58:49AM -0300, Luiz Fernando N. Capitulino wrote:
>  I think you forgot to move the 'result' checking after the urb is
> submitted.

Sorry about that. The running version is correct. I apparently
overlooked that while splitting the patch. Here's the correct version:


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;
 		}
 	}
@@ -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);



-- 
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] ipaq.c bugfixes

[Frank Gevaerts]

On Thu, Jun 01, 2006 at 12:42:45AM +0200, Frank Gevaerts wrote:
> This patch fixes several problems in the ipaq.c driver with connecting
> and disconnecting pocketpc devices: 

Unfortunately, it is apparently not the whole story.

There are some problems:

With this patch, whenever the usb_control_msg returns with ENODEV (-19),
we get the following :

Jun  1 10:02:06 localhost kernel: BUG: unable to handle kernel paging request at virtual address 723d4ecb
Jun  1 10:02:06 localhost kernel:  printing eip:
Jun  1 10:02:06 localhost kernel: b01e579b
Jun  1 10:02:06 localhost kernel: *pde = 00000000
Jun  1 10:02:06 localhost kernel: Oops: 0000 [#1]
Jun  1 10:02:06 localhost kernel: Modules linked in: ppp_deflate zlib_deflate bsd_comp ppp_async crc_ccitt ppp_generic slhc usbhid ipaq uhci_hcd usbserial ohci_hcd 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
Jun  1 10:02:06 localhost kernel: CPU:    0
Jun  1 10:02:06 localhost kernel: EIP:    0060:[check_tty_count+39/110]    Not tainted VLI
Jun  1 10:02:06 localhost kernel: EFLAGS: 00010246   (2.6.17-rc4 #4)
Jun  1 10:02:06 localhost kernel: EIP is at check_tty_count+0x27/0x6e
Jun  1 10:02:06 localhost kernel: eax: 723d4e4f   ebx: c95f6000   ecx: c95f6170   edx: c95f6170
Jun  1 10:02:06 localhost kernel: esi: 00000000   edi: b0287619   ebp: 00000000   esp: cbec7f44
Jun  1 10:02:06 localhost kernel: ds: 007b   es: 007b   ss: 0068
Jun  1 10:02:06 localhost kernel: Process events/0 (pid: 4, threadinfo=cbec6000 task=cbfdfa70)
Jun  1 10:02:06 localhost kernel: Stack: <0>c95f613c c95f6000 00000283 b01e5fb0 cbfe53a8 cbfe53a8 c95f613c cbfe53a0
Jun  1 10:02:06 localhost kernel:        00000283 c95f6000 b01219b1 c95f6000 b01e5f75 cbfe53a8 cbfe53a0 cbfe53b0
Jun  1 10:02:06 localhost kernel:        b0121a55 b0121b34 00000001 00000000 000f41fa 00010000 00000000 00000000
Jun  1 10:02:06 localhost kernel: Call Trace:
Jun  1 10:02:06 localhost kernel:  <b01e5fb0> do_tty_hangup+0x3b/0x263   <b01219b1> run_workqueue+0x64/0x94
Jun  1 10:02:06 localhost kernel:  <b01e5f75> do_tty_hangup+0x0/0x263   <b0121a55> worker_thread+0x0/0x10f
Jun  1 10:02:06 localhost kernel:  <b0121b34> worker_thread+0xdf/0x10f   <b01137ca> default_wake_function+0x0/0x15
Jun  1 10:02:06 localhost kernel:  <b0123f4a> kthread+0x94/0xc1   <b0123eb6> kthread+0x0/0xc1
Jun  1 10:02:06 localhost kernel:  <b0101005> kernel_thread_helper+0x5/0xb
Jun  1 10:02:06 localhost kernel: Code: 59 5e 5f c3 57 89 d7 56 31 f6 53 89 c3 8b 90 70 01 00 00 eb 03 46 89 ca 8b 0a 0f 18 01 90 8d 83 70 01 00 00 39 c2 75 ed 8b 43 04 <81> 78 7c 04 00 02 00 75 16 8b 83 cc 00 00 00 85 c0 74 0c 8b 80
Jun  1 10:02:06 localhost kernel: EIP: [check_tty_count+39/110] check_tty_count+0x27/0x6e SS:ESP 0068:cbec7f44

When I changed te ipaq_open code to only submit the read urb after the
control message succeeds, these disappear.

Whenever the usb_control_msg returns with ETIMEDOUT (-110), in both code
variants, when the device is disconnected we get:

Jun  1 20:23:55 localhost kernel: ------------[ cut here ]------------
Jun  1 20:23:55 localhost kernel: kernel BUG at kernel/workqueue.c:110!
Jun  1 20:23:55 localhost kernel: invalid opcode: 0000 [#1]
Jun  1 20:23:55 localhost kernel: Modules linked in: ppp_deflate zlib_deflate bsd_comp ppp_async crc_ccitt ppp_generic slhc uhci_hcd ohci_hcd usbhid ipaq usbserial 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
Jun  1 20:23:55 localhost kernel: CPU:    0
Jun  1 20:23:55 localhost kernel: EIP:    0060:[queue_work+23/47]    Not tainted VLI
Jun  1 20:23:55 localhost kernel: EFLAGS: 00010a93   (2.6.17-rc4 #4)
Jun  1 20:23:55 localhost kernel: EIP is at queue_work+0x17/0x2f
Jun  1 20:23:55 localhost kernel: eax: c383113c   ebx: b13f29c0   ecx: 00000000   edx: c3831138
Jun  1 20:23:55 localhost kernel: esi: c44e8d60   edi: ca2f4814   ebp: 00000000   esp: c6bafeb8
Jun  1 20:23:55 localhost kernel: ds: 007b   es: 007b   ss: 0068
Jun  1 20:23:55 localhost kernel: Process khubd (pid: 1629, threadinfo=c6bae000 task=cbfd3a90)
Jun  1 20:23:55 localhost kernel: Stack: <0>c44e8d60 cc9bfad3 c3831000 ca2f4800 cc9bcb40 cc9bcb64 ca2f4814 cc9dd838
Jun  1 20:23:55 localhost kernel:        ca2f4800 ca2f487c ca2f4814 b01fb254 ca2f4814 ca2f4814 00000000 cc9f0ba0
Jun  1 20:23:55 localhost kernel:        b01fb419 ca2f4814 b01fac3d ca2f4814 ca2f485c ca2f4814 c5cc3858 00000000
Jun  1 20:23:55 localhost kernel: Call Trace:
Jun  1 20:23:55 localhost kernel:  <cc9bfad3> usb_serial_disconnect+0x59/0xa1 [usbserial]   <cc9dd838> usb_unbind_interface+0x36/0x6f [usbcore]
Jun  1 20:23:55 localhost kernel:  <b01fb254> __device_release_driver+0x5c/0x72   <b01fb419> device_release_driver+0x18/0x26
Jun  1 20:23:55 localhost kernel:  <b01fac3d> bus_remove_device+0x74/0x8c   <b01fa0cc> device_del+0x39/0x65
Jun  1 20:23:55 localhost kernel:  <cc9dcaa1> usb_disable_device+0x6a/0xd4 [usbcore]   <cc9d9225> usb_disconnect+0x7c/0xc9 [usbcore]
Jun  1 20:23:55 localhost kernel:  <cc9d9f3d> hub_thread+0x35b/0x9eb [usbcore]   <b0123f84> autoremove_wake_function+0x0/0x3a
Jun  1 20:23:55 localhost kernel:  <b0123f36> kthread+0x80/0xc1   <cc9d9be2> hub_thread+0x0/0x9eb [usbcore]
Jun  1 20:23:55 localhost kernel:  <b0123f4a> kthread+0x94/0xc1   <b0123eb6> kthread+0x0/0xc1
Jun  1 20:23:55 localhost kernel:  <b0101005> kernel_thread_helper+0x5/0xb
Jun  1 20:23:55 localhost kernel: 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
Jun  1 20:23:55 localhost kernel: EIP: [queue_work+23/47] queue_work+0x17/0x2f SS:ESP 0068:c6bafeb8

This seems to be 100% reproducible. I noticed that serial_open (in
usb-serial.c) sets port->tty = tty and tty->driver_data = port, but
doesn't set them back to NULL if the type->open() fails. Is that correct
?

Also, we have discovered that the slow response of the ipaq on connect
is actually largely caused by the control message retries, so we have
added a sleep at the start of ipaq_open.

The current patch we are testing (not yet ready for inclusion, since
it doesn't work correctly yet and it produces some output that is not
useful in general) is :

diff -urp 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-06-01 20:08:54.000000000 +0200
@@ -71,6 +71,8 @@
 
 static __u16 product, vendor;
 static int debug;
+static int connect_retries = KP_RETRIES;
+static int initial_wait;
 
 /* Function prototypes for an ipaq */
 static int  ipaq_open (struct usb_serial_port *port, struct file *filp);
@@ -583,7 +585,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);
 
@@ -647,12 +649,12 @@ static int ipaq_open(struct usb_serial_p
 	port->read_urb->transfer_buffer_length = URBDATA_SIZE;
 	port->bulk_out_size = port->write_urb->transfer_buffer_length = URBDATA_SIZE;
 	
+	msleep(1000*initial_wait);
 	/* 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;
@@ -669,12 +671,15 @@ static int ipaq_open(struct usb_serial_p
 	while (retries--) {
 		result = usb_control_msg(serial->dev,
 				usb_sndctrlpipe(serial->dev, 0), 0x22, 0x21,
-				0x1, 0, NULL, 0, 100);
+				0x1, 0, NULL, 0, 50);
 		if (result == 0) {
+			info("%s - submitted read urb to %s (serial:%s) after %d retries", __FUNCTION__,serial->dev->devpath,serial->dev->serial==NULL?"":serial->dev->serial,connect_retries-retries);
+			result = usb_submit_urb(port->read_urb, GFP_KERNEL);
 			return 0;
 		}
+		msleep(1000);
 	}
-	err("%s - failed doing control urb, error %d", __FUNCTION__, result);
+	err("%s - failed doing control urb to %s (serial:%s), error %d", __FUNCTION__,serial->dev->devpath,serial->dev->serial==NULL?"":serial->dev->serial, result);
 	goto error;
 
 enomem:
@@ -692,6 +697,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);
+	info("%s - closed %s (serial:%s)", __FUNCTION__,port->serial->dev->devpath,port->serial->dev->serial==NULL?"":port->serial->dev->serial);
 			 
 	/*
 	 * shut down bulk read and write
@@ -855,6 +861,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 +974,9 @@ MODULE_PARM_DESC(vendor, "User specified
 
 module_param(product, ushort, 0);
 MODULE_PARM_DESC(product, "User specified USB idProduct");
+
+module_param(connect_retries, int, 0);
+MODULE_PARM_DESC(connect_retries, "Maximum number of connect retries (100ms each)");
+
+module_param(initial_wait, int, 0);
+MODULE_PARM_DESC(initial_wait, "Time to wait before attempting a connection (in seconds)");
diff -urp 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: [linux-usb-devel] [PATCH] ipaq.c bugfixes

[Luiz Fernando N. Capitulino]

On Thu, 1 Jun 2006 21:16:54 +0200
Frank Gevaerts <frank.gevaerts@fks.be> wrote:

| When I changed te ipaq_open code to only submit the read urb after the
| control message succeeds, these disappear.
| 
| Whenever the usb_control_msg returns with ETIMEDOUT (-110), in both code
| variants, when the device is disconnected we get:

 Did you mean that it happens even if you send the read urb after the
control message?

| Jun  1 20:23:55 localhost kernel: ------------[ cut here ]------------
| Jun  1 20:23:55 localhost kernel: kernel BUG at kernel/workqueue.c:110!
| Jun  1 20:23:55 localhost kernel: invalid opcode: 0000 [#1]
| Jun  1 20:23:55 localhost kernel: Modules linked in: ppp_deflate zlib_deflate bsd_comp ppp_async crc_ccitt ppp_generic slhc uhci_hcd ohci_hcd usbhid ipaq usbserial 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
| Jun  1 20:23:55 localhost kernel: CPU:    0
| Jun  1 20:23:55 localhost kernel: EIP:    0060:[queue_work+23/47]    Not tainted VLI
| Jun  1 20:23:55 localhost kernel: EFLAGS: 00010a93   (2.6.17-rc4 #4)
| Jun  1 20:23:55 localhost kernel: EIP is at queue_work+0x17/0x2f
| Jun  1 20:23:55 localhost kernel: eax: c383113c   ebx: b13f29c0   ecx: 00000000   edx: c3831138
| Jun  1 20:23:55 localhost kernel: esi: c44e8d60   edi: ca2f4814   ebp: 00000000   esp: c6bafeb8
| Jun  1 20:23:55 localhost kernel: ds: 007b   es: 007b   ss: 0068
| Jun  1 20:23:55 localhost kernel: Process khubd (pid: 1629, threadinfo=c6bae000 task=cbfd3a90)
| Jun  1 20:23:55 localhost kernel: Stack: <0>c44e8d60 cc9bfad3 c3831000 ca2f4800 cc9bcb40 cc9bcb64 ca2f4814 cc9dd838
| Jun  1 20:23:55 localhost kernel:        ca2f4800 ca2f487c ca2f4814 b01fb254 ca2f4814 ca2f4814 00000000 cc9f0ba0
| Jun  1 20:23:55 localhost kernel:        b01fb419 ca2f4814 b01fac3d ca2f4814 ca2f485c ca2f4814 c5cc3858 00000000
| Jun  1 20:23:55 localhost kernel: Call Trace:
| Jun  1 20:23:55 localhost kernel:  <cc9bfad3> usb_serial_disconnect+0x59/0xa1 [usbserial]   <cc9dd838> usb_unbind_interface+0x36/0x6f [usbcore]
| Jun  1 20:23:55 localhost kernel:  <b01fb254> __device_release_driver+0x5c/0x72   <b01fb419> device_release_driver+0x18/0x26
| Jun  1 20:23:55 localhost kernel:  <b01fac3d> bus_remove_device+0x74/0x8c   <b01fa0cc> device_del+0x39/0x65
| Jun  1 20:23:55 localhost kernel:  <cc9dcaa1> usb_disable_device+0x6a/0xd4 [usbcore]   <cc9d9225> usb_disconnect+0x7c/0xc9 [usbcore]
| Jun  1 20:23:55 localhost kernel:  <cc9d9f3d> hub_thread+0x35b/0x9eb [usbcore]   <b0123f84> autoremove_wake_function+0x0/0x3a
| Jun  1 20:23:55 localhost kernel:  <b0123f36> kthread+0x80/0xc1   <cc9d9be2> hub_thread+0x0/0x9eb [usbcore]
| Jun  1 20:23:55 localhost kernel:  <b0123f4a> kthread+0x94/0xc1   <b0123eb6> kthread+0x0/0xc1
| Jun  1 20:23:55 localhost kernel:  <b0101005> kernel_thread_helper+0x5/0xb
| Jun  1 20:23:55 localhost kernel: 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
| Jun  1 20:23:55 localhost kernel: EIP: [queue_work+23/47] queue_work+0x17/0x2f SS:ESP 0068:c6bafeb8
| 
| This seems to be 100% reproducible. I noticed that serial_open (in
| usb-serial.c) sets port->tty = tty and tty->driver_data = port, but
| doesn't set them back to NULL if the type->open() fails. Is that correct
| ?

 I don't think it would cause the problem you're getting. 'port' is
valid even if the driver's open() function fails.

| Also, we have discovered that the slow response of the ipaq on connect
| is actually largely caused by the control message retries, so we have
| added a sleep at the start of ipaq_open.
| 
| The current patch we are testing (not yet ready for inclusion, since
| it doesn't work correctly yet and it produces some output that is not
| useful in general) is :

 Well, I'm afraid that every new patch leads to a new problem..
Maybe it's something else..

-- 
Luiz Fernando N. Capitulino

Re: [linux-usb-devel] [PATCH] ipaq.c bugfixes

[Frank Gevaerts]

On Fri, Jun 02, 2006 at 09:59:35AM -0300, Luiz Fernando N. Capitulino wrote:
> On Thu, 1 Jun 2006 21:16:54 +0200
> Frank Gevaerts <frank.gevaerts@fks.be> wrote:
> 
> | When I changed te ipaq_open code to only submit the read urb after the
> | control message succeeds, these disappear.
> | 
> | Whenever the usb_control_msg returns with ETIMEDOUT (-110), in both code
> | variants, when the device is disconnected we get:
> 
>  Did you mean that it happens even if you send the read urb after the
> control message?

Yes.

> | This seems to be 100% reproducible. I noticed that serial_open (in
> | usb-serial.c) sets port->tty = tty and tty->driver_data = port, but
> | doesn't set them back to NULL if the type->open() fails. Is that correct
> | ?
> 
>  I don't think it would cause the problem you're getting. 'port' is
> valid even if the driver's open() function fails.

We are currently trying a version where these are both set to NULL on
failure, and it does seem to help.

> | Also, we have discovered that the slow response of the ipaq on connect
> | is actually largely caused by the control message retries, so we have
> | added a sleep at the start of ipaq_open.
> | 
> | The current patch we are testing (not yet ready for inclusion, since
> | it doesn't work correctly yet and it produces some output that is not
> | useful in general) is :
> 
>  Well, I'm afraid that every new patch leads to a new problem..
> Maybe it's something else..

I'm not sure it's a new problem every time. Some of the earlier bugs
could have been the same as this one (the stack trace was the same)

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