[PATCH] Fix oops in the 8250 serial driver when accessing a removed device

[PATCH] Fix oops in the 8250 serial driver when accessing a removed device

[Laurent Pinchart]

The 8250 serial driver allocates dummy devices for ISA serial ports and lets
userspace applications open them even when they are not present. That hack is
required by setserial to set the ISA serial ports I/O address.

When a plug-and-play 8250 device is detected, the driver reuses one of the
dummy ports. If that device is later removed, the port goes pack to the dummy
ports pool. The capabilities field is not reset, which causes a oops when
trying to access the port.

This patch resets the capabilities field when a 8250 device is unregistered.

Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
---
 drivers/serial/8250.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index ef158c1..40eb91f 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2886,6 +2886,7 @@ void serial8250_unregister_port(int line)
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
 		uart->port.type = PORT_UNKNOWN;
 		uart->port.dev = &serial8250_isa_devs->dev;
+		uart->capabilities = 0;
 		uart_add_one_port(&serial8250_reg, &uart->port);
 	} else {
 		uart->port.dev = NULL;
-- 
1.5.0

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Re: [PATCH] Fix oops in the 8250 serial driver when accessing a removed device

[Alan Cox]

> When a plug-and-play 8250 device is detected, the driver reuses one of the
> dummy ports. If that device is later removed, the port goes pack to the dummy
> ports pool. The capabilities field is not reset, which causes a oops when
> trying to access the port.

Some day we need to allocate new dummy ports so there is always one free.

> This patch resets the capabilities field when a 8250 device is unregistered.

Looks good to me. Would appreciate Russell's view though.

Alan

Re: [PATCH] Fix oops in the 8250 serial driver when accessing a removed device

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

Hi Russell,

On Tuesday 22 July 2008, Alan Cox wrote:
> > When a plug-and-play 8250 device is detected, the driver reuses one of the
> > dummy ports. If that device is later removed, the port goes pack to the
> > dummy ports pool. The capabilities field is not reset, which causes a oops
> > when trying to access the port.
> 
> Some day we need to allocate new dummy ports so there is always one free.
> 
> > This patch resets the capabilities field when a 8250 device is
> > unregistered. 
> 
> Looks good to me. Would appreciate Russell's view though.

Could you please review the patch (available at http://marc.info/?l=linux-serial&m=121674107306179&w=2) ? I'd like it to go into 2.6.27 if possible.

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Fix oops in the 8250 serial driver when accessing a removed device

[Russell King]

On Tue, Jul 22, 2008 at 06:31:50PM +0200, Laurent Pinchart wrote:
> Hi Russell,
> 
> On Tuesday 22 July 2008, Alan Cox wrote:
> > > When a plug-and-play 8250 device is detected, the driver reuses one of the
> > > dummy ports. If that device is later removed, the port goes pack to the
> > > dummy ports pool. The capabilities field is not reset, which causes a oops
> > > when trying to access the port.
> > 
> > Some day we need to allocate new dummy ports so there is always one free.
> > 
> > > This patch resets the capabilities field when a 8250 device is
> > > unregistered. 
> > 
> > Looks good to me. Would appreciate Russell's view though.
> 
> Could you please review the patch (available at http://marc.info/?l=linux-serial&m=121674107306179&w=2) ?
> I'd like it to go into 2.6.27 if possible.

Is there a link to the oops as well?

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

Re: [PATCH] Fix oops in the 8250 serial driver when accessing a removed device

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 3669 bytes --]

On Friday 25 July 2008, Russell King wrote:
> On Tue, Jul 22, 2008 at 06:31:50PM +0200, Laurent Pinchart wrote:
> > Hi Russell,
> > 
> > On Tuesday 22 July 2008, Alan Cox wrote:
> > > > When a plug-and-play 8250 device is detected, the driver reuses one of
> > > > the dummy ports. If that device is later removed, the port goes pack
> > > > to the dummy ports pool. The capabilities field is not reset, which
> > > > causes a oops when trying to access the port.
> > > 
> > > Some day we need to allocate new dummy ports so there is always one
> > > free. 
> > > 
> > > > This patch resets the capabilities field when a 8250 device is
> > > > unregistered. 
> > > 
> > > Looks good to me. Would appreciate Russell's view though.
> > 
> > Could you please review the patch (available at
> > http://marc.info/?l=linux-serial&m=121674107306179&w=2) ? 
> > I'd like it to go into 2.6.27 if possible.
> 
> Is there a link to the oops as well?

I'm afraid not. I had no copy of the oops so I tried to regenerate it. It turned out the bug has been fixed in the meantime.

My patch can probably be dropped. I'm not sure if it's worth bisecting.

On the other hand, I ran into another issue caused by the WARN_ON in uart_flush_buffer@drivers/serial/serial_core.c.

------------[ cut here ]------------
Badness at c0141288 [verbose debug info unavailable]
NIP: c0141288 LR: c013e1f0 CTR: 00000000
REGS: c39f3cc0 TRAP: 0700   Not tainted  (2.6.26-00040-g7f43ec9-dirty)
MSR: 00029032 <EE,ME,IR,DR>  CR: 24242422  XER: 00000000
TASK = c398d900[375] 'hciattach' THREAD: c39f2000
GPR00: 00000000 c39f3d70 c398d900 c39a1c00 00000000 00000001 c39f3d90 00000000
GPR08: c386b000 c023f5b4 00009032 c39a1c00 84044422 1001d9bc 03ff7000 c3fb9000
GPR16: 00000000 00000000 00000000 00000000 c39a1d28 c39a1d20 00000128 00000120
GPR24: c02d0000 00000000 00000000 00000001 00000000 c39f2000 c39a1c00 c39a1c00
NIP [c0141288] uart_flush_buffer+0x2c/0xbc
LR [c013e1f0] tty_driver_flush_buffer+0x34/0x44
Call Trace:
[c39f3d70] [c01370e8] tty_ldisc_flush+0x18/0x5c (unreliable)
[c39f3d80] [c013e1f0] tty_driver_flush_buffer+0x34/0x44
[c39f3d90] [c0183480] hci_uart_flush+0x34/0xb0
[c39f3da0] [c018354c] hci_uart_close+0x50/0x70
[c39f3db0] [c0183704] hci_uart_tty_close+0x38/0xa4
[c39f3dc0] [c0138cdc] release_dev+0x640/0x680
[c39f3e70] [c0138d3c] tty_release+0x20/0x3c
[c39f3e90] [c00724f4] __fput+0x190/0x1b0
[c39f3eb0] [c0070358] filp_close+0x54/0xac
[c39f3ed0] [c001efa8] put_files_struct+0xa0/0xec
[c39f3ef0] [c001f860] do_exit+0x164/0x688
[c39f3f30] [c001fe60] do_group_exit+0xa0/0xdc
[c39f3f40] [c00103d0] ret_from_syscall+0x0/0x38
--- Exception: c01 at 0xff0e91c
    LR = 0xffb4b2c
Instruction dump:
4bffffe4 7c0802a6 9421fff0 93e1000c 7c7f1b78 90010014 81030144 2f880000
419e0010 80080010 2f800000 409e001c <0fe00000> 80010014 83e1000c 38210010

The hciattach process opens the serial port and sets the N_HCI line discipline. At some point the device is disconnected, and uart_remove_one_port is called by serial8250_unregister_port.

A comment in uart_remove_one_port states, right after calling tty_unregister_device,

"All users of this port should now be disconnected from this driver, and the port shut down.  We should be the only thread fiddling with this port from now on."

This seems not to be true, at least with the N_HCI line disclipline. I've tested the N_TTY line discipline by disconnecting the device while performing a 'cat /dev/ttyS0' and no crash occurs.

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Fix oops in the 8250 serial driver when accessing a removed device

[Russell King]

On Fri, Jul 25, 2008 at 01:40:13PM +0200, Laurent Pinchart wrote:
> I'm afraid not. I had no copy of the oops so I tried to regenerate it.
> It turned out the bug has been fixed in the meantime.

Great, maybe someone else fixed it?

> On the other hand, I ran into another issue caused by the WARN_ON in
> uart_flush_buffer@drivers/serial/serial_core.c.

Hardly surprising.  I think you're not the first to report this either,
and I seem to remember logging a complaint with the HCI folk over it.
If it still exists... well...

> NIP [c0141288] uart_flush_buffer+0x2c/0xbc
> LR [c013e1f0] tty_driver_flush_buffer+0x34/0x44
> Call Trace:
> [c39f3d70] [c01370e8] tty_ldisc_flush+0x18/0x5c (unreliable)
> [c39f3d80] [c013e1f0] tty_driver_flush_buffer+0x34/0x44
> [c39f3d90] [c0183480] hci_uart_flush+0x34/0xb0
> [c39f3da0] [c018354c] hci_uart_close+0x50/0x70
> [c39f3db0] [c0183704] hci_uart_tty_close+0x38/0xa4
> [c39f3dc0] [c0138cdc] release_dev+0x640/0x680
> [c39f3e70] [c0138d3c] tty_release+0x20/0x3c
> [c39f3e90] [c00724f4] __fput+0x190/0x1b0
> [c39f3eb0] [c0070358] filp_close+0x54/0xac
> [c39f3ed0] [c001efa8] put_files_struct+0xa0/0xec
> [c39f3ef0] [c001f860] do_exit+0x164/0x688
> [c39f3f30] [c001fe60] do_group_exit+0xa0/0xdc
> [c39f3f40] [c00103d0] ret_from_syscall+0x0/0x38

Basically, what's happened here is that release_dev() has closed down
the serial port, and the serial driver has shut down the hardware and
freed its state.

The next thing that release_dev() then does is shut down the line
discipline, in this case that being HCI.  HCI decides that because
it was running, it wants to flush data.  So it then goes on to call
tty_driver_flush_buffer() on a tty which has already been closed.

This calls into the serial core driver, which quite rightfully complains
that its been called to flush data on an already closed tty.

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