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

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

[David Daney]

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 <ddaney@caviumnetworks.com>
Signed-off-by: Tomaso Paoletti <tpaoletti@caviumnetworks.com>
---
 drivers/serial/8250.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index d4104a3..0688799 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2494,6 +2494,9 @@ static void __init serial8250_isa_init_ports(void)
 		return;
 	first = 0;
 
+	for (i = 0; i < ARRAY_SIZE(irq_lists); i++)
+		spin_lock_init(&irq_lists[i].lock);
+
 	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;
 }
 

Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber them.

[Andrew Morton]

On Tue, 14 Oct 2008 14:37:24 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> Initialize spinlocks in 8250 and don't clobber them.

That's actually quite bad.  There's no reason why an all-zeroes pattern
for a spinlock_t correctly represents the unlocked state.  I guess we
got lucky on the architectures which use this code.

> 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 <ddaney@caviumnetworks.com>
> Signed-off-by: Tomaso Paoletti <tpaoletti@caviumnetworks.com>
> ---
>  drivers/serial/8250.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index d4104a3..0688799 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -2494,6 +2494,9 @@ static void __init serial8250_isa_init_ports(void)
>  		return;
>  	first = 0;
>  
> +	for (i = 0; i < ARRAY_SIZE(irq_lists); i++)
> +		spin_lock_init(&irq_lists[i].lock);

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.

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

Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber them.

[David Daney]

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

Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber them.

[Alan Cox]

> 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.

Alan

Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber them.

[David Daney]

Alan Cox wrote:
>> 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.
> 
My original patch fixed *two* problems.  As you note here, you already fixed the first one.

As far as I know, the second problem is still present, and that is what akpm was referring to above.  Several days ago I posted a revised patch for this here:

http://marc.info/?l=linux-serial&m=122408950013741&w=2

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?

We can just copy field by field as my patch does, or you could do something ugly using memcpy on portions of the structure.  In this case we know which structure elements will be used by the early console, so I just copied them.

Any comments about that patch are certainly most welcome.

Thanks,
David Daney

Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber them.

[Alan Cox]

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

Move the initialisation - or at least memset to zero then spin_lock_init
and fill in the other fields later.