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 <[email protected]>
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;
}
>>>>> On Wed, 25 Jan 2006 22:24:03 -0500, "George G. Davis" <[email protected]> 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
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" <[email protected]> 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
>>>>> On Wed, 1 Feb 2006 10:45:49 -0500, "George G. Davis" <[email protected]> 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
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" <[email protected]> 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
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
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