[PATCH] serial.c procfs kudzu - discussion

[PATCH] serial.c procfs kudzu - discussion

[Ed Vance]

Patch submitted for discussion:

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:2F8 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: Line 78: 237 Segmentation fault

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

This patch adds logic to check both the I/O port field and the I/O memory 
base address field to detect the presence of hardware. The patched driver 
generates the longer format text for memory mapped ports:

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

Created on kernel files rev 2.4.19-pre2

Contributor: Ed Vance

ISSUES AND CONCERNS FOR DISCUSSION:

1. The patch depends on the I/O port address field and the I/O memory 
address field both being zero for unconfigured ports. Anybody know of any 
exception corner cases where a port gets partially configured and gives 
up with one of these fields left non-zero? 

2. The label on the displayed I/O memory address is "port:", just like 
an I/O port address. Should it display "iomem:" instead of "port:"?

3. A trailing space has been added to all text lines as a cushion for 
kudzu. Should this stay in?

4. Should a bug be turned in against kudzu for the weak parser? 


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	Wed Mar  6 14:31:44 2002
@@ -3260,10 +3260,12 @@
 
 	ret = sprintf(buf, "%d: uart:%s port:%lX irq:%d",
 		      state->line, uart_config[state->type].name, 
-		      state->port, state->irq);
+		      (state->port ? state->port : (long)state->iomem_base),
+		      state->irq);
 
