Messages in this thread | | | Date | Mon, 20 Oct 2008 14:37:50 -0700 | From | David Daney <> | Subject | Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber them. |
| |
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
| |