[patch 0/2] serial: explicitly request ttyS0-3 for COM1-4

[patch 0/2] serial: explicitly request ttyS0-3 for COM1-4

[Bjorn Helgaas]

When 8250_pnp discovers COM ports, we only get the correct ttyS names
by accident -- we rely on serial8250_isa_init_ports(), which discovers
the COM ports earlier using the addresses in SERIAL_PORT_DFNS.

These two patches remove the dependency on SERIAL_PORT_DFNS by
explicitly requesting the desired ttyS names.

-- 

[patch 1/2] 8250: add serial8250_register_port_at() for requesting specific ttyS lines

[Bjorn Helgaas]

[-- Attachment #1: 8250-register-at --]
[-- Type: text/plain, Size: 3742 bytes --]

Add an interface for registering a new UART with a specific ttyS name.

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

Index: work5/include/linux/serial_8250.h
===================================================================
--- work5.orig/include/linux/serial_8250.h	2008-01-15 16:31:49.000000000 -0700
+++ work5/include/linux/serial_8250.h	2008-01-15 16:32:42.000000000 -0700
@@ -56,6 +56,7 @@
 struct uart_port;
 
 int serial8250_register_port(struct uart_port *);
+int serial8250_register_port_at(struct uart_port *, int line);
 void serial8250_unregister_port(int line);
 void serial8250_suspend_port(int line);
 void serial8250_resume_port(int line);
Index: work5/drivers/serial/8250.c
===================================================================
--- work5.orig/drivers/serial/8250.c	2008-01-15 16:31:49.000000000 -0700
+++ work5/drivers/serial/8250.c	2008-01-16 09:32:20.000000000 -0700
@@ -2785,6 +2785,25 @@
 	return NULL;
 }
 
