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 [email protected]
Macrolink, Inc. 1500 N. Kellogg Dr Anaheim, CA 92807
----------------------------------------------------------------
Ed Vance ([email protected]) said:
> 4. Should a bug be turned in against kudzu for the weak parser?
Absolutely. When did the serial change go in?
Bill
Bill Nottingham wrote:
> Ed Vance ([email protected]) 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 [email protected]
Macrolink, Inc. 1500 N. Kellogg Dr Anaheim, CA 92807
----------------------------------------------------------------
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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
[email protected] 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
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
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 [email protected]
Macrolink, Inc. 1500 N. Kellogg Dr Anaheim, CA 92807
----------------------------------------------------------------
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