[PATCH] serial.c procfs 2nd try - discussion

[PATCH] serial.c procfs 2nd try - discussion

[Ed Vance]

This serial driver patch modifies function line_info() which assembles text 
to be read from /proc/tty/driver/serial. For example, the text line for the 
COM2 port, which is mapped to I/O space, is as follows:

    "1: uart:16550A port:02F8 irq:3 tx:0 rx:0 "

The patch corrects handling of ports that are on memory mapped devices. 
The unpatched function generates a short text line for such ports because 
the I/O port address field is zero on memory mapped ports:

    "4: uart:16C950/954 port:0 irq:14"

This is the format used for nonexistent ports. Truly nonexistent ports 
always have a port type of "unknown port". kudzu depends on that. The bug 
causes the short format to be used for all memory mapped ports which have 
known types. The absence of the tx: and rx: fields in the short format 
causes kudzu to die during system initialization at the "Checking for new 
hardware" step:

    S05kudzu:   pciserial.c: Line 78: 237 Segmentation fault

This is caused by a null-pointer dereference in kudzu's not-so-robust 
parser, when it lemmings off the end of the invalid short line. 

This patch adds a test to avoid displaying ports that are marked as uart 
type "unknown" instead of displaying a line for all possible port numbers. 
This patch adds logic to check both the I/O port field and the I/O memory 
base address field to display the correct device address. The patched 
driver generates the longer format text for memory mapped ports:

    "4: uart:16C950/954 port:C4800000 irq:14 tx:0 rx:0 "

To avoid breaking kudzu, but still make it possible to see which ports are 
mapped to I/O space and which are mapped to memory, the label on the port 
address field is left as "port:". Memory mapped addresses are displayed 
as eight hex digits. I/O port mapped addresses are displayed as four hex 
digits. The "serinfo" version number on the first line is changed from 1.0 
to 1.1 because of these display differences. 

Created on kernel files rev 2.4.19-pre2

SUMMARY OF CHANGES: 
Changes text generated by serial driver for the /proc/tty/driver/serial 
interface. Serinfo version tag changes from 1.0 to 1.1. Now generates a 
correct display for memory mapped devices. Displays only ports claimed 
by driver. Displays I/O port addresses as 4 digits, and memory mapped 
addresses as 8 digits. New format prevents kudzu crash on null pointer 
de-reference when memory mapped devices are present. 

Contributor: Ed Vance [serial24@macrolink.com]

ISSUES AND CONCERNS FOR DISCUSSION:

1. Does anybody know of anything that will break because the nonexistent 
   ports are not displayed?

2. Does anybody know of anything that will break because of the leading 
   zeros that are now present on the address field? 


diff -urN -X dontdiff.txt linux-2.4.19-pre2/drivers/char/serial.c
patched/drivers/char/serial.c
--- linux-2.4.19-pre2/drivers/char/serial.c	Sat Mar  2 10:38:11 2002
+++ patched/drivers/char/serial.c	Mon Mar 11 16:22:42 2002
@@ -3258,15 +3258,19 @@
 	int	ret;
 	unsigned long flags;
 
-	ret = sprintf(buf, "%d: uart:%s port:%lX irq:%d",
-		      state->line, uart_config[state->type].name, 
-		      state->port, state->irq);
-
-	if (!state->port || (state->type == PORT_UNKNOWN)) {
-		ret += sprintf(buf+ret, "\n");
-		return ret;
+	/*
+	 * Return zero characters for ports not claimed by a driver.
+	 */
+	if (state->type == PORT_UNKNOWN) {
+		return 0;	/* ignore unused ports */
 	}
 
+	ret = sprintf(buf, state->port ? "%d: uart:%s port:%04lX irq:%d"
+				       : "%d: uart:%s port:%08lX irq:%d",
+		      state->line, uart_config[state->type].name, 
+		      (state->port ? state->port : (long)state->iomem_base),
+		      state->irq);
+
 	/*
 	 * Figure out the current RS-232 lines
 	 */
@@ -3336,7 +3340,7 @@
 	int i, len = 0, l;
 	off_t	begin = 0;
 
-	len += sprintf(page, "serinfo:1.0 driver:%s%s revision:%s\n",
+	len += sprintf(page, "serinfo:1.1 driver:%s%s revision:%s\n",
 		       serial_version, LOCAL_VERSTRING, serial_revdate);
 	for (i = 0; i < NR_PORTS && len < 4000; i++) {
 		l = line_info(page + len, &rs_table[i]);


---------------------------------------------------------------- 
Ed Vance              serial24@macrolink.com
Macrolink, Inc.       1500 N. Kellogg Dr  Anaheim, CA  92807
----------------------------------------------------------------

Re: [PATCH] serial.c procfs 2nd try - discussion

[Russell King]

On Mon, Mar 11, 2002 at 05:41:09PM -0800, Ed Vance wrote:
> 2. Does anybody know of anything that will break because of the leading 
>    zeros that are now present on the address field? 

I'm not overly happy with this idea - there isn't anything that says an
ioport address has 4 digits.  I know of machines where an ioport address
has 8, and I'm sure on the Alpha or Sparc64 its probably 16 digits.

It might be a better solution to leave the 'port:' element as-is if
programs like kudzu rely on that label there, and just fix the missing
statistics for iomem ports.

Then file a bug against kudzu and get them to fix that so it doesn't
SEGV when it finds something it doesn't like, and teach it about the
'mem' tag.

If kudzu ignores the serinfo: line as well, that's also another kudzu
bug.

Then fix the proc interface to report a 'mem' tag for each port.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] serial.c procfs 2nd try - discussion

[Jeff Garzik]

Russell King wrote:

>On Mon, Mar 11, 2002 at 05:41:09PM -0800, Ed Vance wrote:
>
>>2. Does anybody know of anything that will break because of the leading 
>>   zeros that are now present on the address field? 
>>
>
>I'm not overly happy with this idea - there isn't anything that says an
>ioport address has 4 digits.  I know of machines where an ioport address
>has 8, and I'm sure on the Alpha or Sparc64 its probably 16 digits.
>
Agreed.  Standard portability convention seems to say that one treats io 
ports and io mem both as unsigned long, including when printing...

    Jeff