+static int serial8250_add_port(struct uart_8250_port *uart, struct uart_port *port)
+{
+	uart_remove_one_port(&serial8250_reg, &uart->port);
+
+	uart->port.iobase   = port->iobase;
+	uart->port.membase  = port->membase;
+	uart->port.irq      = port->irq;
+	uart->port.uartclk  = port->uartclk;
+	uart->port.fifosize = port->fifosize;
+	uart->port.regshift = port->regshift;
+	uart->port.iotype   = port->iotype;
+	uart->port.flags    = port->flags | UPF_BOOT_AUTOCONF;
+	uart->port.mapbase  = port->mapbase;
+	if (port->dev)
+		uart->port.dev = port->dev;
+
+	return uart_add_one_port(&serial8250_reg, &uart->port);
+}
+
 /**
  *	serial8250_register_port - register a serial port
  *	@port: serial port template
@@ -2809,32 +2828,58 @@
 	mutex_lock(&serial_mutex);
 
 	uart = serial8250_find_match_or_unused(port);
-	if (uart) {
-		uart_remove_one_port(&serial8250_reg, &uart->port);
+	if (uart)
+		ret = serial8250_add_port(uart, port);
 
-		uart->port.iobase   = port->iobase;
-		uart->port.membase  = port->membase;
-		uart->port.irq      = port->irq;
-		uart->port.uartclk  = port->uartclk;
-		uart->port.fifosize = port->fifosize;
-		uart->port.regshift = port->regshift;
-		uart->port.iotype   = port->iotype;
-		uart->port.flags    = port->flags | UPF_BOOT_AUTOCONF;
-		uart->port.mapbase  = port->mapbase;
-		if (port->dev)
-			uart->port.dev = port->dev;
-
-		ret = uart_add_one_port(&serial8250_reg, &uart->port);
-		if (ret == 0)
-			ret = uart->port.line;
-	}
 	mutex_unlock(&serial_mutex);
 
+	if (ret == 0)
+		return uart->port.line;
 	return ret;
 }
 EXPORT_SYMBOL(serial8250_register_port);
 
 /**
+ *	serial8250_register_port_at - register a serial port at a specific line
+ *	@port: serial port template
+ *	@line: desired line
+ *
+ *	Same as serial8250_register_port(), except that we request a
+ *	specific ttyS name.  This is used for well-known ports like
+ *	COM1-COM4, which people expect to always be ttyS0-ttyS3.
+ *
+ *	This fails if the requested line is already in use, even though
+ *	other lines might still be available.  The caller can retry with
+ *	serial8250_register_port() if desired.
+ */
+int serial8250_register_port_at(struct uart_port *port, int line)
+{
+	struct uart_8250_port *uart;
+	int ret;
+
+	if (port->uartclk == 0)
+		return -EINVAL;
+
+	if (line >= nr_uarts)
+		return -ENOSPC;
+
+	mutex_lock(&serial_mutex);
+
+	uart = &serial8250_ports[line];
+	if (uart->port.type == PORT_UNKNOWN && uart->port.iobase == 0)
+		ret = serial8250_add_port(uart, port);
+	else
+		ret = -EBUSY;
+
+	mutex_unlock(&serial_mutex);
+
+	if (ret == 0)
+		return uart->port.line;
+	return ret;
+}
+EXPORT_SYMBOL(serial8250_register_port_at);
+
+/**
  *	serial8250_unregister_port - remove a 16x50 serial port at runtime
  *	@line: serial line number
  *

-- 

[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;
 

-- 

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:

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

[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:
>>> +
>> 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 0/2] serial: explicitly request ttyS0-3 for COM1-4

[Russell King]

On Wed, Jan 16, 2008 at 10:05:41AM -0700, Bjorn Helgaas wrote:
> When 8250_pnp discovers COM ports, we only get the correct ttyS names
> by accident -- we rely on serial8250_isa_init_ports(), which discovers
> the COM ports earlier using the addresses in SERIAL_PORT_DFNS.

It's not by accident but by design.  It's quite intentional that it
remembers the addresses of serial ports, and if another port is
registered later with the same base address, it gets the same name.

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

Re: [patch 0/2] serial: explicitly request ttyS0-3 for COM1-4

[Bjorn Helgaas]

On Wednesday 16 January 2008 11:39:34 am Russell King wrote:
> On Wed, Jan 16, 2008 at 10:05:41AM -0700, Bjorn Helgaas wrote:
> > When 8250_pnp discovers COM ports, we only get the correct ttyS names
> > by accident -- we rely on serial8250_isa_init_ports(), which discovers
> > the COM ports earlier using the addresses in SERIAL_PORT_DFNS.
> 
> It's not by accident but by design.  It's quite intentional that it
> remembers the addresses of serial ports, and if another port is
> registered later with the same base address, it gets the same name.

It's certainly by design that if we register a port twice, it gets
the same name both times.

But that's not quite what I was referring to.  I meant that I don't
think 8250_pnp should depend on the fact that COM1-4 have been
previously discovered to get the right names.

Bjorn

Re: [patch 0/2] serial: explicitly request ttyS0-3 for COM1-4

[Russell King]

On Wed, Jan 16, 2008 at 12:59:27PM -0700, Bjorn Helgaas wrote:
> On Wednesday 16 January 2008 11:39:34 am Russell King wrote:
> > On Wed, Jan 16, 2008 at 10:05:41AM -0700, Bjorn Helgaas wrote:
> > > When 8250_pnp discovers COM ports, we only get the correct ttyS names
> > > by accident -- we rely on serial8250_isa_init_ports(), which discovers
> > > the COM ports earlier using the addresses in SERIAL_PORT_DFNS.
> > 
> > It's not by accident but by design.  It's quite intentional that it
> > remembers the addresses of serial ports, and if another port is
> > registered later with the same base address, it gets the same name.
> 
> It's certainly by design that if we register a port twice, it gets
> the same name both times.

Incorrect - if it's not detected first time around (eg, the port isn't
accessible at that time), its slot will still be reserved due to the
way slots are given out.

Slots are given to users in order of:

1. does the IO type and base address of a previously known port match
   the new port?  If so, use that.
2. do we have a slot which has never been used - use that.
3. find the first slot which is not currently in use.

So, the only way we'd get the first set of ttyS slots used is if all of
the following are true:

a) the ISA probe doesn't detect them
b) some other driver registers many ports which don't correspond with any
   of the ISA listed addresses and we run out of empty slots
c) 8250_pnp initialises after this driver

(c) is unlikely, except in the modular case - we explicitly list 8250_pnp
immediately after the 8250 driver so that it gets first call on the
slots during initialisation time, before we probe for PCI devices.

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

Re: [patch 0/2] serial: explicitly request ttyS0-3 for COM1-4

[Bjorn Helgaas]

On Wednesday 16 January 2008 01:14:38 pm Russell King wrote:
> On Wed, Jan 16, 2008 at 12:59:27PM -0700, Bjorn Helgaas wrote:
> > On Wednesday 16 January 2008 11:39:34 am Russell King wrote:
> > > On Wed, Jan 16, 2008 at 10:05:41AM -0700, Bjorn Helgaas wrote:
> > > > When 8250_pnp discovers COM ports, we only get the correct ttyS names
> > > > by accident -- we rely on serial8250_isa_init_ports(), which discovers
> > > > the COM ports earlier using the addresses in SERIAL_PORT_DFNS.
> > > 
> > > It's not by accident but by design.  It's quite intentional that it
> > > remembers the addresses of serial ports, and if another port is
> > > registered later with the same base address, it gets the same name.
> > 
> > It's certainly by design that if we register a port twice, it gets
> > the same name both times.
> 
> Incorrect - if it's not detected first time around (eg, the port isn't
> accessible at that time), its slot will still be reserved due to the
> way slots are given out.
> 
> Slots are given to users in order of:
> 
> 1. does the IO type and base address of a previously known port match
>    the new port?  If so, use that.
> 2. do we have a slot which has never been used - use that.
> 3. find the first slot which is not currently in use.
> 
> So, the only way we'd get the first set of ttyS slots used is if all of
> the following are true:
> 
> a) the ISA probe doesn't detect them
> b) some other driver registers many ports which don't correspond with any
>    of the ISA listed addresses and we run out of empty slots
> c) 8250_pnp initialises after this driver
> 
> (c) is unlikely, except in the modular case - we explicitly list 8250_pnp
> immediately after the 8250 driver so that it gets first call on the
> slots during initialisation time, before we probe for PCI devices.

Let me back up a minute.  I was merely trying to make the point that
if we skip the ISA probe (which uses SERIAL_PORT_DFNS), the 8250_pnp
probe will discover the COM ports but won't necessarily name them the
way we expect (e.g., http://lkml.org/lkml/2007/8/10/409).

Bjorn

Re: [patch 0/2] serial: explicitly request ttyS0-3 for COM1-4

[Russell King]

On Thu, Jan 17, 2008 at 09:07:29AM -0700, Bjorn Helgaas wrote:
> On Wednesday 16 January 2008 01:14:38 pm Russell King wrote:
> > On Wed, Jan 16, 2008 at 12:59:27PM -0700, Bjorn Helgaas wrote:
> > > On Wednesday 16 January 2008 11:39:34 am Russell King wrote:
> > > > On Wed, Jan 16, 2008 at 10:05:41AM -0700, Bjorn Helgaas wrote:
> > > > > When 8250_pnp discovers COM ports, we only get the correct ttyS names
> > > > > by accident -- we rely on serial8250_isa_init_ports(), which discovers
> > > > > the COM ports earlier using the addresses in SERIAL_PORT_DFNS.
> > > > 
> > > > It's not by accident but by design.  It's quite intentional that it
> > > > remembers the addresses of serial ports, and if another port is
> > > > registered later with the same base address, it gets the same name.
> > > 
> > > It's certainly by design that if we register a port twice, it gets
> > > the same name both times.
> > 
> > Incorrect - if it's not detected first time around (eg, the port isn't
> > accessible at that time), its slot will still be reserved due to the
> > way slots are given out.
> > 
> > Slots are given to users in order of:
> > 
> > 1. does the IO type and base address of a previously known port match
> >    the new port?  If so, use that.
> > 2. do we have a slot which has never been used - use that.
> > 3. find the first slot which is not currently in use.
> > 
> > So, the only way we'd get the first set of ttyS slots used is if all of
> > the following are true:
> > 
> > a) the ISA probe doesn't detect them
> > b) some other driver registers many ports which don't correspond with any
> >    of the ISA listed addresses and we run out of empty slots
> > c) 8250_pnp initialises after this driver
> > 
> > (c) is unlikely, except in the modular case - we explicitly list 8250_pnp
> > immediately after the 8250 driver so that it gets first call on the
> > slots during initialisation time, before we probe for PCI devices.
> 
> Let me back up a minute.  I was merely trying to make the point that
> if we skip the ISA probe (which uses SERIAL_PORT_DFNS), the 8250_pnp
> probe will discover the COM ports but won't necessarily name them the
> way we expect (e.g., http://lkml.org/lkml/2007/8/10/409).

It would be useful to see the full kernel messages to diagnose what's
going on.  Given that there's no changes to the serial probing code
(or even the code which allocates slots) there's something fishy going
on.  Are they using mainline kernels or do the kernels they're using
have some other changes which aren't in mainline?

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

Re: [patch 0/2] serial: explicitly request ttyS0-3 for COM1-4

[Bjorn Helgaas]

On Thursday 17 January 2008 09:16:51 am Russell King wrote:
> On Thu, Jan 17, 2008 at 09:07:29AM -0700, Bjorn Helgaas wrote:
> > On Wednesday 16 January 2008 01:14:38 pm Russell King wrote:
> > > On Wed, Jan 16, 2008 at 12:59:27PM -0700, Bjorn Helgaas wrote:
> > > > On Wednesday 16 January 2008 11:39:34 am Russell King wrote:
> > > > > On Wed, Jan 16, 2008 at 10:05:41AM -0700, Bjorn Helgaas wrote:
> > > > > > When 8250_pnp discovers COM ports, we only get the correct ttyS names
> > > > > > by accident -- we rely on serial8250_isa_init_ports(), which discovers
> > > > > > the COM ports earlier using the addresses in SERIAL_PORT_DFNS.
> > > > > 
> > > > > It's not by accident but by design.  It's quite intentional that it
> > > > > remembers the addresses of serial ports, and if another port is
> > > > > registered later with the same base address, it gets the same name.
> > > > 
> > > > It's certainly by design that if we register a port twice, it gets
> > > > the same name both times.
> > > 
> > > Incorrect - if it's not detected first time around (eg, the port isn't
> > > accessible at that time), its slot will still be reserved due to the
> > > way slots are given out.
> > > 
> > > Slots are given to users in order of:
> > > 
> > > 1. does the IO type and base address of a previously known port match
> > >    the new port?  If so, use that.
> > > 2. do we have a slot which has never been used - use that.
> > > 3. find the first slot which is not currently in use.
> > > 
> > > So, the only way we'd get the first set of ttyS slots used is if all of
> > > the following are true:
> > > 
> > > a) the ISA probe doesn't detect them
> > > b) some other driver registers many ports which don't correspond with any
> > >    of the ISA listed addresses and we run out of empty slots
> > > c) 8250_pnp initialises after this driver
> > > 
> > > (c) is unlikely, except in the modular case - we explicitly list 8250_pnp
> > > immediately after the 8250 driver so that it gets first call on the
> > > slots during initialisation time, before we probe for PCI devices.
> > 
> > Let me back up a minute.  I was merely trying to make the point that
> > if we skip the ISA probe (which uses SERIAL_PORT_DFNS), the 8250_pnp
> > probe will discover the COM ports but won't necessarily name them the
> > way we expect (e.g., http://lkml.org/lkml/2007/8/10/409).
> 
> It would be useful to see the full kernel messages to diagnose what's
> going on.  Given that there's no changes to the serial probing code
> (or even the code which allocates slots) there's something fishy going
> on.  Are they using mainline kernels or do the kernels they're using
> have some other changes which aren't in mainline?

I'm not trying to fix a current problem.  The URL I quoted was from
2.6.22, where we didn't do the ISA probe.  We fixed that problem by
putting the ISA probe back the way it was in 2.6.21.

But I do still want to remove the dependency between the ISA probe
and the 8250_pnp probe because it seems like it's more complicated
than it needs to be, and if we could figure out a way to safely remove
the ISA probe, it would help solve the IRDA issues.

Bjorn