drivers/char/serial.c bug in ST16C654 detection
PATCH 1/1 drivers/char/serial.c bug in ST16C654 detection
This fixes a bug in the autoconfig_startech_uarts function in serial.c. The problem is that 0's are written to the baud rate registers in order to detect an XR16C850 or XR16C854. This makes the Exar ST16C654 go kablooey. Saving and restoring the baud rate registers after the test fixes it.
I'm assuming that the XR16C85[04] detection works as is and doesn't need the original baud rate restored. If I'm wrong, I'll rewrite the patch.
-VAL
drivers/char/serial.c +17 -2
@@ -3507,7 +3507,7 @@ struct serial_state *state, unsigned long flags) { -unsigned char scratch, scratch2, scratch3; +unsigned char scratch, scratch2, scratch3, scratch4;/* * First we check to see if it's an Oxford Semiconductor UART.
@@ -3551,17 +3551,32 @@* XR16C854. * */ + +/* Save the DLL and DLM */ +serial_outp (info,UART_LCR ,UART_LCR_DLAB ); + scratch3 =serial_inp (info,UART_DLL ); + scratch4 =serial_inp (info,UART_DLM ); +serial_outp (info,UART_DLL ,0 );serial_outp (info,UART_DLM ,0 ); - state->revision =serial_inp (info,UART_DLL ); + scratch2 =serial_inp (info,UART_DLL ); scratch =serial_inp (info,UART_DLM );serial_outp (info,UART_LCR ,0 ); +if (scratch ==0x10 || scratch ==0x14 ) { +if (scratch ==0x10 ) + state->revision = scratch2; state->type =PORT_16850 ;return ; } +/* Restore the DLL and DLM */ + +serial_outp (info,UART_LCR ,UART_LCR_DLAB ); +serial_outp (info,UART_DLL , scratch3); +serial_outp (info,UART_DLM , scratch4); +serial_outp (info,UART_LCR ,0 );/* * We distinguish between the '654 and the '650 by counting * how many bytes are in the FIFO. I'm using this for now,
From: "Val Henson" <[email protected]>
Quoted 5 lines
This fixes a bug in the
autoconfig_startech_uartsfunction inserial.c. The problem is that 0's are written to the baud rate registers in order to detect an XR16C850 or XR16C854. This makes the Exar ST16C654 go kablooey. Saving and restoring the baud rate registers after the test fixes it.
What version of serial.c? I'm assuming 5.05.
Define "go kablooey". We haven't noticed any problems, and we supplied this bit of code.
The size_fifo() routine supplies its own baud rate divisor, and on any rs_open() change_speed() sets the baud rate properly. I can't figure what you might be seeing.
Quoted 3 lines
I'm assuming that the XR16C85[04] detection works as is and doesn't need the original baud rate restored. If I'm wrong, I'll rewrite the patch.
You're correct that the detection works as is, except that you broke it in your patch. See below.
Quoted 16 lines
drivers/char/serial.c Sat May 12 05:19:26 2001 renamed from drivers/char/serial.c Thu Apr 19 00:26:34 2001 +2
-1@@ -3507,7 +3507,7 @@ struct serial_state *state, unsigned long flags) { - unsigned char scratch, scratch2, scratch3; + unsigned char scratch, scratch2, scratch3, scratch4; /* * First we check to see if it's an Oxford Semiconductor UART.@@ -3551,17 +3551,32 @@ * XR16C854. * */ +
drivers/char/serial.c Sat May 12 05:19:26 2001 line 3563
Quoted 10 lines
drivers/char/serial.c Sat May 12 05:19:26 2001 renamed from drivers/char/serial.c Thu Apr 19 00:26:34 2001 +6
-1+ /* Save the DLL and DLM */ + serial_outp(info, UART_LCR, UART_LCR_DLAB); + scratch3 = serial_inp(info, UART_DLL); + scratch4 = serial_inp(info, UART_DLM); + serial_outp(info, UART_DLL, 0); serial_outp(info, UART_DLM, 0); - state->revision = serial_inp(info, UART_DLL); + scratch2 = serial_inp(info, UART_DLL);
This isn't necessary. The revision field is only checked for 950s, so setting it here doesn't harm anything. If the current (only) example of checking it is followed as normal procedure, the port type will always be checked first, before checking the revision, ensuring only valid revisions are referenced.
drivers/char/serial.c Sat May 12 05:19:26 2001 line 3572
Quoted 9 lines
drivers/char/serial.c Sat May 12 05:19:26 2001 renamed from drivers/char/serial.c Thu Apr 19 00:26:34 2001 +3
-0scratch = serial_inp(info, UART_DLM); serial_outp(info, UART_LCR, 0); + if (scratch == 0x10 || scratch == 0x14) { + if (scratch == 0x10) + state->revision = scratch2; state->type = PORT_16850; return; }
Only saving the revision for 850s is probably wrong. It should be saved for all the 85x uarts.
drivers/char/serial.c Sat May 12 05:19:26 2001 line 3581
Quoted 9 lines
drivers/char/serial.c Sat May 12 05:19:26 2001 renamed from drivers/char/serial.c Thu Apr 19 00:26:34 2001 +6
-0+ /* Restore the DLL and DLM */ + + serial_outp(info, UART_LCR, UART_LCR_DLAB); + serial_outp(info, UART_DLL, scratch3); + serial_outp(info, UART_DLM, scratch4); + serial_outp(info, UART_LCR, 0); /* * We distinguish between the '654 and the '650 by counting * how many bytes are in the FIFO. I'm using this for now,
..Stu
Quoted 1 line · Stuart MacDonald
What version of
serial.c? I'm assuming 5.05.
Serial driver version 5.05a (2001-03-20) with MANY_PORTS SHARE_IRQ SERIAL_PCI enabled
Quoted 2 lines
Define "go kablooey". We haven't noticed any problems, and we supplied this bit of code.
"Go kablooey" means that all serial output stops and the kernel never finishes booting. This patch makes it correctly detect the controller, continue to produce serial output, and finish booting.
Quoted 3 lines
The
size_fifo()routine supplies its own baud rate divisor, and on anyrs_open()change_speed()sets the baud rate properly. I can't figure what you might be seeing.
I don't know why it doesn't work the way you describe. If I comment out the section of code that sets the baud rate to 0, everything works fine. Otherwise, it doesn't even finish booting. The Exar datasheet at http://www.exar.com/products/st16c654.pdf doesn't define what happens if you set the baud rate registers to 0.
Quoted 6 lines
This isn't necessary. The revision field is only checked for 950s, so setting it here doesn't harm anything. If the current (only) example of checking it is followed as normal procedure, the port type will always be checked first, before checking the revision, ensuring only valid revisions are referenced.
It's just sloppy programming. There's no benefit from setting an invalid revision for the Exar and if the usage of state->revision changes it may introduce a bug.
Quoted 2 lines
Only saving the revision for 850s is probably wrong. It should be saved for all the 85x uarts.
The comment above it should also be changed then. If someone knows for sure whether the revision should be saved for the XR16C854 then the comment can be made clearer. See patch below.
-VAL
drivers/char/serial.c +17 -4
@@ -3507,7 +3507,7 @@ struct serial_state *state, unsigned long flags) { -unsigned char scratch, scratch2, scratch3; +unsigned char scratch, scratch2, scratch3, scratch4;/* * First we check to see if it's an Oxford Semiconductor UART.
@@ -3548,20 +3548,33 @@* then reading back DLL and DLM. If DLM reads back 0x10, * then the UART is a XR16C850 and the DLL contains the chip * revision. If DLM reads back 0x14, then the UART is a -* XR16C854. -* +* XR16C854 and the DLL may or may not contain the revision. */ + +/* Save the DLL and DLM */ +serial_outp (info,UART_LCR ,UART_LCR_DLAB ); + scratch3 =serial_inp (info,UART_DLL ); + scratch4 =serial_inp (info,UART_DLM ); +serial_outp (info,UART_DLL ,0 );serial_outp (info,UART_DLM ,0 ); - state->revision =serial_inp (info,UART_DLL ); + scratch2 =serial_inp (info,UART_DLL ); scratch =serial_inp (info,UART_DLM );serial_outp (info,UART_LCR ,0 ); +if (scratch ==0x10 || scratch ==0x14 ) { + state->revision = scratch2; state->type =PORT_16850 ;return ; } +/* Restore the DLL and DLM */ + +serial_outp (info,UART_LCR ,UART_LCR_DLAB ); +serial_outp (info,UART_DLL , scratch3); +serial_outp (info,UART_DLM , scratch4); +serial_outp (info,UART_LCR ,0 );/* * We distinguish between the '654 and the '650 by counting * how many bytes are in the FIFO. I'm using this for now,
From: "Val Henson" <[email protected]>
Quoted 2 lines
Serial driver version 5.05a (2001-03-20) with
MANY_PORTSSHARE_IRQSERIAL_PCIenabled
Hmm. I've been looking at 5.05 (from http://serial.sourceforge.net),
I'm getting 2.4.4 and 2.4.5-pre2 to see what's in there.
Quoted 3 lines
"Go kablooey" means that all serial output stops and the kernel never finishes booting. This patch makes it correctly detect the controller, continue to produce serial output, and finish booting.
Kernel version? Distribution? Are you using a serial console?
Quoted 5 lines
I don't know why it doesn't work the way you describe. If I comment out the section of code that sets the baud rate to 0, everything works fine. Otherwise, it doesn't even finish booting. The Exar datasheet at http://www.exar.com/products/st16c654.pdf doesn't define what happens if you set the baud rate registers to 0.
Yeah, I double checked the 654 and 864 sheets yesterday. No mention, which is sorta odd for the 864, since they explicitly say to write the 0s to get the revision. I'd have to assume that it's bad. This bit I copied from one of our hardware guys, and it worked, so I never looked too closely at it. My apologies.
Quoted 3 lines
It's just sloppy programming. There's no benefit from setting an invalid revision for the Exar and if the usage of
state->revisionchanges it may introduce a bug.
I agree with you. However, the revision is Ted's code, and I've generally found it easier to get our patches in to the driver if I do what he's already doing. However, I agree with you.
Quoted 3 lines
The comment above it should also be changed then. If someone knows for sure whether the revision should be saved for the XR16C854 then the comment can be made clearer. See patch below.
The revision should always be saved if it's available. Hmm. I didn't look carefully yesterday. The DLL always contains the revision for the 85x family. Why do you think it doesn't?
I think your patch below is good, I'm just mystified by this kablooey thing.
..Stu
drivers/char/serial.c Tue May 15 03:19:08 2001 renamed from drivers/char/serial.c Thu Apr 19 00:26:34 2001 +17 -4
@@ -3507,7 +3507,7 @@ struct serial_state *state, unsigned long flags) { - unsigned char scratch, scratch2, scratch3; + unsigned char scratch, scratch2, scratch3, scratch4; /* * First we check to see if it's an Oxford Semiconductor UART.
@@ -3548,20 +3548,33 @@ * then reading back DLL and DLM. If DLM reads back 0x10, * then the UART is a XR16C850 and the DLL contains the chip * revision. If DLM reads back 0x14, then the UART is a - * XR16C854. - * + * XR16C854 and the DLL may or may not contain the revision. */ + + /* Save the DLL and DLM */ + serial_outp(info, UART_LCR, UART_LCR_DLAB); + scratch3 = serial_inp(info, UART_DLL); + scratch4 = serial_inp(info, UART_DLM); + serial_outp(info, UART_DLL, 0); serial_outp(info, UART_DLM, 0); - state->revision = serial_inp(info, UART_DLL); + scratch2 = serial_inp(info, UART_DLL); scratch = serial_inp(info, UART_DLM); serial_outp(info, UART_LCR, 0); + if (scratch == 0x10 || scratch == 0x14) { + state->revision = scratch2; state->type = PORT_16850; return; } + /* Restore the DLL and DLM */ + + serial_outp(info, UART_LCR, UART_LCR_DLAB); + serial_outp(info, UART_DLL, scratch3); + serial_outp(info, UART_DLM, scratch4); + serial_outp(info, UART_LCR, 0); /* * We distinguish between the '654 and the '650 by counting * how many bytes are in the FIFO. I'm using this for now,
Anyone know where Ted Tso is? He hasn't posted in several weeks. Alan, will you put this in -ac? This patch fixes a bug in serial.c that causes a crash on machines with a ST16C654.
Quoted 1 line · Stuart MacDonald
Kernel version? Distribution? Are you using a serial console?
2.4.5-pre1 (see patch).
Quoted 3 lines
The revision should always be saved if it's available. Hmm. I didn't look carefully yesterday. The DLL always contains the revision for the 85x family. Why do you think it doesn't?
Because the comment was ambiguous. I don't have the data sheet for the 85x family so I just wrote the code according to the comment. I'll change the comment to make it clear.
Quoted 2 lines
I think your patch below is good, I'm just mystified by this kablooey thing.
Thanks. Corrected version of the patch is below.
-VAL
drivers/char/serial.c +17 -4
@@ -3507,7 +3507,7 @@ struct serial_state *state, unsigned long flags) { -unsigned char scratch, scratch2, scratch3; +unsigned char scratch, scratch2, scratch3, scratch4;/* * First we check to see if it's an Oxford Semiconductor UART.
@@ -3548,20 +3548,33 @@* then reading back DLL and DLM. If DLM reads back 0x10, * then the UART is a XR16C850 and the DLL contains the chip * revision. If DLM reads back 0x14, then the UART is a -* XR16C854. -* +* XR16C854 and the DLL contains the chip revision. */ + +/* Save the DLL and DLM */ +serial_outp (info,UART_LCR ,UART_LCR_DLAB ); + scratch3 =serial_inp (info,UART_DLL ); + scratch4 =serial_inp (info,UART_DLM ); +serial_outp (info,UART_DLL ,0 );serial_outp (info,UART_DLM ,0 ); - state->revision =serial_inp (info,UART_DLL ); + scratch2 =serial_inp (info,UART_DLL ); scratch =serial_inp (info,UART_DLM );serial_outp (info,UART_LCR ,0 ); +if (scratch ==0x10 || scratch ==0x14 ) { + state->revision = scratch2; state->type =PORT_16850 ;return ; } +/* Restore the DLL and DLM */ + +serial_outp (info,UART_LCR ,UART_LCR_DLAB ); +serial_outp (info,UART_DLL , scratch3); +serial_outp (info,UART_DLM , scratch4); +serial_outp (info,UART_LCR ,0 );/* * We distinguish between the '654 and the '650 by counting * how many bytes are in the FIFO. I'm using this for now,
From: "Val Henson" <[email protected]>
Quoted 1 line
Anyone know where Ted Tso is? He hasn't posted in several weeks.
Haven't heard from him in a while. I've got a patch or two pending as well, one of which modifies this code to check for 16c2850s. Usually he just says he's really busy.
Quoted 2 lines
Quoted 1 line
Kernel version? Distribution? Are you using a serial console?
2.4.5-pre1 (see patch).
Are you using the serial console though? That seems to be implied by your problem, but I just want to check.
Quoted 3 lines
Because the comment was ambiguous. I don't have the data sheet for the 85x family so I just wrote the code according to the comment. I'll change the comment to make it clear.
Hm. I didn't really read the comment, since I know what the code is doing. :-) I can send you an 864 sheet if you'd like; we most likely have the other 85x sheets around somewhere; all pdf format.
..Stu
Quoted 2 lines · Stuart MacDonald
Are you using the serial console though? That seems to be implied by your problem, but I just want to check.
Yes, I have serial console only on this board.
-VAL
Quoted 3 lines · Val Henson
Anyone know where Ted Tso is? He hasn't posted in several weeks. Alan, will you put this in -ac? This patch fixes a bug in
serial.cthat causes a crash on machines with a ST16C654.
I'm around, just swamped with a few other things. Dealing with serial driver issues is on the top of my to do list....
- Ted