Use resource_size_t for serial MMIO addresses

Use resource_size_t for serial MMIO addresses

[David Gibson]

Andrew, please apply to -mm.  I think these should be good to merge
for 2.6.22.

At present, MMIO addresses for serial port (the ->mapbase field in
uart_port and other structures) are unsigned longs.  This causes
problems on some 32-bit platforms which have a >32-bit physical
address bus, for example the embedded PowerPC 440GP chip, which has a
36-bit physical address bus, and on-chip serial ports located at an
MMIO address above 4GB.

The second patch in this series changes mapbase to a resource_size_t
(which can be 64-bit on the problematic platforms) in struct uart_port
and struct plat_serial8250_port.  It does *not* change the type in
serial_struct, because that structure is exposed to userspace.  It is
therefore unsafe to use setserial to change the address parameters on
a port using a mapbase above 4GB.

The first patch in the series contains the damage of the setserial
problem.  It allows serial ports to be marked with a new
UPF_FIXED_PORT flag, which causes any attempts to change the port's
type (PIO/MMIO etc.), address or irq with setserial to be ignored.
While using setserial to alter the port address is useful for legacy
ISA ports, it is generally a bad idea for ports such as on-chip or
other hardwired ports where the arch code has good information (from
firmware or hardware probing) on the port's type and address.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 1/2] Define FIXED_PORT flag for serial_core

[David Gibson]

At present, the serial core always allows setserial in userspace to
change the port address, irq and base clock of any serial port.  That
makes sense for legacy ISA ports, but not for (say) embedded ns16550
compatible serial ports at peculiar addresses.  In these cases, the
kernel code configuring the ports must know exactly where they are,
and their clocking arrangements (which can be unusual on embedded
boards).  It doesn't make sense for userspace to change these
settings.

Therefore, this patch defines a UPF_FIXED_PORT flag for the uart_port
structure.  If this flag is set when the serial port is configured,
any attempts to alter the port's type, io address, irq or base clock
with setserial are ignored.

In addition this patch uses the new flag for on-chip serial ports
probed in arch/powerpc/kernel/legacy_serial.c, and for other
hard-wired serial ports probed by drivers/serial/of_serial.c.

Signed-off-by: David Gibson <dwg@au1.ibm.com>
---

 arch/powerpc/kernel/legacy_serial.c |    3 ++-
 drivers/serial/of_serial.c          |    3 ++-
 drivers/serial/serial_core.c        |   22 +++++++++++++---------
 include/linux/serial_core.h         |    1 +
 4 files changed, 18 insertions(+), 11 deletions(-)

Index: working-2.6/drivers/serial/serial_core.c
===================================================================
--- working-2.6.orig/drivers/serial/serial_core.c	2007-02-19 11:05:32.000000000 +1100
+++ working-2.6/drivers/serial/serial_core.c	2007-02-20 11:38:08.000000000 +1100
@@ -672,19 +672,21 @@ static int uart_set_info(struct uart_sta
 	 */
 	mutex_lock(&state->mutex);
 
-	change_irq  = new_serial.irq != port->irq;
+	change_irq  = !(port->flags & UPF_FIXED_PORT)
+		&& new_serial.irq != port->irq;
 
 	/*
 	 * Since changing the 'type' of the port changes its resource
 	 * allocations, we should treat type changes the same as
 	 * IO port changes.
 	 */
-	change_port = new_port != port->iobase ||
-		      (unsigned long)new_serial.iomem_base != port->mapbase ||
-		      new_serial.hub6 != port->hub6 ||
-		      new_serial.io_type != port->iotype ||
-		      new_serial.iomem_reg_shift != port->regshift ||
-		      new_serial.type != port->type;
+	change_port = !(port->flags & UPF_FIXED_PORT)
+		&& (new_port != port->iobase ||
+		    (unsigned long)new_serial.iomem_base != port->mapbase ||
+		    new_serial.hub6 != port->hub6 ||
+		    new_serial.io_type != port->iotype ||
+		    new_serial.iomem_reg_shift != port->regshift ||
+		    new_serial.type != port->type);
 
 	old_flags = port->flags;
 	new_flags = new_serial.flags;
@@ -796,8 +798,10 @@ static int uart_set_info(struct uart_sta
 		}
 	}
 
