serial: Initialize spinlocks in 8250 and don't clobber them.

Initialize spinlocks in 8250 and don't clobber them.

Spinlock debugging fails in 8250.c because the lock fields in irq_lists are not initialized. Initialize them.

In serial8250_isa_init_ports(), the port's lock is initialized. We should not overwrite it. Only copy in the fields we need.

Signed-off-by David Daney <[email protected]>

Signed-off-by Tomaso Paoletti <[email protected]>

---
 drivers/serial/8250.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

drivers/serial/8250.c +0 -0

index d4104a3..0688799 100644

drivers/serial/8250.c +6 -2

 	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;

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.

Quoted 19 lines

(quoted diff) +6 -2

 	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;

Review static struct uart_driver serial8250_reg = { · line 2724

Quoted 10 lines

(quoted diff) +8 -0

+	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?

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?

Quoted 3 lines

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?

Kind of irrelevant now however, the split of patches that caused the original bug is over and the NR_IRQ removal patch half of it hit Linus tree.

The question is: What is the best way to initialize some (or all) fields of a structure *except* a single lock field that was previously initialized?