Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[Bodo Eggert]

Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> On Wednesday 16 January 2008 11:44:37 am H. Peter Anvin wrote:
>> Bjorn Helgaas wrote:

>> > +#ifdef CONFIG_X86
>> > +  switch (port->iobase) {
>> > +  case 0x3f8:     return 0;       /* COM1 -> ttyS0 */
>> > +  case 0x2f8:     return 1;       /* COM2 -> ttyS1 */
>> > +  case 0x3e8:     return 2;       /* COM3 -> ttyS2 */
>> > +  case 0x2e8:     return 3;       /* COM4 -> ttyS3 */
>> > +  }
>> > +#endif
>> > +
>> 
>> Arguably, the right thing is to use the addresses present in the array
>> at address 0x400.  In particular, COM3 and COM4 aren't always at those
>> addresses.
> 
> Wow.  I bow before your storehouse of x86 arcana :-)
> 
> I guess you're referring to the "BIOS data area," which I'd never
> heard of before (but fortunately, Google knows).

You'll want to google for Ralph Brown ...
if your back allowes bowing that much.

> What would you think about doing this only for COM1 and COM2?  The
> only real value for doing this in the first place is so "console=ttyS0"
> always goes to COM1, even if we don't have SERIAL_PORT_DFNS.  User-
> space ought to use some sort of udev magic if it cares about persistent
> naming.

Since the first four COM ports are magic, and since using the BIOS port
numbers will move the non-legacy ports anyway, you should use all up to
four stored port numbers.

BTW1: These addresses may be used to detect ports on non-standard addresses,
but unfortunately they don't tell the IRQ.

BTW2: When I submitted a patch using the BIOS data area, I was told that it
might not exist on systems booting from non-PC firmware. This claim was not
yet backed with any knowledge, nor did anybody suggest a way to detect this
situation.

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[H. Peter Anvin]

Bodo Eggert wrote:
> 
> BTW1: These addresses may be used to detect ports on non-standard addresses,
> but unfortunately they don't tell the IRQ.
> 
> BTW2: When I submitted a patch using the BIOS data area, I was told that it
> might not exist on systems booting from non-PC firmware. This claim was not
> yet backed with any knowledge, nor did anybody suggest a way to detect this
> situation.
> 

This is, of course, true.  It doesn't exactly help that some (most?) 
non-PC firmware at least mimic the BIOS data area.

In this particular case, there is some minor sanity-checking that can be 
done: the values should be nonzero and aligned 8.

	-hpa

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[Rene Herman]

On 16-01-08 21:42, H. Peter Anvin wrote:

> Bodo Eggert wrote:
>> 
>> BTW1: These addresses may be used to detect ports on non-standard 
>> addresses, but unfortunately they don't tell the IRQ.
>> 
>> BTW2: When I submitted a patch using the BIOS data area, I was told 
>> that it might not exist on systems booting from non-PC firmware. This 
>> claim was not yet backed with any knowledge, nor did anybody suggest a 
>> way to detect this situation.
> 
> This is, of course, true.  It doesn't exactly help that some (most?) 
> non-PC firmware at least mimic the BIOS data area.
> 
> In this particular case, there is some minor sanity-checking that can be
> done: the values should be nonzero and aligned 8.

The number of places expected to contain something sensible should I believe 
first be verified at 0x410 -- the equipment word. Bits 11-9 (0x0e00) should 
be the number of serial ports, 0 to 4 (so 5-7 is also a sanity check) and if 
BIOSes can be expected to zero out the non-used base-addresses (at 0x400, 
0x402, 0x404, 0x406) that's another sanity check. Don't know if they can 
though...

Rene.

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[H. Peter Anvin]

Rene Herman wrote:
> 
> The number of places expected to contain something sensible should I 
> believe first be verified at 0x410 -- the equipment word. Bits 11-9 
> (0x0e00) should be the number of serial ports, 0 to 4 (so 5-7 is also a 
> sanity check) and if BIOSes can be expected to zero out the non-used 
> base-addresses (at 0x400, 0x402, 0x404, 0x406) that's another sanity 
> check. Don't know if they can though...
> 

Probably not.  However, the ports marked valid should be nonzero and 
aligned-8.

	-hpa

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[H. Peter Anvin]

Bjorn Helgaas wrote:
>>> +
>> Arguably, the right thing is to use the addresses present in the array 
>> at address 0x400.  In particular, COM3 and COM4 aren't always at those 
>> addresses.
> 
> Wow.  I bow before your storehouse of x86 arcana :-)
> 
> I guess you're referring to the "BIOS data area," which I'd never
> heard of before (but fortunately, Google knows).
> 
> What would you think about doing this only for COM1 and COM2?  The
> only real value for doing this in the first place is so "console=ttyS0"
> always goes to COM1, even if we don't have SERIAL_PORT_DFNS.  User-
> space ought to use some sort of udev magic if it cares about persistent
> naming.
> 

Well, there are four ports that the BIOS have slots for, and if so, we 
probably should use them.

	-hpa

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[Bjorn Helgaas]

