[PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[George G. Davis]

Need spin_lock_init(&serial8250_ports[port->line].port.lock) in
early_serial_setup() since we're copying struct uart_port *port
into serial8250_ports[port->line].port and *port.lock is typically
unitiliased by the caller.

Signed-off-by: George G. Davis <gdavis@mvista.com>

Index: linux-2.6/drivers/serial/8250.c
===================================================================
--- linux-2.6.orig/drivers/serial/8250.c
+++ linux-2.6/drivers/serial/8250.c
@@ -2340,6 +2340,7 @@ int __init early_serial_setup(struct uar
 	serial8250_isa_init_ports();
 	serial8250_ports[port->line].port	= *port;
 	serial8250_ports[port->line].port.ops	= &serial8250_pops;
+	spin_lock_init(&serial8250_ports[port->line].port.lock);
 	return 0;
 }
 

Re: [PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[Atsushi Nemoto]

>>>>> On Wed, 25 Jan 2006 22:24:03 -0500, "George G. Davis" <gdavis@mvista.com> said:

gdavis> Need spin_lock_init(&serial8250_ports[port->line].port.lock)
gdavis> in early_serial_setup() since we're copying struct uart_port
gdavis> *port into serial8250_ports[port->line].port and *port.lock is
gdavis> typically unitiliased by the caller.

Is this really needed?   The port.lock will be initialized in
uart_set_options() or uart_add_one_port().

I think spin_lock_init() in serial8250_isa_init_ports() can be omitted
also.

---
Atsushi Nemoto

Re: [PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[George G. Davis]

On Tue, Jan 31, 2006 at 12:39:27AM +0900, Atsushi Nemoto wrote:
> >>>>> On Wed, 25 Jan 2006 22:24:03 -0500, "George G. Davis" <gdavis@mvista.com> said:
> 
> gdavis> Need spin_lock_init(&serial8250_ports[port->line].port.lock)
> gdavis> in early_serial_setup() since we're copying struct uart_port
> gdavis> *port into serial8250_ports[port->line].port and *port.lock is
> gdavis> typically unitiliased by the caller.
> 
> Is this really needed?

Yes, IMO, it is required for targets which use early_serial_setup().  If
you have doubts, enable DEBUG_SPINLOCK and you will see the problem but
only if your target uses early_serial_setup().

> The port.lock will be initialized in
> uart_set_options()

uart_set_options() is only called from serial8250_console_setup() when 8250
serial devices have already been registered via serial8250_isa_init_ports()
(for legacy devices only though AFAICT).

> or uart_add_one_port().

But uart_add_one_port() intentionally does not spin_lock_init() the
port.lock of the serial console device under the assumption that
it is already done.


Here's the call sequence:


start_kernel()
	...
	console_init()
		...
		serial8250_console_init()
			serial8250_isa_init_ports()
				if (first)
					spin_lock_init() /* port.lock init */
				...

All 8250 serial port.locks are now initialised but no ports have
been registered (on targets which do not register legacy serial ports
via old_serial_port[]) at this point.


			register_console()
				serial8250_console_setup() /* -ENODEV */
	...
	rest_init()
		...
		/* arch_initcalls */
		early_serial_setup()
			serial8250_isa_init_ports() /* !first, do nothing */
			serial8250_ports[port->line].port = *port;
			...

serial8250_ports[port->line].port.lock is over written above and may
not be properly initialised.


		/* module_inits */
		serial8250_init()
			...
			serial8250_register_ports()
				serial8250_isa_init_ports() /* !first */
				uart_add_one_port()
					if (!uart_console(port))
						spin_lock_init(&port->lock);
					uart_configure_port()


At this point I see the following with DEBUG_SPINLOCK enabled:


Serial: 8250/16550 driver $Revision: 1.90 $ 4 ports, IRQ sharing disabled
BUG: spinlock bad magic on CPU#0, swapper/1
 lock: c02fcffc, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
[<c002bdec>] (dump_stack+0x0/0x14) from [<c0136268>] (spin_bug+0x94/0xac)
[<c01361d4>] (spin_bug+0x0/0xac) from [<c01362a8>] (_raw_spin_lock+0x28/0x160)
 r5 = A0000013  r4 = C02FCFFC 
[<c0136280>] (_raw_spin_lock+0x0/0x160) from [<c024459c>] (_spin_lock_irqsave+0x2c/0x34)
 r8 = 00000000  r7 = C02FCFFC  r6 = C02A4144  r5 = A0000013
 r4 = C02FCFFC 
[<c0244570>] (_spin_lock_irqsave+0x0/0x34) from [<c0168748>] (serial8250_config_port+0x7c/0x988)
 r5 = C02FCFFC  r4 = C02FCFFC 
[<c01686cc>] (serial8250_config_port+0x0/0x988) from [<c01668cc>] (uart_add_one_port+0x110/0x2a0)
[<c01667bc>] (uart_add_one_port+0x0/0x2a0) from [<c001a174>] (serial8250_init+0xec/0x170)
 r8 = C02A40F4  r7 = C1C7B2B0  r6 = C02FD2FC  r5 = 00000000
 r4 = C02FCFFC 
[<c001a088>] (serial8250_init+0x0/0x170) from [<c0026110>] (init+0xa0/0x228)
 r8 = 00000000  r7 = C00205B4  r6 = 00000001  r5 = C0374000
 r4 = C0020504 
[<c0026070>] (init+0x0/0x228) from [<c00446cc>] (do_exit+0x0/0x874)
 r7 = 00000000  r6 = 00000000  r5 = 00000000  r4 = 00000000
BUG: spinlock lockup on CPU#0, swapper/1, c02fcffc
[<c002bdec>] (dump_stack+0x0/0x14) from [<c01363a8>] (_raw_spin_lock+0x128/0x160)
[<c0136280>] (_raw_spin_lock+0x0/0x160) from [<c024459c>] (_spin_lock_irqsave+0x2c/0x34)
 r8 = 00000000  r7 = C02FCFFC  r6 = C02A4144  r5 = A0000013
 r4 = C02FCFFC 
[<c0244570>] (_spin_lock_irqsave+0x0/0x34) from [<c0168748>] (serial8250_config_port+0x7c/0x988)
 r5 = C02FCFFC  r4 = C02FCFFC 
[<c01686cc>] (serial8250_config_port+0x0/0x988) from [<c01668cc>] (uart_add_one_port+0x110/0x2a0)
[<c01667bc>] (uart_add_one_port+0x0/0x2a0) from [<c001a174>] (serial8250_init+0xec/0x170)
 r8 = C02A40F4  r7 = C1C7B2B0  r6 = C02FD2FC  r5 = 00000000
 r4 = C02FCFFC 
[<c001a088>] (serial8250_init+0x0/0x170) from [<c0026110>] (init+0xa0/0x228)
 r8 = 00000000  r7 = C00205B4  r6 = 00000001  r5 = C0374000
 r4 = C0020504 
[<c0026070>] (init+0x0/0x228) from [<c00446cc>] (do_exit+0x0/0x874)
 r7 = 00000000  r6 = 00000000  r5 = 00000000  r4 = 00000000


Adding spin_lock_init() in early_serial_setup() resolves the above.  Perhaps
there is a better way to fix this?

> 
> I think spin_lock_init() in serial8250_isa_init_ports() can be omitted
> also.

AFAICS, there are still a few corner cases where port.lock's are not
initialised on targets which do not define legacy serial ports via
old_serial_port[].


Thanks!

--
Regards,
George
> 
> ---
> Atsushi Nemoto

Re: [PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[Atsushi Nemoto]

>>>>> On Wed, 1 Feb 2006 10:45:49 -0500, "George G. Davis" <gdavis@mvista.com> said:

gdavis> uart_set_options() is only called from
gdavis> serial8250_console_setup() when 8250 serial devices have
gdavis> already been registered via serial8250_isa_init_ports() (for
gdavis> legacy devices only though AFAICT).

OK, I see.  Then, please look at this patch in 2.6.16-rc1-mm4.

serial-initialize-spinlock-for-port-failed-to-setup.patch

Does this fix your problem ?

---
Atsushi Nemoto

Re: [PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[George G. Davis]

On Thu, Feb 02, 2006 at 01:01:18AM +0900, Atsushi Nemoto wrote:
> >>>>> On Wed, 1 Feb 2006 10:45:49 -0500, "George G. Davis" <gdavis@mvista.com> said:
> 
> gdavis> uart_set_options() is only called from
> gdavis> serial8250_console_setup() when 8250 serial devices have
> gdavis> already been registered via serial8250_isa_init_ports() (for
> gdavis> legacy devices only though AFAICT).
> 
> OK, I see.  Then, please look at this patch in 2.6.16-rc1-mm4.
> 
> serial-initialize-spinlock-for-port-failed-to-setup.patch
> 
> Does this fix your problem ?

That fixes the problem.  Thanks!

--
Regards,
George

Re: [PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[Russell King]

On Wed, Feb 01, 2006 at 10:45:49AM -0500, George G. Davis wrote:
> But uart_add_one_port() intentionally does not spin_lock_init() the
> port.lock of the serial console device under the assumption that
> it is already done.

Yes, and there's a bug in there atm...

> Here's the call sequence:
> 
> start_kernel()
> 	...
> 	console_init()
> 		...
> 		serial8250_console_init()
> 			serial8250_isa_init_ports()
> 				if (first)
> 					spin_lock_init() /* port.lock init */
> 				...
> 
> All 8250 serial port.locks are now initialised but no ports have
> been registered (on targets which do not register legacy serial ports
> via old_serial_port[]) at this point.

This initialisation is actually pointless here.

> 
> 			register_console()
> 				serial8250_console_setup() /* -ENODEV */
> 	...
> 	rest_init()
> 		...
> 		/* arch_initcalls */
> 		early_serial_setup()

This is where it goes wrong.  Don't call early_serial_setup() after
"early".  Use a platform device instead.

I absolutely detest the number of ways to initialise an 8250 port -
I'd like there to be only one way, but that doesn't satisfy everyone.
That one way is via platform devices.  Please use that method in
preference to everything else, _especially_ from architecture code.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] serial: Add spin_lock_init() in 8250 early_serial_setup() to init port.lock

[George G. Davis]

On Wed, Feb 01, 2006 at 11:00:13PM +0000, Russell King wrote:
> On Wed, Feb 01, 2006 at 10:45:49AM -0500, George G. Davis wrote:
> > But uart_add_one_port() intentionally does not spin_lock_init() the
> > port.lock of the serial console device under the assumption that
> > it is already done.
> 
> Yes, and there's a bug in there atm...

Which is resolved via:

ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.16-rc1/2.6.16-rc1-mm4/broken-out/serial-initialize-spinlock-for-port-failed-to-setup.patch

Since the above fixes the problem, this patch is withdrawn.

> > 		serial8250_console_init()
> > 			serial8250_isa_init_ports()
> > 				if (first)
> > 					spin_lock_init() /* port.lock init */

> This initialisation is actually pointless here.

Agreed, when serial-initialize-spinlock-for-port-failed-to-setup.patch is
applied, the above spin_lock_init() is no longer required.

> > 			register_console()
> > 				serial8250_console_setup() /* -ENODEV */
> > 	...
> > 	rest_init()
> > 		...
> > 		/* arch_initcalls */
> > 		early_serial_setup()
> 
> This is where it goes wrong.  Don't call early_serial_setup() after
> "early".  Use a platform device instead.

Ok, this was perhaps a bad example, some machines call early_serial_setup()
much earlier via setup_arch() or sooner.  This case used an arch_initcall
which is admittedly rather late to be calling early_* funcs.  My bad...

> I absolutely detest the number of ways to initialise an 8250 port -
> I'd like there to be only one way, but that doesn't satisfy everyone.
> That one way is via platform devices.

FWIW, I agree but had to use early_serial_setup() for other reasons (kgdb8250
vs. standard 8250 serial port fights in an earlier 2.6 kernel release).  Well,
aside from my brain damaged use of early_serial_setup(), there was a real
bug initialising the serial console spinlock.  So it wasn't a total waste
of time.  : )

> Please use that method in
> preference to everything else, _especially_ from architecture code.

FWIW, I also prefer to see early_serial_setup() killed off and force current
users to register 8250 ports via platform devices.  But I'm stuck with
early_serial_setup() in my current case.

Thanks for your comments.

--
Regards,
George