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
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 <[email protected]>
---
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));
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 <[email protected]>
---
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;
On Tue, 20 Feb 2007 14:19:51 +1100 (EST)
David Gibson <[email protected]> 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 <[email protected]>
Acked-by: Alan Cox <[email protected]>
On Tue, 20 Feb 2007 14:19:51 +1100 (EST)
David Gibson <[email protected]> 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 <[email protected]>
>
Acked-by: Alan Cox <[email protected]>
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:
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