[PATCH RESEND] timbuart: Support for beeing probed more than once

[PATCH RESEND] timbuart: Support for beeing probed more than once

[Richard Röjfors]

There was a problem in the current implementation where a global static
uart_driver struct was used. The same struct was reused every time the
driver got probed. Since the struct has a state within the serial core
it can not be reused.

A uart_driver struct is added to the timbuart_port struct which is
allocated per platform device.

The probe and remove functions are declared __devinit and __devexit.

Signed-off-by: Richard Röjfors <richard.rojfors@mocean-labs.com>
---
diff --git a/drivers/serial/timbuart.c b/drivers/serial/timbuart.c
index 063a313..75a741b 100644
--- a/drivers/serial/timbuart.c
+++ b/drivers/serial/timbuart.c
@@ -31,6 +31,7 @@

 struct timbuart_port {
 	struct uart_port	port;
+	struct uart_driver	uart_driver;
 	struct tasklet_struct	tasklet;
 	int			usedma;
 	u32			last_ier;
@@ -410,7 +411,7 @@ static struct uart_ops timbuart_ops = {
 	.verify_port = timbuart_verify_port
 };

-static struct uart_driver timbuart_driver = {
+static const __devinitconst struct uart_driver timbuart_driver_template = {
 	.owner = THIS_MODULE,
 	.driver_name = "timberdale_uart",
 	.dev_name = "ttyTU",
@@ -419,7 +420,7 @@ static struct uart_driver timbuart_driver = {
 	.nr = 1
 };

-static int timbuart_probe(struct platform_device *dev)
+static int __devinit timbuart_probe(struct platform_device *dev)
 {
 	int err;
 	struct timbuart_port *uart;
@@ -433,6 +434,8 @@ static int timbuart_probe(struct platform_device *dev)
 		goto err_mem;
 	}

+	uart->uart_driver = timbuart_driver_template;
+
 	uart->usedma = 0;

 	uart->port.uartclk = 3250000 * 16;
@@ -461,11 +464,11 @@ static int timbuart_probe(struct platform_device *dev)

 	tasklet_init(&uart->tasklet, timbuart_tasklet, (unsigned long)uart);

-	err = uart_register_driver(&timbuart_driver);
+	err = uart_register_driver(&uart->uart_driver);
 	if (err)
 		goto err_register;

-	err = uart_add_one_port(&timbuart_driver, &uart->port);
+	err = uart_add_one_port(&uart->uart_driver, &uart->port);
 	if (err)
 		goto err_add_port;

@@ -474,7 +477,7 @@ static int timbuart_probe(struct platform_device *dev)
 	return 0;

 err_add_port:
-	uart_unregister_driver(&timbuart_driver);
+	uart_unregister_driver(&uart->uart_driver);
 err_register:
 	kfree(uart);
 err_mem:
@@ -484,13 +487,13 @@ err_mem:
 	return err;
 }

-static int timbuart_remove(struct platform_device *dev)
+static int __devexit timbuart_remove(struct platform_device *dev)
 {
 	struct timbuart_port *uart = platform_get_drvdata(dev);

 	tasklet_kill(&uart->tasklet);
-	uart_remove_one_port(&timbuart_driver, &uart->port);
-	uart_unregister_driver(&timbuart_driver);
+	uart_remove_one_port(&uart->uart_driver, &uart->port);
+	uart_unregister_driver(&uart->uart_driver);
 	kfree(uart);

 	return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] timbuart: Support for beeing probed more than once

[Andrew Morton]

On Tue, 22 Sep 2009 14:25:31 +0200
Richard R__jfors <richard.rojfors@mocean-labs.com> wrote:

> There was a problem in the current implementation where a global static
> uart_driver struct was used. The same struct was reused every time the
> driver got probed. Since the struct has a state within the serial core
> it can not be reused.
> 
> A uart_driver struct is added to the timbuart_port struct which is
> allocated per platform device.
> 
> The probe and remove functions are declared __devinit and __devexit.

Are you sure?  Lots of other serial drivers appears to do it this way
and I'm ununaware of it causing any problems there.

Exactly what problem(s) are you observing?  (This should have been
described in the original changelog btw).

Re: [PATCH RESEND] timbuart: Support for beeing probed more than once

[Richard Röjfors]

Andrew Morton wrote:
> On Tue, 22 Sep 2009 14:25:31 +0200
> Richard R__jfors <richard.rojfors@mocean-labs.com> wrote:
> 
>> There was a problem in the current implementation where a global static
>> uart_driver struct was used. The same struct was reused every time the
>> driver got probed. Since the struct has a state within the serial core
>> it can not be reused.
>>
>> A uart_driver struct is added to the timbuart_port struct which is
>> allocated per platform device.
>>
>> The probe and remove functions are declared __devinit and __devexit.
> 
> Are you sure?  Lots of other serial drivers appears to do it this way
> and I'm ununaware of it causing any problems there.

I suppose it's not common that UART hardware comes and goes, or having several underlying devices
for the same driver.

> Exactly what problem(s) are you observing?  (This should have been
> described in the original changelog btw).

This is what happens on 2.6.32-rc1:

1. The underlaying platform device shows up -> timbuart is probed

2. The underlaying platform device disappears -> timbuart is removed

3. The underlaying platform device shows up:
[  369.722127] ------------[ cut here ]------------
[  369.722135] kernel BUG at drivers/serial/serial_core.c:2347!
[  369.722141] invalid opcode: 0000 [#1] PREEMPT SMP
[  369.722150] last sysfs file: /sys/module/mfd_core/initstate
[  369.722156] Modules linked in: timberdale(+) timbuart max7301 radio_timb joydev ks8842 adv7180
saa7706h tef6862 tsc2007 xilinx_spi_pltfm xilinx_spi i2c_xiic asix timbmlb spi_bitbang most
timblogiw timbdma snd_timbi2s usbnet mfd_core [last unloaded: timberdale]
[  369.722204]
[  369.722212] Pid: 2777, comm: modprobe Not tainted (2.6.32-rc2-ivi #2)
[  369.722219] EIP: 0060:[<c12389e8>] EFLAGS: 00010286 CPU: 0
[  369.722233] EIP is at uart_register_driver+0x12/0x142
[  369.722240] EAX: f8272968 EBX: f75ba080 ECX: ffffffff EDX: 00ca1000
[  369.722246] ESI: fffffff4 EDI: f6e970c0 EBP: f6b6bd70 ESP: f6b6bd5c
[  369.722253]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[  369.722261] Process modprobe (pid: 2777, ti=f6b6a000 task=f6b02ff0 task.ti=f6b6a000)
[  369.722266] Stack:
[  369.722270]  f8272968 f6b6bd70 f75ba080 fffffff4 f6e970c0 f6b6bd8c f8272533 f8272887
[  369.722287] <0> f6e970c8 f6e970c8 ffffffed f8272934 f6b6bd94 c12400e2 f6b6bda8 c123f43c
[  369.722294] <0> f8272934 f6e970c8 c16b9844 f6b6bdb8 c123f54f f6b6bdc4 00000000 f6b6bdd8
[  369.722294] Call Trace:
[  369.722294]  [<f8272533>] ? timbuart_probe+0x12d/0x185 [timbuart]
[  369.722294]  [<c12400e2>] ? platform_drv_probe+0xc/0xe
[  369.722294]  [<c123f43c>] ? driver_probe_device+0x79/0x105
[  369.722294]  [<c123f54f>] ? __device_attach+0x28/0x30
[  369.722294]  [<c123eba1>] ? bus_for_each_drv+0x3d/0x67
[  369.722294]  [<c123f5ba>] ? device_attach+0x44/0x58
[  369.722294]  [<c123f527>] ? __device_attach+0x0/0x30
[  369.722294]  [<c123ea37>] ? bus_probe_device+0x1f/0x34
[  369.722294]  [<c123d8dd>] ? device_add+0x313/0x44e
[  369.722294]  [<c14893b7>] ? _write_unlock+0x8/0x1f
[  369.722294]  [<c1240643>] ? platform_device_add+0xd9/0x11c
[  369.722294]  [<f81fe18d>] ? mfd_add_devices+0x16d/0x1c4 [mfd_core]
[  369.722294]  [<f826e3c6>] ? timb_probe+0x313/0x43c [timberdale]
[  369.722294]  [<c119f6b4>] ? local_pci_probe+0xe/0x10
[  369.722294]  [<c11a00b5>] ? pci_device_probe+0x43/0x66
[  369.722294]  [<c123f43c>] ? driver_probe_device+0x79/0x105
[  369.722294]  [<c123f50b>] ? __driver_attach+0x43/0x5f
[  369.722294]  [<c123eddf>] ? bus_for_each_dev+0x3d/0x67
[  369.722294]  [<c123f313>] ? driver_attach+0x14/0x16
[  369.722294]  [<c123f4c8>] ? __driver_attach+0x0/0x5f
[  369.722294]  [<c123e866>] ? bus_add_driver+0xf9/0x223
[  369.722294]  [<c123f74f>] ? driver_register+0x8b/0xeb
[  369.722294]  [<c11a0279>] ? __pci_register_driver+0x43/0x9c
[  369.722294]  [<c104cbbf>] ? __blocking_notifier_call_chain+0x40/0x4c
[  369.722294]  [<f8275000>] ? timberdale_init+0x0/0x48 [timberdale]
[  369.722294]  [<f8275017>] ? timberdale_init+0x17/0x48 [timberdale]
[  369.722294]  [<c1001139>] ? do_one_initcall+0x4c/0x131
[  369.722294]  [<c105d0c8>] ? sys_init_module+0xa7/0x1db
[  369.722294]  [<c1002aa1>] ? syscall_call+0x7/0xb
[  369.722294] Code: 20 6a 00 e8 fe c6 de ff 5b 8b 45 e8 e8 51 f8 24 00 8d 65 f4 5b 5e 5f 5d c3 55
89 e5 57 56 53 83 ec 08 89 45 ec 83 78 1c 00 74 04 <0f> 0b eb fe 8b 55 ec 31 db 69 42 14 c4 00 00 00
ba d0 80 00 00
[  369.722294] EIP: [<c12389e8>] uart_register_driver+0x12/0x142 SS:ESP 0068:f6b6bd5c
[  369.722758] ---[ end trace ad0b6892a395c249 ]---


The reason is as you see line 2347 of serial_core:
int uart_register_driver(struct uart_driver *drv)
{
	struct tty_driver *normal = NULL;
	int i, retval;

	BUG_ON(drv->state);

The drv->state is not 0, since the "old" struct is reused. The same would happen if the same static
uart_driver struct is used for several devices.

We could set state to NULL, since it was actually deallocated when the driver was unregistered (step
2 above). That would be fine for my case where I only have one platform device which comes and goes.

But the case where we have two platform devices in parallell would break, because the uart_driver
struct is used internally in serial_core. Setting state to NULL when probing the second would make
the state disappear for the first device.

Row 2375 of serial_core:
	normal->driver_state    = drv;

My conclusion is, we need a uart_driver struct per actual hardware device.

--Richard