-	port->irq              = new_serial.irq;
-	port->uartclk          = new_serial.baud_base * 16;
+	if (change_irq)
+		port->irq      = new_serial.irq;
+	if (! (port->flags & UPF_FIXED_PORT))
+		port->uartclk  = new_serial.baud_base * 16;
 	port->flags            = (port->flags & ~UPF_CHANGE_MASK) |
 				 (new_flags & UPF_CHANGE_MASK);
 	port->custom_divisor   = new_serial.custom_divisor;
Index: working-2.6/include/linux/serial_core.h
===================================================================
--- working-2.6.orig/include/linux/serial_core.h	2007-02-19 11:05:32.000000000 +1100
+++ working-2.6/include/linux/serial_core.h	2007-02-20 11:38:08.000000000 +1100
@@ -260,6 +260,7 @@ struct uart_port {
 #define UPF_CONS_FLOW		((__force upf_t) (1 << 23))
 #define UPF_SHARE_IRQ		((__force upf_t) (1 << 24))
 #define UPF_BOOT_AUTOCONF	((__force upf_t) (1 << 28))
+#define UPF_FIXED_PORT		((__force upf_t) (1 << 29))
 #define UPF_DEAD		((__force upf_t) (1 << 30))
 #define UPF_IOREMAP		((__force upf_t) (1 << 31))
 
Index: working-2.6/arch/powerpc/kernel/legacy_serial.c
===================================================================
--- working-2.6.orig/arch/powerpc/kernel/legacy_serial.c	2007-02-15 10:05:18.000000000 +1100
+++ working-2.6/arch/powerpc/kernel/legacy_serial.c	2007-02-20 10:50:16.000000000 +1100
@@ -115,7 +115,8 @@ static int __init add_legacy_soc_port(st
 {
 	u64 addr;
 	const u32 *addrp;
-	upf_t flags = UPF_BOOT_AUTOCONF | UPF_SKIP_TEST | UPF_SHARE_IRQ;
+	upf_t flags = UPF_BOOT_AUTOCONF | UPF_SKIP_TEST | UPF_SHARE_IRQ
+		| UPF_FIXED_PORT;
 	struct device_node *tsi = of_get_parent(np);
 
 	/* We only support ports that have a clock frequency properly
Index: working-2.6/drivers/serial/of_serial.c
===================================================================
--- working-2.6.orig/drivers/serial/of_serial.c	2007-02-20 11:38:16.000000000 +1100
+++ working-2.6/drivers/serial/of_serial.c	2007-02-20 11:38:30.000000000 +1100
@@ -48,7 +48,8 @@ static int __devinit of_platform_serial_
 	port->iotype = UPIO_MEM;
 	port->type = type;
 	port->uartclk = *clk;
-	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP;
+	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
+		| UPF_FIXED_PORT;
 	port->dev = &ofdev->dev;
 	port->custom_divisor = *clk / (16 * (*spd));
 

Re: [PATCH 1/2] Define FIXED_PORT flag for serial_core

[Alan]

On Tue, 20 Feb 2007 14:19:51 +1100 (EST)
David Gibson <david@gibson.dropbear.id.au> wrote:

> At present, the serial core always allows setserial in userspace to
> change the port address, irq and base clock of any serial port.  That
> makes sense for legacy ISA ports, but not for (say) embedded ns16550
> compatible serial ports at peculiar addresses.  In these cases, the
> kernel code configuring the ports must know exactly where they are,
> and their clocking arrangements (which can be unusual on embedded
> boards).  It doesn't make sense for userspace to change these
> settings.
> 
> Therefore, this patch defines a UPF_FIXED_PORT flag for the uart_port
> structure.  If this flag is set when the serial port is configured,
> any attempts to alter the port's type, io address, irq or base clock
> with setserial are ignored.
> 
> In addition this patch uses the new flag for on-chip serial ports
> probed in arch/powerpc/kernel/legacy_serial.c, and for other
> hard-wired serial ports probed by drivers/serial/of_serial.c.
> 
> Signed-off-by: David Gibson <dwg@au1.ibm.com>
>
Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH 1/2] Define FIXED_PORT flag for serial_core

[Russell King]

On Tue, Feb 20, 2007 at 02:19:51PM +1100, David Gibson wrote:
> Therefore, this patch defines a UPF_FIXED_PORT flag for the uart_port
> structure.  If this flag is set when the serial port is configured,
> any attempts to alter the port's type, io address, irq or base clock
> with setserial are ignored.

I've been wondering about this, and it is questionable whether we
should allow any serial port which isn't owned by the legacy platform
device (the one called "serial8250", iow by the 8250 driver itself)
to have the base addresses and interrupts changed.

IOW, we apply this "fixed port" to any port registered by probe
modules external to the 8250 driver itself, such as PCI, PNP, etc.

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

Re: [PATCH 1/2] Define FIXED_PORT flag for serial_core

[David Gibson]

On Wed, Feb 28, 2007 at 10:26:30PM +0000, Russell King wrote:
> On Tue, Feb 20, 2007 at 02:19:51PM +1100, David Gibson wrote:
> > Therefore, this patch defines a UPF_FIXED_PORT flag for the uart_port
> > structure.  If this flag is set when the serial port is configured,
> > any attempts to alter the port's type, io address, irq or base clock
> > with setserial are ignored.
> 
> I've been wondering about this, and it is questionable whether we
> should allow any serial port which isn't owned by the legacy platform
> device (the one called "serial8250", iow by the 8250 driver itself)
> to have the base addresses and interrupts changed.
> 
> IOW, we apply this "fixed port" to any port registered by probe
> modules external to the 8250 driver itself, such as PCI, PNP, etc.

Sounds reasonable to me.  But maybe in that case we should invert the
sense of the flag.  UPF_MOVABLE_PORT or UPF_USER_CONFIGURABLE or
something.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 1/2] Define FIXED_PORT flag for serial_core

[Russell King]

On Thu, Mar 01, 2007 at 10:44:24AM +1100, David Gibson wrote:
> On Wed, Feb 28, 2007 at 10:26:30PM +0000, Russell King wrote:
> > On Tue, Feb 20, 2007 at 02:19:51PM +1100, David Gibson wrote:
> > > Therefore, this patch defines a UPF_FIXED_PORT flag for the uart_port
> > > structure.  If this flag is set when the serial port is configured,
> > > any attempts to alter the port's type, io address, irq or base clock
> > > with setserial are ignored.
> > 
> > I've been wondering about this, and it is questionable whether we
> > should allow any serial port which isn't owned by the legacy platform
> > device (the one called "serial8250", iow by the 8250 driver itself)
> > to have the base addresses and interrupts changed.
> > 
> > IOW, we apply this "fixed port" to any port registered by probe
> > modules external to the 8250 driver itself, such as PCI, PNP, etc.
> 
> Sounds reasonable to me.  But maybe in that case we should invert the
> sense of the flag.  UPF_MOVABLE_PORT or UPF_USER_CONFIGURABLE or
> something.

I was thinking about not even having a flag, but instead checking for
port->dev == &serial8250_isa_devs->dev.

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

Re: [PATCH 1/2] Define FIXED_PORT flag for serial_core

[David Gibson]

On Thu, Mar 01, 2007 at 10:30:04AM +0000, Russell King wrote:
> On Thu, Mar 01, 2007 at 10:44:24AM +1100, David Gibson wrote:
> > On Wed, Feb 28, 2007 at 10:26:30PM +0000, Russell King wrote:
> > > On Tue, Feb 20, 2007 at 02:19:51PM +1100, David Gibson wrote:
> > > > Therefore, this patch defines a UPF_FIXED_PORT flag for the uart_port
> > > > structure.  If this flag is set when the serial port is configured,
> > > > any attempts to alter the port's type, io address, irq or base clock
> > > > with setserial are ignored.
> > > 
> > > I've been wondering about this, and it is questionable whether we
> > > should allow any serial port which isn't owned by the legacy platform
> > > device (the one called "serial8250", iow by the 8250 driver itself)
> > > to have the base addresses and interrupts changed.
> > > 
> > > IOW, we apply this "fixed port" to any port registered by probe
> > > modules external to the 8250 driver itself, such as PCI, PNP, etc.
> > 
> > Sounds reasonable to me.  But maybe in that case we should invert the
> > sense of the flag.  UPF_MOVABLE_PORT or UPF_USER_CONFIGURABLE or
> > something.
> 
> I was thinking about not even having a flag, but instead checking for
> port->dev == &serial8250_isa_devs->dev.

Hmm.. ok.  Will you spin a patch, or should I?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 2/2] Use resource_size_t for serial port IO addresses

[David Gibson]

At present, various parts of the serial code use unsigned long to
define resource addresses.  This is a problem, because some 32-bit
platforms have physical addresses larger than 32-bits, and have mmio
serial uarts located above the 4GB point.

This patch changes the type of mapbase in both struct uart_port and
struct plat_serial8250_port to resource_size_t, which can be
configured to be 64 bits on such platforms.  The mapbase in
serial_struct can't safely be changed, because that structure is user
visible.

Signed-off-by: David Gibson <dwg@au1.ibm.com>
---

 drivers/serial/8250.c        |    5 +++--
 drivers/serial/8250_early.c  |   16 +++++++++-------
 drivers/serial/serial_core.c |    9 +++++----
 include/linux/serial_8250.h  |    2 +-
 include/linux/serial_core.h  |    2 +-
 5 files changed, 19 insertions(+), 15 deletions(-)

Index: working-2.6/include/linux/serial_core.h
===================================================================
--- working-2.6.orig/include/linux/serial_core.h	2007-02-19 11:07:42.000000000 +1100
+++ working-2.6/include/linux/serial_core.h	2007-02-19 11:07:43.000000000 +1100
@@ -273,7 +273,7 @@ struct uart_port {
 	const struct uart_ops	*ops;
 	unsigned int		custom_divisor;
 	unsigned int		line;			/* port index */
-	unsigned long		mapbase;		/* for ioremap */
+	resource_size_t		mapbase;		/* for ioremap */
 	struct device		*dev;			/* parent device */
 	unsigned char		hub6;			/* this should be in the 8250 driver */
 	unsigned char		unused[3];
Index: working-2.6/drivers/serial/serial_core.c
===================================================================
--- working-2.6.orig/drivers/serial/serial_core.c	2007-02-19 11:07:42.000000000 +1100
+++ working-2.6/drivers/serial/serial_core.c	2007-02-19 11:07:43.000000000 +1100
@@ -633,7 +633,7 @@ static int uart_get_info(struct uart_sta
 	tmp.hub6	    = port->hub6;
 	tmp.io_type         = port->iotype;
 	tmp.iomem_reg_shift = port->regshift;
-	tmp.iomem_base      = (void *)port->mapbase;
+	tmp.iomem_base      = (void *)(unsigned long)port->mapbase;
 
 	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
 		return -EFAULT;
@@ -1673,10 +1673,11 @@ static int uart_line_info(char *buf, str
 		return 0;
 
 	mmio = port->iotype >= UPIO_MEM;
-	ret = sprintf(buf, "%d: uart:%s %s%08lX irq:%d",
+	ret = sprintf(buf, "%d: uart:%s %s%08llX irq:%d",
 			port->line, uart_type(port),
 			mmio ? "mmio:0x" : "port:",
-			mmio ? port->mapbase : (unsigned long) port->iobase,
+			mmio ? (unsigned long long)port->mapbase
+		             : (unsigned long long) port->iobase,
 			port->irq);
 
 	if (port->type == PORT_UNKNOWN) {
@@ -2069,7 +2070,7 @@ uart_report_port(struct uart_driver *drv
 	case UPIO_AU:
 	case UPIO_TSI:
 		snprintf(address, sizeof(address),
-			 "MMIO 0x%lx", port->mapbase);
+			 "MMIO 0x%llx", (unsigned long long)port->mapbase);
 		break;
 	default:
 		strlcpy(address, "*unknown*", sizeof(address));
Index: working-2.6/drivers/serial/8250_early.c
===================================================================
--- working-2.6.orig/drivers/serial/8250_early.c	2007-02-19 11:07:40.000000000 +1100
+++ working-2.6/drivers/serial/8250_early.c	2007-02-19 11:07:43.000000000 +1100
@@ -145,8 +145,9 @@ static int __init parse_options(struct e
 		port->mapbase = simple_strtoul(options + 5, &options, 0);
 		port->membase = ioremap(port->mapbase, mapsize);
 		if (!port->membase) {
-			printk(KERN_ERR "%s: Couldn't ioremap 0x%lx\n",
-				__FUNCTION__, port->mapbase);
+			printk(KERN_ERR "%s: Couldn't ioremap 0x%llx\n",
+				__FUNCTION__,
+			       (unsigned long long)port->mapbase);
 			return -ENOMEM;
 		}
 		mmio = 1;
@@ -168,9 +169,10 @@ static int __init parse_options(struct e
 			device->baud);
 	}
 
-	printk(KERN_INFO "Early serial console at %s 0x%lx (options '%s')\n",
+	printk(KERN_INFO "Early serial console at %s 0x%llx (options '%s')\n",
 		mmio ? "MMIO" : "I/O port",
-		mmio ? port->mapbase : (unsigned long) port->iobase,
+		mmio ? (unsigned long long) port->mapbase
+	             : (unsigned long long) port->iobase,
 		device->options);
 	return 0;
 }
@@ -236,10 +238,10 @@ static int __init early_uart_console_swi
 	mmio = (port->iotype == UPIO_MEM);
 	line = serial8250_start_console(port, device->options);
 	if (line < 0)
-		printk("No ttyS device at %s 0x%lx for console\n",
+		printk("No ttyS device at %s 0x%llx for console\n",
 			mmio ? "MMIO" : "I/O port",
-			mmio ? port->mapbase :
-			    (unsigned long) port->iobase);
+			mmio ? (unsigned long long) port->mapbase
+		             : (unsigned long long) port->iobase);
 
 	unregister_console(&early_uart_console);
 	if (mmio)
Index: working-2.6/include/linux/serial_8250.h
===================================================================
--- working-2.6.orig/include/linux/serial_8250.h	2007-02-19 11:07:40.000000000 +1100
+++ working-2.6/include/linux/serial_8250.h	2007-02-19 11:07:43.000000000 +1100
@@ -20,7 +20,7 @@
 struct plat_serial8250_port {
 	unsigned long	iobase;		/* io base address */
 	void __iomem	*membase;	/* ioremap cookie or NULL */
-	unsigned long	mapbase;	/* resource base */
+	resource_size_t	mapbase;	/* resource base */
 	unsigned int	irq;		/* interrupt number */
 	unsigned int	uartclk;	/* UART clock rate */
 	unsigned char	regshift;	/* register shift */
Index: working-2.6/drivers/serial/8250.c
===================================================================
--- working-2.6.orig/drivers/serial/8250.c	2007-02-19 11:07:40.000000000 +1100
+++ working-2.6/drivers/serial/8250.c	2007-02-19 11:07:43.000000000 +1100
@@ -2550,8 +2550,9 @@ static int __devinit serial8250_probe(st
 		ret = serial8250_register_port(&port);
 		if (ret < 0) {
 			dev_err(&dev->dev, "unable to register port at index %d "
-				"(IO%lx MEM%lx IRQ%d): %d\n", i,
-				p->iobase, p->mapbase, p->irq, ret);
+				"(IO%lx MEM%llx IRQ%d): %d\n", i,
+				p->iobase, (unsigned long long)p->mapbase,
+				p->irq, ret);
 		}
 	}
 	return 0;

Re: [PATCH 2/2] Use resource_size_t for serial port IO addresses

[Alan]

On Tue, 20 Feb 2007 14:19:51 +1100 (EST)
David Gibson <david@gibson.dropbear.id.au> wrote:

> At present, various parts of the serial code use unsigned long to
> define resource addresses.  This is a problem, because some 32-bit
> platforms have physical addresses larger than 32-bits, and have mmio
> serial uarts located above the 4GB point.
> 
> This patch changes the type of mapbase in both struct uart_port and
> struct plat_serial8250_port to resource_size_t, which can be
> configured to be 64 bits on such platforms.  The mapbase in
> serial_struct can't safely be changed, because that structure is user
> visible.
> 
> Signed-off-by: David Gibson <dwg@au1.ibm.com>

Acked-by: Alan Cox <alan@redhat.com>