-	if (!state->port || (state->type == PORT_UNKNOWN)) {
-		ret += sprintf(buf+ret, "\n");
+	if ((!state->port && !state->iomem_base) ||
+	    (state->type == PORT_UNKNOWN)) {
+		ret += sprintf(buf+ret, " \n");
 		return ret;
 	}
 

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

Re: [PATCH] serial.c procfs kudzu - discussion

[Bill Nottingham]

Ed Vance (EdV@macrolink.com) said: 
> 4. Should a bug be turned in against kudzu for the weak parser? 

Absolutely. When did the serial change go in?

Bill

RE: [PATCH] serial.c procfs kudzu - discussion

[Ed Vance]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Bill Nottingham wrote:
> Ed Vance (EdV@macrolink.com) said: 
> > 4. Should a bug be turned in against kudzu for the weak parser? 
> 
>Absolutely. When did the serial change go in?

Hi Bill,

This is not the result of a recent change to the serial driver. I don't 
know how far back this bug goes, but I suspect it is as old as the proc 
fs serial support. 

The recent event was me adding memory mapped support for a couple of 
CompactPCI serial mux cards. Apparently everyone else on the planet 
uses I/O space (or just did not report it) and the driver/kudzu 
combination works fine with serial cards in I/O space. 

Do you want me to go ahead and put it into bugzilla? 

BTW, I have attached the procfs data produced with and without the 
driver bug.

Best regards,
Ed Vance

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



[-- Attachment #2: bad_proc_serial --]
[-- Type: application/octet-stream, Size: 2151 bytes --]

serinfo:1.0 driver:5.05c revision:2001-07-08
0: uart:16550A port:3F8 irq:4 tx:0 rx:0 RTS|DTR
1: uart:16550A port:2F8 irq:3 tx:0 rx:0 
2: uart:unknown port:3E8 irq:4
3: uart:unknown port:2E8 irq:3
4: uart:16C950/954 port:0 irq:14
5: uart:16C950/954 port:0 irq:14
6: uart:16C950/954 port:0 irq:14
7: uart:16C950/954 port:0 irq:14
8: uart:16C950/954 port:0 irq:14
9: uart:16C950/954 port:0 irq:14
10: uart:16C950/954 port:0 irq:14
11: uart:16C950/954 port:0 irq:14
12: uart:16C950/954 port:0 irq:14
13: uart:16C950/954 port:0 irq:14
14: uart:16C950/954 port:0 irq:14
15: uart:16C950/954 port:0 irq:14
16: uart:16C950/954 port:0 irq:14
17: uart:16C950/954 port:0 irq:14
18: uart:16C950/954 port:0 irq:14
19: uart:16C950/954 port:0 irq:14
20: uart:ST16654 port:0 irq:10
21: uart:ST16654 port:0 irq:10
22: uart:ST16654 port:0 irq:10
23: uart:ST16654 port:0 irq:10
24: uart:ST16654 port:0 irq:10
25: uart:ST16654 port:0 irq:10
26: uart:ST16654 port:0 irq:10
27: uart:ST16654 port:0 irq:10
28: uart:ST16654 port:0 irq:10
29: uart:ST16654 port:0 irq:10
30: uart:ST16654 port:0 irq:10
31: uart:ST16654 port:0 irq:10
32: uart:ST16654 port:0 irq:10
33: uart:ST16654 port:0 irq:10
34: uart:ST16654 port:0 irq:10
35: uart:ST16654 port:0 irq:10
36: uart:unknown port:302 irq:3
37: uart:unknown port:302 irq:3
38: uart:unknown port:302 irq:3
39: uart:unknown port:302 irq:3
40: uart:unknown port:302 irq:3
41: uart:unknown port:302 irq:3
42: uart:unknown port:302 irq:3
43: uart:unknown port:302 irq:3
44: uart:unknown port:0 irq:0
45: uart:unknown port:0 irq:0
46: uart:unknown port:0 irq:0
47: uart:unknown port:0 irq:0
48: uart:unknown port:0 irq:0
49: uart:unknown port:0 irq:0
50: uart:unknown port:0 irq:0
51: uart:unknown port:0 irq:0
52: uart:unknown port:0 irq:0
53: uart:unknown port:0 irq:0
54: uart:unknown port:0 irq:0
55: uart:unknown port:0 irq:0
56: uart:unknown port:0 irq:0
57: uart:unknown port:0 irq:0
58: uart:unknown port:0 irq:0
59: uart:unknown port:0 irq:0
60: uart:unknown port:0 irq:0
61: uart:unknown port:0 irq:0
62: uart:unknown port:0 irq:0
63: uart:unknown port:0 irq:0

[-- Attachment #3: good_proc_serial --]
[-- Type: application/octet-stream, Size: 2770 bytes --]

serinfo:1.0 driver:5.05c-mlA3 revision:2001-11-27
0: uart:16550A port:3F8 irq:4 baud:9600 tx:11 rx:0 
1: uart:16550A port:2F8 irq:3 baud:9600 tx:11 rx:26 
2: uart:unknown port:3E8 irq:4 
3: uart:unknown port:2E8 irq:3 
4: uart:16C950/954 port:C4800000 irq:14 tx:0 rx:0 
5: uart:16C950/954 port:C4802020 irq:14 tx:0 rx:0 
6: uart:16C950/954 port:C4804040 irq:14 tx:0 rx:0 
7: uart:16C950/954 port:C4806060 irq:14 tx:0 rx:0 
8: uart:16C950/954 port:C4808000 irq:14 tx:0 rx:0 
9: uart:16C950/954 port:C480A020 irq:14 tx:0 rx:0 
10: uart:16C950/954 port:C480C040 irq:14 tx:0 rx:0 
11: uart:16C950/954 port:C480E060 irq:14 tx:0 rx:0 
12: uart:16C950/954 port:C4810400 irq:14 tx:0 rx:0 
13: uart:16C950/954 port:C4812420 irq:14 tx:0 rx:0 
14: uart:16C950/954 port:C4814440 irq:14 tx:0 rx:0 
15: uart:16C950/954 port:C4816460 irq:14 tx:0 rx:0 
16: uart:16C950/954 port:C4818800 irq:14 tx:0 rx:0 
17: uart:16C950/954 port:C481A820 irq:14 tx:0 rx:0 
18: uart:16C950/954 port:C481C840 irq:14 tx:0 rx:0 
19: uart:16C950/954 port:C481E860 irq:14 tx:0 rx:0 
20: uart:ST16654 port:C4820000 irq:10 tx:0 rx:0 
21: uart:ST16654 port:C4822008 irq:10 tx:0 rx:0 
22: uart:ST16654 port:C4824010 irq:10 tx:0 rx:0 
23: uart:ST16654 port:C4826018 irq:10 tx:0 rx:0 
24: uart:ST16654 port:C4828020 irq:10 tx:0 rx:0 
25: uart:ST16654 port:C482A028 irq:10 tx:0 rx:0 
26: uart:ST16654 port:C482C030 irq:10 tx:0 rx:0 
27: uart:ST16654 port:C482E038 irq:10 tx:0 rx:0 
28: uart:ST16654 port:C4830040 irq:10 tx:0 rx:0 
29: uart:ST16654 port:C4832048 irq:10 tx:0 rx:0 
30: uart:ST16654 port:C4834050 irq:10 tx:0 rx:0 
31: uart:ST16654 port:C4836058 irq:10 tx:0 rx:0 
32: uart:ST16654 port:C4838060 irq:10 tx:0 rx:0 CD
33: uart:ST16654 port:C483A068 irq:10 tx:0 rx:0 CD
34: uart:ST16654 port:C483C070 irq:10 tx:0 rx:0 CD
35: uart:ST16654 port:C483E078 irq:10 tx:0 rx:0 CD
36: uart:unknown port:0 irq:0 
37: uart:unknown port:0 irq:0 
38: uart:unknown port:0 irq:0 
39: uart:unknown port:0 irq:0 
40: uart:unknown port:0 irq:0 
41: uart:unknown port:0 irq:0 
42: uart:unknown port:0 irq:0 
43: uart:unknown port:0 irq:0 
44: uart:unknown port:0 irq:0 
45: uart:unknown port:0 irq:0 
46: uart:unknown port:0 irq:0 
47: uart:unknown port:0 irq:0 
48: uart:unknown port:0 irq:0 
49: uart:unknown port:0 irq:0 
50: uart:unknown port:0 irq:0 
51: uart:unknown port:0 irq:0 
52: uart:unknown port:0 irq:0 
53: uart:unknown port:0 irq:0 
54: uart:unknown port:0 irq:0 
55: uart:unknown port:0 irq:0 
56: uart:unknown port:0 irq:0 
57: uart:unknown port:0 irq:0 
58: uart:unknown port:0 irq:0 
59: uart:unknown port:0 irq:0 
60: uart:unknown port:0 irq:0 
61: uart:unknown port:0 irq:0 
62: uart:unknown port:0 irq:0 
63: uart:unknown port:0 irq:0 

Re: [PATCH] serial.c procfs kudzu - discussion

[Russell King]

On Thu, Mar 07, 2002 at 03:12:54PM -0800, Ed Vance wrote:
> This is not the result of a recent change to the serial driver. I don't 
> know how far back this bug goes, but I suspect it is as old as the proc 
> fs serial support. 

I think there are two bugs here that need treating in different ways.

1. Not displaying port statistics for iomem-based ports.  This is
   probably an oversight when iomem ports were added to the serial
   driver.

2. "port:" entry being 0.  I don't think we really want to report IO
   port or memory addresses here without giving userspace some
   indication which we're reporting.

For 2, I'd suggest replacing "port:" with "mem:" for iomem ports, and
changing the serinfo: line to reflect the changed format (this is
probably ignored by kudzu though.)

Does this sound reasonable?

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

[David Woodhouse]

rmk@arm.linux.org.uk said:
>  I think there are two bugs here that need treating in different ways.
> 

Don't forget the fact that non-existent ports are visible, you can open 
their device nodes, etc. That's just screwed. 

--
dwmw2

RE: [PATCH] serial.c procfs kudzu - discussion

[Ed Vance]

On Fri Mar 08, 2002, Russell King wrote:
> 
> I think there are two bugs here that need treating in different ways.
> 
> 1. Not displaying port statistics for iomem-based ports.  This is
>    probably an oversight when iomem ports were added to the serial
>    driver.
> 
> 2. "port:" entry being 0.  I don't think we really want to report IO
>    port or memory addresses here without giving userspace some
>    indication which we're reporting.
> 
> For 2, I'd suggest replacing "port:" with "mem:" for iomem ports, and
> changing the serinfo: line to reflect the changed format (this is
> probably ignored by kudzu though.)
> 
> Does this sound reasonable?

Hi Russell,

Yes. I'll change the serinfo line rev marking from 1.0 to 1.1 and label 
the  iomem value as "mem". If I remember correctly, kudzu detects that 
field by its delimiters, so it does not matter that we change the field 
label. It's probably easiest for me to verify by just trying it. If there 
is a surprise there, I will inform Bill Nottingham at Red Hat. 

Also, I'll verify that the port statistics flow unit runs for the iomem 
case.

Thanks,
Ed Vance

RE: [PATCH] serial.c procfs kudzu - discussion

[Ed Vance]

On Fri Mar 08, 2002, David Woodhouse wrote:
> 
> Don't forget the fact that non-existent ports are visible, you 
> can open their device nodes, etc. That's just screwed. 

David,

Care to submit a patch or propose a specific method, for discussion?

Ed Vance

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

RE: [PATCH] serial.c procfs kudzu - discussion

[Ed Vance]

On Fri Mar 08, 2002, Ed Vance wrote:
> 
> On Fri Mar 08, 2002, Russell King wrote:
> > 
> > 2. "port:" entry being 0.  I don't think we really want to report IO
> >    port or memory addresses here without giving userspace some
> >    indication which we're reporting.
> > 
> > For 2, I'd suggest replacing "port:" with "mem:" for iomem ports, and
> > changing the serinfo: line to reflect the changed format (this is
> > probably ignored by kudzu though.)
> >
> Yes. I'll change the serinfo line rev marking from 1.0 to 1.1 and label 
> the  iomem value as "mem". If I remember correctly, kudzu detects that 
> field by its delimiters, so it does not matter that we change the field 
> label. It's probably easiest for me to verify by just trying it. If there 
> is a surprise there, I will inform Bill Nottingham at Red Hat. 

Bill,

I did not remember kudzu's parsing of the "port" field as well as I thought.

It does a string match against the first three labels, "uart", "port", and 
"irq". So, if I change the second label to "mem" for ports that are mapped 
into memory space, then it will break kudzu. In function InitSerials(), 
pointer variable "port" becomes null at pciserial.c:61 and causes strchr() 
to explode at line pciserial.c:71. (kudzu-0.99.23 from Red Hat 7.2) 

The only way we could differentiate I/O and memory addresses without 
breaking the current kudzu (that I could think of at my present caffeine 
level) would be to leave it "port" and output four hex digits for I/O 
addresses and eight digits for memory addresses. (just a bit ugly)

All,

Is this correction of information presented to the user from the driver 
worth changing kudzu? Opinions please. 

Best regards,
Ed Vance