Hi Greg,
i get the following backtrace when booting the kernel with "console=ttyUSB0
console=tty0" while using a Keyspan USA-19HS the usb-to-serial converter
connected to a desktop machine:
<snip>
[ 43.782384] usbcore: registered new interface driver usbserial
[ 43.782444] drivers/usb/serial/usb-serial.c: USB Serial Driver core
[ 43.782543] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan - (without firmware)
[ 43.782652] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan 1 port adapter
[ 43.782759] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan 2 port adapter
[ 43.782866] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan 4 port adapter
[ 43.782980] usbcore: registered new interface driver keyspan
[ 43.783040] drivers/usb/serial/keyspan.c: v1.1.5:Keyspan USB to Serial Converter Driver
...
[ 124.816533] usb 3-1: new full speed USB device using uhci_hcd and address 2
[ 125.135811] usb 3-1: configuration #1 chosen from 2 choices
[ 125.140709] keyspan 3-1:1.0: Keyspan 1 port adapter converter detected
[ 125.141110] usb 3-1: Keyspan 1 port adapter converter now attached to ttyUSB0
[ 125.142446] BUG: unable to handle kernel NULL pointer dereference at virtual address 00000084
[ 125.142597] printing eip: c02654ca *pde = 00000000
[ 125.142764] BUG: using smp_processor_id() in preemptible [00000001] code: khubd/142
[ 125.142861] caller is die+0x59/0x1eb
[ 125.142930] [<c0105026>] show_trace_log_lvl+0x1a/0x2f
[ 125.143054] [<c0105a19>] show_trace+0x12/0x14
[ 125.143173] [<c0105b34>] dump_stack+0x16/0x18
[ 125.143293] [<c01e3c2f>] debug_smp_processor_id+0xa3/0xb8
[ 125.143429] [<c0105310>] die+0x59/0x1eb
[ 125.143546] [<c0119b07>] do_page_fault+0x42c/0x505
[ 125.143680] [<c02f8622>] error_code+0x72/0x78
[ 125.143802] [<c0262a50>] usb_console_setup+0x182/0x282
[ 125.143925] [<c0122676>] register_console+0xe9/0x21c
[ 125.144048] [<c02628a4>] usb_serial_console_init+0x31/0x33
[ 125.144171] [<c026190f>] usb_serial_probe+0xe3c/0xf55
[ 125.144293] [<c0253718>] usb_probe_interface+0xb6/0xe7
[ 125.144424] [<c0233b8c>] driver_probe_device+0xcb/0x14f
[ 125.144548] [<c0233c18>] __device_attach+0x8/0xa
[ 125.144678] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
[ 125.144799] [<c0233ca9>] device_attach+0x70/0x85
[ 125.144919] [<c0232f2f>] bus_attach_device+0x29/0x77
[ 125.145040] [<c0232044>] device_add+0x302/0x514
[ 125.145160] [<c02521ff>] usb_set_configuration+0x418/0x46d
[ 125.145283] [<c0258b67>] generic_probe+0x53/0x94
[ 125.145403] [<c02534a9>] usb_probe_device+0x38/0x3e
[ 125.145523] [<c0233b8c>] driver_probe_device+0xcb/0x14f
[ 125.145656] [<c0233c18>] __device_attach+0x8/0xa
[ 125.145776] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
[ 125.146675] [<c0233ca9>] device_attach+0x70/0x85
[ 125.146795] [<c0232f2f>] bus_attach_device+0x29/0x77
[ 125.146916] [<c0232044>] device_add+0x302/0x514
[ 125.147036] [<c024dc69>] usb_new_device+0x44/0x82
[ 125.147160] [<c024eeaa>] hub_thread+0x65a/0xa13
[ 125.147280] [<c0132daf>] kthread+0x3b/0x64
[ 125.147400] [<c0104c87>] kernel_thread_helper+0x7/0x10
[ 125.147521] =======================
[ 125.147600] Oops: 0000 [#1] PREEMPT SMP
[ 125.147805] Modules linked in: usbhid video output tg3 intel_agp uhci_hcd psmouse agpgart rtc evdev
[ 125.148403]
[ 125.148466] Pid: 142, comm: khubd Not tainted (2.6.24-rc1-521-g54866f0 #16)
[ 125.148546] BUG: using smp_processor_id() in preemptible [00000001] code: khubd/142
[ 125.148646] caller is __show_registers+0xad/0x1d8
[ 125.148717] [<c0105026>] show_trace_log_lvl+0x1a/0x2f
[ 125.148839] [<c0105a19>] show_trace+0x12/0x14
[ 125.148958] [<c0105b34>] dump_stack+0x16/0x18
[ 125.149077] [<c01e3c2f>] debug_smp_processor_id+0xa3/0xb8
[ 125.149201] [<c0102344>] __show_registers+0xad/0x1d8
[ 125.149321] [<c01050f7>] show_registers+0x19/0x1d9
[ 125.149440] [<c01053d6>] die+0x11f/0x1eb
[ 125.149557] [<c0119b07>] do_page_fault+0x42c/0x505
[ 125.149688] [<c02f8622>] error_code+0x72/0x78
[ 125.149808] [<c0262a50>] usb_console_setup+0x182/0x282
[ 125.149930] [<c0122676>] register_console+0xe9/0x21c
[ 125.150051] [<c02628a4>] usb_serial_console_init+0x31/0x33
[ 125.150175] [<c026190f>] usb_serial_probe+0xe3c/0xf55
[ 125.150296] [<c0253718>] usb_probe_interface+0xb6/0xe7
[ 125.150416] [<c0233b8c>] driver_probe_device+0xcb/0x14f
[ 125.150538] [<c0233c18>] __device_attach+0x8/0xa
[ 125.150669] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
[ 125.150792] [<c0233ca9>] device_attach+0x70/0x85
[ 125.150913] [<c0232f2f>] bus_attach_device+0x29/0x77
[ 125.151036] [<c0232044>] device_add+0x302/0x514
[ 125.151156] [<c02521ff>] usb_set_configuration+0x418/0x46d
[ 125.151281] [<c0258b67>] generic_probe+0x53/0x94
[ 125.151402] [<c02534a9>] usb_probe_device+0x38/0x3e
[ 125.151523] [<c0233b8c>] driver_probe_device+0xcb/0x14f
[ 125.151658] [<c0233c18>] __device_attach+0x8/0xa
[ 125.151780] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
[ 125.151902] [<c0233ca9>] device_attach+0x70/0x85
[ 125.152023] [<c0232f2f>] bus_attach_device+0x29/0x77
[ 125.152146] [<c0232044>] device_add+0x302/0x514
[ 125.152266] [<c024dc69>] usb_new_device+0x44/0x82
[ 125.152388] [<c024eeaa>] hub_thread+0x65a/0xa13
[ 125.152508] [<c0132daf>] kthread+0x3b/0x64
[ 125.152633] [<c0104c87>] kernel_thread_helper+0x7/0x10
[ 125.152754] =======================
[ 125.152822] EIP: 0060:[<c02654ca>] EFLAGS: 00010246 CPU: 0
[ 125.152901] EIP is at keyspan_open+0x11c/0x19d
[ 125.152971] EAX: 00000000 EBX: c19e7c00 ECX: c19e7c00 EDX: c19e7c00
[ 125.153047] ESI: c1bd3e00 EDI: 00000002 EBP: c1886b54 ESP: c1886b1c
[ 125.153123] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 125.153197] Process khubd (pid: 142, ti=c1886000 task=c1843080 task.ti=c1886000)
[ 125.153274] Stack: 00000246 22222222 22222222 22222222 22222222 c03eeb40 c19e7c00 0000006e
[ 125.153775] c1aeab40 c030fcec c02f72b5 c19e7c00 0000006e c03eed18 c1886bb0 c0262a50
[ 125.154264] c02316d6 c1886b68 c0232e33 c1aeab40 c1886b78 c01dd877 00000cbd c1886b80
[ 125.154762] Call Trace:
[ 125.154875] [<c0105026>] show_trace_log_lvl+0x1a/0x2f
[ 125.154995] [<c01050d6>] show_stack_log_lvl+0x9b/0xa3
[ 125.155115] [<c0105182>] show_registers+0xa4/0x1d9
[ 125.155235] [<c01053d6>] die+0x11f/0x1eb
[ 125.155352] [<c0119b07>] do_page_fault+0x42c/0x505
[ 125.155472] [<c02f8622>] error_code+0x72/0x78
[ 125.155654] [<c0262a50>] usb_console_setup+0x182/0x282
[ 125.155774] [<c0122676>] register_console+0xe9/0x21c
[ 125.155893] [<c02628a4>] usb_serial_console_init+0x31/0x33
[ 125.156013] [<c026190f>] usb_serial_probe+0xe3c/0xf55
[ 125.156131] [<c0253718>] usb_probe_interface+0xb6/0xe7
[ 125.156251] [<c0233b8c>] driver_probe_device+0xcb/0x14f
[ 125.156370] [<c0233c18>] __device_attach+0x8/0xa
[ 125.156488] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
[ 125.156616] [<c0233ca9>] device_attach+0x70/0x85
[ 125.156734] [<c0232f2f>] bus_attach_device+0x29/0x77
[ 125.156852] [<c0232044>] device_add+0x302/0x514
[ 125.156969] [<c02521ff>] usb_set_configuration+0x418/0x46d
[ 125.157089] [<c0258b67>] generic_probe+0x53/0x94
[ 125.157207] [<c02534a9>] usb_probe_device+0x38/0x3e
[ 125.157325] [<c0233b8c>] driver_probe_device+0xcb/0x14f
[ 125.157445] [<c0233c18>] __device_attach+0x8/0xa
[ 125.157562] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
[ 125.157689] [<c0233ca9>] device_attach+0x70/0x85
[ 125.157806] [<c0232f2f>] bus_attach_device+0x29/0x77
[ 125.157925] [<c0232044>] device_add+0x302/0x514
[ 125.158041] [<c024dc69>] usb_new_device+0x44/0x82
[ 125.158160] [<c024eeaa>] hub_thread+0x65a/0xa13
[ 125.158277] [<c0132daf>] kthread+0x3b/0x64
[ 125.158393] [<c0104c87>] kernel_thread_helper+0x7/0x10
[ 125.158512] =======================
[ 125.158577] Code: 74 08 8b 4d e8 8b 01 89 42 28 8b 96 98 00 00 00 85 d2 74 08 8b 5d e8 8b 03 89 42 28 8b 55 e0 8b 4d e0 8b 5d e0 8b 42 04 8a 49 48 <8b> 90 84 00 00 00 8b 7a 08 88 4d f3 8b 13 8a 52 0c 88 55 e7 e8
[ 50.035324] EIP: [<c02654ca>] keyspan_open+0x11c/0x19d SS:ESP 0068:c1886b1c
</snip>
and this happens, imho, because in usb_console_setup(), port->tty is set to NULL
prior to calling serial->type->open() which is keyspan_open() in this case. In
keyspan_open(), otoh, some premature terminal config is done for the purposes of
the setup message by deref'ing, among others, port->tty->termios->c_flag, which,
as we saw before :) is NULL and BAM! The patch below is against current git
(v2.6.24-rc1-573-g74521c2).
---
From: Borislav Petkov <[email protected]>
Remove redundant code leading to NULL ptr deref and let terminal config settings
take place in the proper initialization path in usb_console_setup().
Signed-off-by: Borislav Petkov <[email protected]>
--
drivers/usb/serial/keyspan.c | 38 ++++++--------------------------------
1 files changed, 6 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
index 6bfdba6..1f7ab15 100644
--- a/drivers/usb/serial/keyspan.c
+++ b/drivers/usb/serial/keyspan.c
@@ -1215,20 +1215,18 @@ static int keyspan_chars_in_buffer (struct usb_serial_port *port)
static int keyspan_open (struct usb_serial_port *port, struct file *filp)
{
- struct keyspan_port_private *p_priv;
- struct keyspan_serial_private *s_priv;
- struct usb_serial *serial = port->serial;
+ struct keyspan_port_private *p_priv;
+ struct keyspan_serial_private *s_priv;
+ struct usb_serial *serial = port->serial;
const struct keyspan_device_details *d_details;
int i, err;
- int baud_rate, device_port;
struct urb *urb;
- unsigned int cflag;
s_priv = usb_get_serial_data(serial);
p_priv = usb_get_serial_port_data(port);
d_details = p_priv->device_details;
-
- dbg("%s - port%d.", __FUNCTION__, port->number);
+
+ dbg("%s - port%d.", __FUNCTION__, port->number);
/* Set some sane defaults */
p_priv->rts_state = 1;
@@ -1249,7 +1247,7 @@ static int keyspan_open (struct usb_serial_port *port, struct file *filp)
urb->dev = serial->dev;
/* make sure endpoint data toggle is synchronized with the device */
-
+
usb_clear_halt(urb->dev, urb->pipe);
if ((err = usb_submit_urb(urb, GFP_KERNEL)) != 0) {
@@ -1265,30 +1263,6 @@ static int keyspan_open (struct usb_serial_port *port, struct file *filp)
/* usb_settoggle(urb->dev, usb_pipeendpoint(urb->pipe), usb_pipeout(urb->pipe), 0); */
}
- /* get the terminal config for the setup message now so we don't
- * need to send 2 of them */
-
- cflag = port->tty->termios->c_cflag;
- device_port = port->number - port->serial->minor;
-
- /* Baud rate calculation takes baud rate as an integer
- so other rates can be generated if desired. */
- baud_rate = tty_get_baud_rate(port->tty);
- /* If no match or invalid, leave as default */
- if (baud_rate >= 0
- && d_details->calculate_baud_rate(baud_rate, d_details->baudclk,
- NULL, NULL, NULL, device_port) == KEYSPAN_BAUD_RATE_OK) {
- p_priv->baud = baud_rate;
- }
-
- /* set CTS/RTS handshake etc. */
- p_priv->cflag = cflag;
- p_priv->flow_control = (cflag & CRTSCTS)? flow_cts: flow_none;
-
- keyspan_send_setup(port, 1);
- //mdelay(100);
- //keyspan_set_termios(port, NULL);
-
return (0);
}
--
Regards/Gru?,
Boris.
Greg & Boris,
I've been out of town and just saw this.
This patch will work with the 19HS but WILL BREAK all other Keyspan
adapters. It will take me a few days to get to looking at a correct fix but
that keyspan_send_setup(port, 1) (and the '1' is the important part)
must happen once
when the port is first opened. The cflag can just be set to whatever
the normal default
is for your serial environment. What are the defaults?
Lucy
Borislav Petkov wrote:
> Hi Greg,
>
> i get the following backtrace when booting the kernel with "console=ttyUSB0
> console=tty0" while using a Keyspan USA-19HS the usb-to-serial converter
> connected to a desktop machine:
>
> <snip>
> [ 43.782384] usbcore: registered new interface driver usbserial
> [ 43.782444] drivers/usb/serial/usb-serial.c: USB Serial Driver core
> [ 43.782543] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan - (without firmware)
> [ 43.782652] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan 1 port adapter
> [ 43.782759] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan 2 port adapter
> [ 43.782866] drivers/usb/serial/usb-serial.c: USB Serial support registered for Keyspan 4 port adapter
> [ 43.782980] usbcore: registered new interface driver keyspan
> [ 43.783040] drivers/usb/serial/keyspan.c: v1.1.5:Keyspan USB to Serial Converter Driver
> ...
> [ 124.816533] usb 3-1: new full speed USB device using uhci_hcd and address 2
> [ 125.135811] usb 3-1: configuration #1 chosen from 2 choices
> [ 125.140709] keyspan 3-1:1.0: Keyspan 1 port adapter converter detected
> [ 125.141110] usb 3-1: Keyspan 1 port adapter converter now attached to ttyUSB0
> [ 125.142446] BUG: unable to handle kernel NULL pointer dereference at virtual address 00000084
> [ 125.142597] printing eip: c02654ca *pde = 00000000
> [ 125.142764] BUG: using smp_processor_id() in preemptible [00000001] code: khubd/142
> [ 125.142861] caller is die+0x59/0x1eb
> [ 125.142930] [<c0105026>] show_trace_log_lvl+0x1a/0x2f
> [ 125.143054] [<c0105a19>] show_trace+0x12/0x14
> [ 125.143173] [<c0105b34>] dump_stack+0x16/0x18
> [ 125.143293] [<c01e3c2f>] debug_smp_processor_id+0xa3/0xb8
> [ 125.143429] [<c0105310>] die+0x59/0x1eb
> [ 125.143546] [<c0119b07>] do_page_fault+0x42c/0x505
> [ 125.143680] [<c02f8622>] error_code+0x72/0x78
> [ 125.143802] [<c0262a50>] usb_console_setup+0x182/0x282
> [ 125.143925] [<c0122676>] register_console+0xe9/0x21c
> [ 125.144048] [<c02628a4>] usb_serial_console_init+0x31/0x33
> [ 125.144171] [<c026190f>] usb_serial_probe+0xe3c/0xf55
> [ 125.144293] [<c0253718>] usb_probe_interface+0xb6/0xe7
> [ 125.144424] [<c0233b8c>] driver_probe_device+0xcb/0x14f
> [ 125.144548] [<c0233c18>] __device_attach+0x8/0xa
> [ 125.144678] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
> [ 125.144799] [<c0233ca9>] device_attach+0x70/0x85
> [ 125.144919] [<c0232f2f>] bus_attach_device+0x29/0x77
> [ 125.145040] [<c0232044>] device_add+0x302/0x514
> [ 125.145160] [<c02521ff>] usb_set_configuration+0x418/0x46d
> [ 125.145283] [<c0258b67>] generic_probe+0x53/0x94
> [ 125.145403] [<c02534a9>] usb_probe_device+0x38/0x3e
> [ 125.145523] [<c0233b8c>] driver_probe_device+0xcb/0x14f
> [ 125.145656] [<c0233c18>] __device_attach+0x8/0xa
> [ 125.145776] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
> [ 125.146675] [<c0233ca9>] device_attach+0x70/0x85
> [ 125.146795] [<c0232f2f>] bus_attach_device+0x29/0x77
> [ 125.146916] [<c0232044>] device_add+0x302/0x514
> [ 125.147036] [<c024dc69>] usb_new_device+0x44/0x82
> [ 125.147160] [<c024eeaa>] hub_thread+0x65a/0xa13
> [ 125.147280] [<c0132daf>] kthread+0x3b/0x64
> [ 125.147400] [<c0104c87>] kernel_thread_helper+0x7/0x10
> [ 125.147521] =======================
> [ 125.147600] Oops: 0000 [#1] PREEMPT SMP
> [ 125.147805] Modules linked in: usbhid video output tg3 intel_agp uhci_hcd psmouse agpgart rtc evdev
> [ 125.148403]
> [ 125.148466] Pid: 142, comm: khubd Not tainted (2.6.24-rc1-521-g54866f0 #16)
> [ 125.148546] BUG: using smp_processor_id() in preemptible [00000001] code: khubd/142
> [ 125.148646] caller is __show_registers+0xad/0x1d8
> [ 125.148717] [<c0105026>] show_trace_log_lvl+0x1a/0x2f
> [ 125.148839] [<c0105a19>] show_trace+0x12/0x14
> [ 125.148958] [<c0105b34>] dump_stack+0x16/0x18
> [ 125.149077] [<c01e3c2f>] debug_smp_processor_id+0xa3/0xb8
> [ 125.149201] [<c0102344>] __show_registers+0xad/0x1d8
> [ 125.149321] [<c01050f7>] show_registers+0x19/0x1d9
> [ 125.149440] [<c01053d6>] die+0x11f/0x1eb
> [ 125.149557] [<c0119b07>] do_page_fault+0x42c/0x505
> [ 125.149688] [<c02f8622>] error_code+0x72/0x78
> [ 125.149808] [<c0262a50>] usb_console_setup+0x182/0x282
> [ 125.149930] [<c0122676>] register_console+0xe9/0x21c
> [ 125.150051] [<c02628a4>] usb_serial_console_init+0x31/0x33
> [ 125.150175] [<c026190f>] usb_serial_probe+0xe3c/0xf55
> [ 125.150296] [<c0253718>] usb_probe_interface+0xb6/0xe7
> [ 125.150416] [<c0233b8c>] driver_probe_device+0xcb/0x14f
> [ 125.150538] [<c0233c18>] __device_attach+0x8/0xa
> [ 125.150669] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
> [ 125.150792] [<c0233ca9>] device_attach+0x70/0x85
> [ 125.150913] [<c0232f2f>] bus_attach_device+0x29/0x77
> [ 125.151036] [<c0232044>] device_add+0x302/0x514
> [ 125.151156] [<c02521ff>] usb_set_configuration+0x418/0x46d
> [ 125.151281] [<c0258b67>] generic_probe+0x53/0x94
> [ 125.151402] [<c02534a9>] usb_probe_device+0x38/0x3e
> [ 125.151523] [<c0233b8c>] driver_probe_device+0xcb/0x14f
> [ 125.151658] [<c0233c18>] __device_attach+0x8/0xa
> [ 125.151780] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
> [ 125.151902] [<c0233ca9>] device_attach+0x70/0x85
> [ 125.152023] [<c0232f2f>] bus_attach_device+0x29/0x77
> [ 125.152146] [<c0232044>] device_add+0x302/0x514
> [ 125.152266] [<c024dc69>] usb_new_device+0x44/0x82
> [ 125.152388] [<c024eeaa>] hub_thread+0x65a/0xa13
> [ 125.152508] [<c0132daf>] kthread+0x3b/0x64
> [ 125.152633] [<c0104c87>] kernel_thread_helper+0x7/0x10
> [ 125.152754] =======================
> [ 125.152822] EIP: 0060:[<c02654ca>] EFLAGS: 00010246 CPU: 0
> [ 125.152901] EIP is at keyspan_open+0x11c/0x19d
> [ 125.152971] EAX: 00000000 EBX: c19e7c00 ECX: c19e7c00 EDX: c19e7c00
> [ 125.153047] ESI: c1bd3e00 EDI: 00000002 EBP: c1886b54 ESP: c1886b1c
> [ 125.153123] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> [ 125.153197] Process khubd (pid: 142, ti=c1886000 task=c1843080 task.ti=c1886000)
> [ 125.153274] Stack: 00000246 22222222 22222222 22222222 22222222 c03eeb40 c19e7c00 0000006e
> [ 125.153775] c1aeab40 c030fcec c02f72b5 c19e7c00 0000006e c03eed18 c1886bb0 c0262a50
> [ 125.154264] c02316d6 c1886b68 c0232e33 c1aeab40 c1886b78 c01dd877 00000cbd c1886b80
> [ 125.154762] Call Trace:
> [ 125.154875] [<c0105026>] show_trace_log_lvl+0x1a/0x2f
> [ 125.154995] [<c01050d6>] show_stack_log_lvl+0x9b/0xa3
> [ 125.155115] [<c0105182>] show_registers+0xa4/0x1d9
> [ 125.155235] [<c01053d6>] die+0x11f/0x1eb
> [ 125.155352] [<c0119b07>] do_page_fault+0x42c/0x505
> [ 125.155472] [<c02f8622>] error_code+0x72/0x78
> [ 125.155654] [<c0262a50>] usb_console_setup+0x182/0x282
> [ 125.155774] [<c0122676>] register_console+0xe9/0x21c
> [ 125.155893] [<c02628a4>] usb_serial_console_init+0x31/0x33
> [ 125.156013] [<c026190f>] usb_serial_probe+0xe3c/0xf55
> [ 125.156131] [<c0253718>] usb_probe_interface+0xb6/0xe7
> [ 125.156251] [<c0233b8c>] driver_probe_device+0xcb/0x14f
> [ 125.156370] [<c0233c18>] __device_attach+0x8/0xa
> [ 125.156488] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
> [ 125.156616] [<c0233ca9>] device_attach+0x70/0x85
> [ 125.156734] [<c0232f2f>] bus_attach_device+0x29/0x77
> [ 125.156852] [<c0232044>] device_add+0x302/0x514
> [ 125.156969] [<c02521ff>] usb_set_configuration+0x418/0x46d
> [ 125.157089] [<c0258b67>] generic_probe+0x53/0x94
> [ 125.157207] [<c02534a9>] usb_probe_device+0x38/0x3e
> [ 125.157325] [<c0233b8c>] driver_probe_device+0xcb/0x14f
> [ 125.157445] [<c0233c18>] __device_attach+0x8/0xa
> [ 125.157562] [<c0232fb8>] bus_for_each_drv+0x3b/0x63
> [ 125.157689] [<c0233ca9>] device_attach+0x70/0x85
> [ 125.157806] [<c0232f2f>] bus_attach_device+0x29/0x77
> [ 125.157925] [<c0232044>] device_add+0x302/0x514
> [ 125.158041] [<c024dc69>] usb_new_device+0x44/0x82
> [ 125.158160] [<c024eeaa>] hub_thread+0x65a/0xa13
> [ 125.158277] [<c0132daf>] kthread+0x3b/0x64
> [ 125.158393] [<c0104c87>] kernel_thread_helper+0x7/0x10
> [ 125.158512] =======================
> [ 125.158577] Code: 74 08 8b 4d e8 8b 01 89 42 28 8b 96 98 00 00 00 85 d2 74 08 8b 5d e8 8b 03 89 42 28 8b 55 e0 8b 4d e0 8b 5d e0 8b 42 04 8a 49 48 <8b> 90 84 00 00 00 8b 7a 08 88 4d f3 8b 13 8a 52 0c 88 55 e7 e8
> [ 50.035324] EIP: [<c02654ca>] keyspan_open+0x11c/0x19d SS:ESP 0068:c1886b1c
>
> </snip>
>
> and this happens, imho, because in usb_console_setup(), port->tty is set to NULL
> prior to calling serial->type->open() which is keyspan_open() in this case. In
> keyspan_open(), otoh, some premature terminal config is done for the purposes of
> the setup message by deref'ing, among others, port->tty->termios->c_flag, which,
> as we saw before :) is NULL and BAM! The patch below is against current git
> (v2.6.24-rc1-573-g74521c2).
>
> ---
> From: Borislav Petkov <[email protected]>
>
> Remove redundant code leading to NULL ptr deref and let terminal config settings
> take place in the proper initialization path in usb_console_setup().
>
> Signed-off-by: Borislav Petkov <[email protected]>
> --
>
> drivers/usb/serial/keyspan.c | 38 ++++++--------------------------------
> 1 files changed, 6 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
> index 6bfdba6..1f7ab15 100644
> --- a/drivers/usb/serial/keyspan.c
> +++ b/drivers/usb/serial/keyspan.c
> @@ -1215,20 +1215,18 @@ static int keyspan_chars_in_buffer (struct usb_serial_port *port)
>
> static int keyspan_open (struct usb_serial_port *port, struct file *filp)
> {
> - struct keyspan_port_private *p_priv;
> - struct keyspan_serial_private *s_priv;
> - struct usb_serial *serial = port->serial;
> + struct keyspan_port_private *p_priv;
> + struct keyspan_serial_private *s_priv;
> + struct usb_serial *serial = port->serial;
> const struct keyspan_device_details *d_details;
> int i, err;
> - int baud_rate, device_port;
> struct urb *urb;
> - unsigned int cflag;
>
> s_priv = usb_get_serial_data(serial);
> p_priv = usb_get_serial_port_data(port);
> d_details = p_priv->device_details;
> -
> - dbg("%s - port%d.", __FUNCTION__, port->number);
> +
> + dbg("%s - port%d.", __FUNCTION__, port->number);
>
> /* Set some sane defaults */
> p_priv->rts_state = 1;
> @@ -1249,7 +1247,7 @@ static int keyspan_open (struct usb_serial_port *port, struct file *filp)
> urb->dev = serial->dev;
>
> /* make sure endpoint data toggle is synchronized with the device */
> -
> +
> usb_clear_halt(urb->dev, urb->pipe);
>
> if ((err = usb_submit_urb(urb, GFP_KERNEL)) != 0) {
> @@ -1265,30 +1263,6 @@ static int keyspan_open (struct usb_serial_port *port, struct file *filp)
> /* usb_settoggle(urb->dev, usb_pipeendpoint(urb->pipe), usb_pipeout(urb->pipe), 0); */
> }
>
> - /* get the terminal config for the setup message now so we don't
> - * need to send 2 of them */
> -
> - cflag = port->tty->termios->c_cflag;
> - device_port = port->number - port->serial->minor;
> -
> - /* Baud rate calculation takes baud rate as an integer
> - so other rates can be generated if desired. */
> - baud_rate = tty_get_baud_rate(port->tty);
> - /* If no match or invalid, leave as default */
> - if (baud_rate >= 0
> - && d_details->calculate_baud_rate(baud_rate, d_details->baudclk,
> - NULL, NULL, NULL, device_port) == KEYSPAN_BAUD_RATE_OK) {
> - p_priv->baud = baud_rate;
> - }
> -
> - /* set CTS/RTS handshake etc. */
> - p_priv->cflag = cflag;
> - p_priv->flow_control = (cflag & CRTSCTS)? flow_cts: flow_none;
> -
> - keyspan_send_setup(port, 1);
> - //mdelay(100);
> - //keyspan_set_termios(port, NULL);
> -
> return (0);
> }
>
>
On Thu, 15 Nov 2007 09:49:22 -0800 Lucy McCoy <[email protected]> wrote:
> Greg & Boris,
>
> I've been out of town and just saw this.
>
> This patch will work with the 19HS but WILL BREAK all other Keyspan
> adapters. It will take me a few days to get to looking at a correct fix but
> that keyspan_send_setup(port, 1) (and the '1' is the important part)
> must happen once
> when the port is first opened. The cflag can just be set to whatever
> the normal default
> is for your serial environment. What are the defaults?
>
drat, this patch just went into mainline.
If you're sure that it'll break things, I'll revert it while you cook up
something better, OK?
Yes, it will break things. I'll try to get to it soon...
Andrew Morton wrote:
> On Thu, 15 Nov 2007 09:49:22 -0800 Lucy McCoy <[email protected]> wrote:
>
>
>> Greg & Boris,
>>
>> I've been out of town and just saw this.
>>
>> This patch will work with the 19HS but WILL BREAK all other Keyspan
>> adapters. It will take me a few days to get to looking at a correct fix but
>> that keyspan_send_setup(port, 1) (and the '1' is the important part)
>> must happen once
>> when the port is first opened. The cflag can just be set to whatever
>> the normal default
>> is for your serial environment. What are the defaults?
>>
>>
>
> drat, this patch just went into mainline.
>
> If you're sure that it'll break things, I'll revert it while you cook up
> something better, OK?
>
>
>
On Thu, Nov 15, 2007 at 09:49:22AM -0800, Lucy McCoy wrote:
> Greg & Boris,
>
> I've been out of town and just saw this.
>
> This patch will work with the 19HS but WILL BREAK all other Keyspan
> adapters. It will take me a few days to get to looking at a correct fix but
> that keyspan_send_setup(port, 1) (and the '1' is the important part) must
> happen once
> when the port is first opened. The cflag can just be set to whatever the
> normal default
> is for your serial environment. What are the defaults?
Hi Lucy,
sorry but i don't have the other usb-to-serial models to test them too, i guess
you're in the position to do that :) Well, from what i've unterstood from
reading the code, you can still do the keyspan_send_setup() call since the
switch-case-statement is not handling the 19s series so while this call won't have any
influence on them, it'll still init the others properly, no? Anyway, you can send me your
fix for testing.
Thanks.
--
Regards/Gru?,
Boris.
Hi Boris,
Actually the 19HS is the usa90 so it is included in the switch but
that isn't a problem.
I'm not familiar with the termios stuff on Linux so can you take a look
at the following modified code to see if this solves your NULL ptr problem?
Also,....
Does cflag need to be set to anything other than CS8?
Is keyspan_open_port() always followed by a keyspan_set_termios()?
Thanks,
Lucy
static int keyspan_open (struct usb_serial_port *port, struct file *filp)
{
struct keyspan_port_private *p_priv;
struct keyspan_serial_private *s_priv;
struct usb_serial *serial = port->serial;
const struct keyspan_device_details *d_details;
int i, err;
struct urb *urb;
s_priv = usb_get_serial_data(serial);
p_priv = usb_get_serial_port_data(port);
d_details = p_priv->device_details;
dbg("%s - port%d.", __FUNCTION__, port->number);
/* Set some sane defaults */
p_priv->rts_state = 1;
p_priv->dtr_state = 1;
p_priv->baud = 9600;
/* set CTS/RTS handshake etc. */
p_priv->cflag = CS8; /* 8/N/1 */
p_priv->flow_control = flow_none;
/* force baud and lcr to be set on open */
p_priv->old_baud = 0;
p_priv->old_cflag = 0;
p_priv->out_flip = 0;
p_priv->in_flip = 0;
/* Reset low level data toggle and start reading from endpoints */
for (i = 0; i < 2; i++) {
if ((urb = p_priv->in_urbs[i]) == NULL)
continue;
urb->dev = serial->dev;
/* make sure endpoint data toggle is synchronized with the device */
usb_clear_halt(urb->dev, urb->pipe);
if ((err = usb_submit_urb(urb, GFP_KERNEL)) != 0) {
dbg("%s - submit urb %d failed (%d)", __FUNCTION__, i, err);
}
}
/* Reset low level data toggle on out endpoints */
for (i = 0; i < 2; i++) {
if ((urb = p_priv->out_urbs[i]) == NULL)
continue;
urb->dev = serial->dev;
/* usb_settoggle(urb->dev, usb_pipeendpoint(urb->pipe), usb_pipeout(urb->pipe), 0); */
}
keyspan_send_setup(port, 1);
//mdelay(100);
//keyspan_set_termios(port, NULL);
return (0);
}
Lucy McCoy
[email protected]
-----Original Message-----
From: Borislav Petkov <[email protected]>
Sent: Thursday, November 15, 2007 12:40pm
To: Lucy McCoy <[email protected]>
Cc: Greg KH <[email protected]>, [email protected], Andrew Morton <[email protected]>
Subject: Re: [PATCH] keyspan: init termios properly
On Thu, Nov 15, 2007 at 09:49:22AM -0800, Lucy McCoy wrote:
> Greg & Boris,
>
> I've been out of town and just saw this.
>
> This patch will work with the 19HS but WILL BREAK all other Keyspan
> adapters. It will take me a few days to get to looking at a correct fix but
> that keyspan_send_setup(port, 1) (and the '1' is the important part) must
> happen once
> when the port is first opened. The cflag can just be set to whatever the
> normal default
> is for your serial environment. What are the defaults?
Hi Lucy,
sorry but i don't have the other usb-to-serial models to test them too, i guess
you're in the position to do that :) Well, from what i've unterstood from
reading the code, you can still do the keyspan_send_setup() call since the
switch-case-statement is not handling the 19s series so while this call won't have any
influence on them, it'll still init the others properly, no? Anyway, you can send me your
fix for testing.
Thanks.
--
Regards/Gruß,
Boris.
On Thu, Nov 15, 2007 at 01:10:16PM -0800, Lucy McCoy wrote:
Hi Lucy,
> I'm not familiar with the termios stuff on Linux so can you take a look
> at the following modified code to see if this solves your NULL ptr problem?
will test the patch tomorrow and get back to you with results.
> Does cflag need to be set to anything other than CS8?
i'm not that familiar with serial port settings either, but IIRC the cflag is
used to set the requested byte size of the transferred chars (cf
http://en.wikipedia.org/wiki/Serial_port#Data_Bits). Thus, according to the
above CS8 is used for setting up transfer of any kind of data over the serial connection.
> Is keyspan_open_port() always followed by a keyspan_set_termios()?
err, you mean keyspan_open() ...
..and yes, almost all of the usb to serial converter drivers call their own
*_set_termios-routine so that they could "synchronize" the line discipline user
settings with those of the converter.
--
Regards/Gru?,
Boris.
On Thu, Nov 15, 2007 at 01:10:16PM -0800, Lucy McCoy wrote:
> static int keyspan_open (struct usb_serial_port *port, struct file *filp)
> {
> struct keyspan_port_private *p_priv;
> struct keyspan_serial_private *s_priv;
> struct usb_serial *serial = port->serial;
> const struct keyspan_device_details *d_details;
> int i, err;
> struct urb *urb;
>
> s_priv = usb_get_serial_data(serial);
> p_priv = usb_get_serial_port_data(port);
> d_details = p_priv->device_details;
>
> dbg("%s - port%d.", __FUNCTION__, port->number);
>
> /* Set some sane defaults */
> p_priv->rts_state = 1;
> p_priv->dtr_state = 1;
> p_priv->baud = 9600;
>
> /* set CTS/RTS handshake etc. */
> p_priv->cflag = CS8; /* 8/N/1 */
> p_priv->flow_control = flow_none;
>
> /* force baud and lcr to be set on open */
> p_priv->old_baud = 0;
> p_priv->old_cflag = 0;
>
> p_priv->out_flip = 0;
> p_priv->in_flip = 0;
>
> /* Reset low level data toggle and start reading from endpoints */
> for (i = 0; i < 2; i++) {
> if ((urb = p_priv->in_urbs[i]) == NULL)
> continue;
> urb->dev = serial->dev;
>
> /* make sure endpoint data toggle is synchronized with the device */
>
> usb_clear_halt(urb->dev, urb->pipe);
>
> if ((err = usb_submit_urb(urb, GFP_KERNEL)) != 0) {
> dbg("%s - submit urb %d failed (%d)", __FUNCTION__, i, err);
> }
> }
>
> /* Reset low level data toggle on out endpoints */
> for (i = 0; i < 2; i++) {
> if ((urb = p_priv->out_urbs[i]) == NULL)
> continue;
> urb->dev = serial->dev;
> /* usb_settoggle(urb->dev, usb_pipeendpoint(urb->pipe), usb_pipeout(urb->pipe), 0); */
> }
>
> keyspan_send_setup(port, 1);
> //mdelay(100);
> //keyspan_set_termios(port, NULL);
>
> return (0);
> }
>
yes, after testing this i can confirm that this one fixes the NULL ptr
problem here so you might want to submit a proper patch to Greg.
--
Regards/Gru?,
Boris.
On Sun, 18 Nov 2007 14:11:30 +0100
Borislav Petkov <[email protected]> wrote:
> On Thu, Nov 15, 2007 at 01:10:16PM -0800, Lucy McCoy wrote:
>
> > static int keyspan_open (struct usb_serial_port *port, struct file *filp)
> > {
> > struct keyspan_port_private *p_priv;
> > struct keyspan_serial_private *s_priv;
> > struct usb_serial *serial = port->serial;
> > const struct keyspan_device_details *d_details;
> > int i, err;
> > struct urb *urb;
> >
> > s_priv = usb_get_serial_data(serial);
> > p_priv = usb_get_serial_port_data(port);
> > d_details = p_priv->device_details;
> >
> > dbg("%s - port%d.", __FUNCTION__, port->number);
> >
> > /* Set some sane defaults */
> > p_priv->rts_state = 1;
> > p_priv->dtr_state = 1;
> > p_priv->baud = 9600;
> >
> > /* set CTS/RTS handshake etc. */
> > p_priv->cflag = CS8; /* 8/N/1 */
> > p_priv->flow_control = flow_none;
> >
> > /* force baud and lcr to be set on open */
> > p_priv->old_baud = 0;
> > p_priv->old_cflag = 0;
> >
> > p_priv->out_flip = 0;
> > p_priv->in_flip = 0;
> >
> > /* Reset low level data toggle and start reading from endpoints */
> > for (i = 0; i < 2; i++) {
> > if ((urb = p_priv->in_urbs[i]) == NULL)
> > continue;
> > urb->dev = serial->dev;
> >
> > /* make sure endpoint data toggle is synchronized with the device */
> >
> > usb_clear_halt(urb->dev, urb->pipe);
> >
> > if ((err = usb_submit_urb(urb, GFP_KERNEL)) != 0) {
> > dbg("%s - submit urb %d failed (%d)", __FUNCTION__, i, err);
> > }
> > }
> >
> > /* Reset low level data toggle on out endpoints */
> > for (i = 0; i < 2; i++) {
> > if ((urb = p_priv->out_urbs[i]) == NULL)
> > continue;
> > urb->dev = serial->dev;
> > /* usb_settoggle(urb->dev, usb_pipeendpoint(urb->pipe), usb_pipeout(urb->pipe), 0); */
> > }
> >
> > keyspan_send_setup(port, 1);
> > //mdelay(100);
> > //keyspan_set_termios(port, NULL);
> >
> > return (0);
> > }
> >
>
> yes, after testing this i can confirm that this one fixes the NULL ptr
> problem here so you might want to submit a proper patch to Greg.
I'll merge revert-keyspan-init-termios-properly.patch soon, but afaik we
are still awaiting the real fix for this problem?
On Mon, Nov 26, 2007 at 02:18:52PM -0800, Andrew Morton wrote:
> On Sun, 18 Nov 2007 14:11:30 +0100
> Borislav Petkov <[email protected]> wrote:
>
> > On Thu, Nov 15, 2007 at 01:10:16PM -0800, Lucy McCoy wrote:
...
> > yes, after testing this i can confirm that this one fixes the NULL ptr
> > problem here so you might want to submit a proper patch to Greg.
>
> I'll merge revert-keyspan-init-termios-properly.patch soon, but afaik we
> are still awaiting the real fix for this problem?
Hi Andrew,
sorry for the late reply - i was away from the country and couldn't read mail.
Yes, we are still awaiting the real fix afaik but the code fragment above
removes the NULL ptr deref so we should at least merge that. Will prepare a
patch for this later today...
--
Regards/Gru?,
Boris.
Hi All,
I've been too busy to get to this but i'd rather not use the code
fragment i sent Boris to try. It would be better to go ahead with the
tty setup if the pointer is not NULL, otherwise use the defaults and not
reference the NULL pointer. I'll see if I can get to this today.
Lucy
Borislav Petkov wrote:
> On Mon, Nov 26, 2007 at 02:18:52PM -0800, Andrew Morton wrote:
>
>> On Sun, 18 Nov 2007 14:11:30 +0100
>> Borislav Petkov <[email protected]> wrote:
>>
>>
>>> On Thu, Nov 15, 2007 at 01:10:16PM -0800, Lucy McCoy wrote:
>>>
>
> ...
>
>
>>> yes, after testing this i can confirm that this one fixes the NULL ptr
>>> problem here so you might want to submit a proper patch to Greg.
>>>
>> I'll merge revert-keyspan-init-termios-properly.patch soon, but afaik we
>> are still awaiting the real fix for this problem?
>>
>
> Hi Andrew,
> sorry for the late reply - i was away from the country and couldn't read mail.
> Yes, we are still awaiting the real fix afaik but the code fragment above
> removes the NULL ptr deref so we should at least merge that. Will prepare a
> patch for this later today...
>
>
On Fri, Nov 30, 2007 at 09:23:43AM -0800, Lucy McCoy wrote:
> Hi All,
>
> I've been too busy to get to this but i'd rather not use the code fragment
> i sent Boris to try. It would be better to go ahead with the tty setup if
> the pointer is not NULL, otherwise use the defaults and not reference the
> NULL pointer. I'll see if I can get to this today.
you might also want to take a look at usa90_indat_callback() - there is
port->tty accessed again and this time it kills the machine completely when
something is sent over the serial line from the other end (through minicom, for
example). Oops is at
http://www.screenshots.cc/view_image/ab9a1837/cimg00772.jpg
--
Regards/Gru?,
Boris.
On Sun, 2 Dec 2007 09:03:40 +0100
Borislav Petkov <[email protected]> wrote:
> On Fri, Nov 30, 2007 at 09:23:43AM -0800, Lucy McCoy wrote:
> > Hi All,
> >
> > I've been too busy to get to this but i'd rather not use the code fragment
> > i sent Boris to try. It would be better to go ahead with the tty setup if
> > the pointer is not NULL, otherwise use the defaults and not reference the
> > NULL pointer. I'll see if I can get to this today.
> you might also want to take a look at usa90_indat_callback() - there is
> port->tty accessed again and this time it kills the machine completely when
> something is sent over the serial line from the other end (through minicom, for
> example). Oops is at
>
> http://www.screenshots.cc/view_image/ab9a1837/cimg00772.jpg
Change it to if (tty && urb->actual_length) in that function and it'll
probably cure it
On Sun, Dec 02, 2007 at 01:57:34PM +0000, Alan Cox wrote:
> On Sun, 2 Dec 2007 09:03:40 +0100
> Borislav Petkov <[email protected]> wrote:
>
> > On Fri, Nov 30, 2007 at 09:23:43AM -0800, Lucy McCoy wrote:
> > > Hi All,
> > >
> > > I've been too busy to get to this but i'd rather not use the code fragment
> > > i sent Boris to try. It would be better to go ahead with the tty setup if
> > > the pointer is not NULL, otherwise use the defaults and not reference the
> > > NULL pointer. I'll see if I can get to this today.
> > you might also want to take a look at usa90_indat_callback() - there is
> > port->tty accessed again and this time it kills the machine completely when
> > something is sent over the serial line from the other end (through minicom, for
> > example). Oops is at
> >
> > http://www.screenshots.cc/view_image/ab9a1837/cimg00772.jpg
>
> Change it to if (tty && urb->actual_length) in that function and it'll
> probably cure it
yep, this does the trick, however, this is just a brown paper bag over the issue, afaict.
Most of the usb serial drivers do something in the likes of
struct usb_serial_port *port = (struct usb_serial_port *) urb->context;
...
tty = port->tty;
...
tty_insert_flip_{char,string,..}(tty,..)
in their _indat_callback()s.
In the keyspan case the port->tty is being accessed already in the ->open() function
from usb_console_setup(), before setting the termios settings and later again in the
_indat callback. I'm clearly missing the big picture here so what is the proper way
to initialize termios settings correctly in a driver?
--
Regards/Gru?,
Boris.
