Andrew Morton wrote:
[...]
OK.. But serial8250_isa_init_ports() has so many callsites that I'd
worry that we end up running this initialisation multiple times. Say,
if the right combination of boot options is provided? This is probably
a benign thing, but it's not desirable.
A simple "fix" would be
static void __init irq_lists_init(void)
{
static unsigned long done;
if (!test_and_set_bit(0, &done)) {
int i;
for (i = 0; i < ARRAY_SIZE(irq_lists); i++)
spin_lock_init(&irq_lists[i].lock);
}
}
A better fix would be to initialise all those spinlocks at compile
time. But given the need to pass the address of each lock into each
lock's initialiser, that could be tricky.
Alan Cox already fixed this part different way.
for (i = 0; i < nr_uarts; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
@@ -2699,12 +2702,24 @@ static struct uart_driver serial8250_reg = {
*/
int __init early_serial_setup(struct uart_port *port)
{
+ struct uart_port *p;
+
if (port->line >= ARRAY_SIZE(serial8250_ports))
return -ENODEV;
serial8250_isa_init_ports();
- serial8250_ports[port->line].port = *port;
- serial8250_ports[port->line].port.ops = &serial8250_pops;
+ p = &serial8250_ports[port->line].port;
+ p->iobase = port->iobase;
+ p->membase = port->membase;
+ p->irq = port->irq;
+ p->uartclk = port->uartclk;
+ p->fifosize = port->fifosize;
+ p->regshift = port->regshift;
+ p->iotype = port->iotype;
+ p->flags = port->flags;
+ p->mapbase = port->mapbase;
+ p->private_data = port->private_data;
+ p->ops = &serial8250_pops;
return 0;
}
Having to spell out each member like this is pretty nasty from a
maintainability point of view. If new fields are added to uart_port,
we surely will forget to update this code.
But yes, copying a spinlock by value is quite wrong. Perhaps we could
retain the struct assigment and then run spin_lock_init() to get the
spinlock into a sane state?
It is ugly, I will think about this part more.
Thanks,
David Daney