On Wednesday 16 January 2008 11:44:37 am H. Peter Anvin wrote:
> Bjorn Helgaas wrote:
> > x86 users expect COM1-COM4 ports at the conventional ioport addresses
> > to be named ttyS0-ttyS3.  For PNP devices, the BIOS determines the
> > order we discover them, so we might discover COM2 before COM1.
> > 
> > We currently always get the correct names, even without this patch.
> > But that's only because serial8250_isa_init_ports() first registers
> > the hard-coded list of COM port addresses from SERIAL_PORT_DFNS.  When
> > PNP rediscovers one of those ports, it gets the already-established
> > line.
> > 
> > This patch removes the implicit dependency on SERIAL_PORT_DFNS by
> > requesting the names we desire.
> >  
> >  static int __devinit
> > +serial_pnp_line(struct uart_port *port)
> > +{
> > +#ifdef CONFIG_X86
> > +	switch (port->iobase) {
> > +	case 0x3f8:	return 0;	/* COM1 -> ttyS0 */
> > +	case 0x2f8:	return 1;	/* COM2 -> ttyS1 */
> > +	case 0x3e8:	return 2;	/* COM3 -> ttyS2 */
> > +	case 0x2e8:	return 3;	/* COM4 -> ttyS3 */
> > +	}
> > +#endif
> > +
> 
> Arguably, the right thing is to use the addresses present in the array 
> at address 0x400.  In particular, COM3 and COM4 aren't always at those 
> addresses.

Wow.  I bow before your storehouse of x86 arcana :-)

I guess you're referring to the "BIOS data area," which I'd never
heard of before (but fortunately, Google knows).

What would you think about doing this only for COM1 and COM2?  The
only real value for doing this in the first place is so "console=ttyS0"
always goes to COM1, even if we don't have SERIAL_PORT_DFNS.  User-
space ought to use some sort of udev magic if it cares about persistent
naming.

Bjorn

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[H. Peter Anvin]

Bjorn Helgaas wrote:
> x86 users expect COM1-COM4 ports at the conventional ioport addresses
> to be named ttyS0-ttyS3.  For PNP devices, the BIOS determines the
> order we discover them, so we might discover COM2 before COM1.
> 
> We currently always get the correct names, even without this patch.
> But that's only because serial8250_isa_init_ports() first registers
> the hard-coded list of COM port addresses from SERIAL_PORT_DFNS.  When
> PNP rediscovers one of those ports, it gets the already-established
> line.
> 
> This patch removes the implicit dependency on SERIAL_PORT_DFNS by
> requesting the names we desire.
>  
>  static int __devinit
> +serial_pnp_line(struct uart_port *port)
> +{
> +#ifdef CONFIG_X86
> +	switch (port->iobase) {
> +	case 0x3f8:	return 0;	/* COM1 -> ttyS0 */
> +	case 0x2f8:	return 1;	/* COM2 -> ttyS1 */
> +	case 0x3e8:	return 2;	/* COM3 -> ttyS2 */
> +	case 0x2e8:	return 3;	/* COM4 -> ttyS3 */
> +	}
> +#endif
> +

Arguably, the right thing is to use the addresses present in the array 
at address 0x400.  In particular, COM3 and COM4 aren't always at those 
addresses.

	-hpa

Re: [patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[Russell King]

On Wed, Jan 16, 2008 at 10:05:43AM -0700, Bjorn Helgaas wrote:
>  static int __devinit
> +serial_pnp_line(struct uart_port *port)
> +{
> +#ifdef CONFIG_X86
> +	switch (port->iobase) {
> +	case 0x3f8:	return 0;	/* COM1 -> ttyS0 */
> +	case 0x2f8:	return 1;	/* COM2 -> ttyS1 */
> +	case 0x3e8:	return 2;	/* COM3 -> ttyS2 */
> +	case 0x2e8:	return 3;	/* COM4 -> ttyS3 */
> +	}
> +#endif

So what if someone intentionally modifies SERIAL_DEFN_PORTS to point
ttyS[0-3] somewhere else?  They also have to modify this code as well.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

[patch 2/2] 8250_pnp: register x86 COM ports at the conventional ttyS names

[Bjorn Helgaas]

[-- Attachment #1: 8250-pnp-names --]
[-- Type: text/plain, Size: 1808 bytes --]

x86 users expect COM1-COM4 ports at the conventional ioport addresses
to be named ttyS0-ttyS3.  For PNP devices, the BIOS determines the
order we discover them, so we might discover COM2 before COM1.

We currently always get the correct names, even without this patch.
But that's only because serial8250_isa_init_ports() first registers
the hard-coded list of COM port addresses from SERIAL_PORT_DFNS.  When
PNP rediscovers one of those ports, it gets the already-established
line.

This patch removes the implicit dependency on SERIAL_PORT_DFNS by
requesting the names we desire.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work5/drivers/serial/8250_pnp.c
===================================================================
--- work5.orig/drivers/serial/8250_pnp.c	2008-01-16 09:29:33.000000000 -0700
+++ work5/drivers/serial/8250_pnp.c	2008-01-16 09:51:09.000000000 -0700
@@ -427,6 +427,21 @@
 }
 
 static int __devinit
+serial_pnp_line(struct uart_port *port)
+{
+#ifdef CONFIG_X86
+	switch (port->iobase) {
+	case 0x3f8:	return 0;	/* COM1 -> ttyS0 */
+	case 0x2f8:	return 1;	/* COM2 -> ttyS1 */
+	case 0x3e8:	return 2;	/* COM3 -> ttyS2 */
+	case 0x2e8:	return 3;	/* COM4 -> ttyS3 */
+	}
+#endif
+
+	return -ENODEV;
+}
+
+static int __devinit
 serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 {
 	struct uart_port port;
@@ -462,7 +477,17 @@
 	port.uartclk = 1843200;
 	port.dev = &dev->dev;
 
-	line = serial8250_register_port(&port);
+	line = serial_pnp_line(&port);
+	if (line >= 0)
+		line = serial8250_register_port_at(&port, line);
+
+	/*
+	 * If the desired line was busy, or if we don't care which line
+	 * we get, use the regular registration.
+	 */
+	if (line < 0)
+		line = serial8250_register_port(&port);
+
 	if (line < 0)
 		return -ENODEV;
 

--