* [PATCH 1/4] tty/serial: at91: Handle shutdown more safely
[not found] <cover.1389090437.git.nicolas.ferre@atmel.com>
@ 2014-01-07 10:45 ` Nicolas Ferre
2014-01-07 10:45 ` [PATCH 2/4] tty/serial: at91: fix race condition in atmel_serial_remove Nicolas Ferre
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Nicolas Ferre @ 2014-01-07 10:45 UTC (permalink / raw)
To: gregkh
Cc: Leilei Zhao, mark.roszko, mdeneen, linux-arm-kernel, linux-kernel,
linux-serial, stable, Nicolas Ferre
From: Marek Roszko <mark.roszko@gmail.com>
Interrupts were being cleaned up late in the shutdown handler, it is possible
that an interrupt can occur and schedule a tasklet that runs after the port is
cleaned up. There is a null dereference due to this race condition with the
following stacktrace:
[<c02092b0>] (atmel_tasklet_func+0x514/0x814) from [<c001fd34>] (tasklet_action+0x70/0xa8)
[<c001fd34>] (tasklet_action+0x70/0xa8) from [<c001f60c>] (__do_softirq+0x90/0x144)
[<c001f60c>] (__do_softirq+0x90/0x144) from [<c001fa18>] (irq_exit+0x40/0x4c)
[<c001fa18>] (irq_exit+0x40/0x4c) from [<c000e298>] (handle_IRQ+0x64/0x84)
[<c000e298>] (handle_IRQ+0x64/0x84) from [<c000d6c0>] (__irq_svc+0x40/0x50)
[<c000d6c0>] (__irq_svc+0x40/0x50) from [<c0208060>] (atmel_rx_dma_release+0x88/0xb8)
[<c0208060>] (atmel_rx_dma_release+0x88/0xb8) from [<c0209740>] (atmel_shutdown+0x104/0x160)
[<c0209740>] (atmel_shutdown+0x104/0x160) from [<c0205e8c>] (uart_port_shutdown+0x2c/0x38)
Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
drivers/tty/serial/atmel_serial.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index c7d99af46a96..48ea47a32d5f 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1650,12 +1650,24 @@ static int atmel_startup(struct uart_port *port)
static void atmel_shutdown(struct uart_port *port)
{
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+
/*
- * Ensure everything is stopped.
+ * Clear out any scheduled tasklets before
+ * we destroy the buffers
+ */
+ tasklet_kill(&atmel_port->tasklet);
+
+ /*
+ * Ensure everything is stopped and
+ * disable all interrupts, port and break condition.
*/
atmel_stop_rx(port);
atmel_stop_tx(port);
+ UART_PUT_CR(port, ATMEL_US_RSTSTA);
+ UART_PUT_IDR(port, -1);
+
+
/*
* Shut-down the DMA.
*/
@@ -1665,12 +1677,6 @@ static void atmel_shutdown(struct uart_port *port)
atmel_port->release_tx(port);
/*
- * Disable all interrupts, port and break condition.
- */
- UART_PUT_CR(port, ATMEL_US_RSTSTA);
- UART_PUT_IDR(port, -1);
-
- /*
* Free the interrupt
*/
free_irq(port->irq, port);
--
1.8.2.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/4] tty/serial: at91: fix race condition in atmel_serial_remove
[not found] <cover.1389090437.git.nicolas.ferre@atmel.com>
2014-01-07 10:45 ` [PATCH 1/4] tty/serial: at91: Handle shutdown more safely Nicolas Ferre
@ 2014-01-07 10:45 ` Nicolas Ferre
2014-01-07 10:45 ` [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function Nicolas Ferre
2014-01-07 10:45 ` [PATCH 4/4] tty/serial: at91: reset rx_ring when port is shutdown Nicolas Ferre
3 siblings, 0 replies; 6+ messages in thread
From: Nicolas Ferre @ 2014-01-07 10:45 UTC (permalink / raw)
To: gregkh
Cc: Leilei Zhao, mark.roszko, mdeneen, linux-arm-kernel, linux-kernel,
linux-serial, stable, Nicolas Ferre
From: Marek Roszko <mark.roszko@gmail.com>
The _remove callback could be called when a tasklet is scheduled. tasklet_kill
was called inside the function in order to free up any scheduled tasklets.
However it was called after uart_remove_one_port which destroys tty references
needed in the port for atmel_tasklet_func.
Simply putting the tasklet_kill at the start of the function will prevent this
conflict.
Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
drivers/tty/serial/atmel_serial.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 48ea47a32d5f..c421d11b3d4c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2447,11 +2447,12 @@ static int atmel_serial_remove(struct platform_device *pdev)
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
int ret = 0;
+ tasklet_kill(&atmel_port->tasklet);
+
device_init_wakeup(&pdev->dev, 0);
ret = uart_remove_one_port(&atmel_uart, port);
- tasklet_kill(&atmel_port->tasklet);
kfree(atmel_port->rx_ring.buf);
/* "port" is allocated statically, so we shouldn't free it */
--
1.8.2.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function
[not found] <cover.1389090437.git.nicolas.ferre@atmel.com>
2014-01-07 10:45 ` [PATCH 1/4] tty/serial: at91: Handle shutdown more safely Nicolas Ferre
2014-01-07 10:45 ` [PATCH 2/4] tty/serial: at91: fix race condition in atmel_serial_remove Nicolas Ferre
@ 2014-01-07 10:45 ` Nicolas Ferre
2014-01-08 1:11 ` Greg KH
2014-01-07 10:45 ` [PATCH 4/4] tty/serial: at91: reset rx_ring when port is shutdown Nicolas Ferre
3 siblings, 1 reply; 6+ messages in thread
From: Nicolas Ferre @ 2014-01-07 10:45 UTC (permalink / raw)
To: gregkh
Cc: Leilei Zhao, mark.roszko, mdeneen, linux-arm-kernel, linux-kernel,
linux-serial, stable, Nicolas Ferre
From: Marek Roszko <mark.roszko@gmail.com>
Something asks a tasklet to be scheduled when the uart port is closed.
Need to supress the kernel panic for now by checking if the port is NULL or
not.
Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
drivers/tty/serial/atmel_serial.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index c421d11b3d4c..6e68486c83cb 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1360,6 +1360,10 @@ static void atmel_tasklet_func(unsigned long data)
unsigned int status;
unsigned int status_change;
+ if(!port->state || !port->state->port.tty)
+ /* uart has been closed */
+ return;
+
/* The interrupt handler does not take the lock */
spin_lock(&port->lock);
--
1.8.2.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function
2014-01-07 10:45 ` [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function Nicolas Ferre
@ 2014-01-08 1:11 ` Greg KH
[not found] ` <CAJjB1qLDd6JFb4LwD5iokg5=O8Bwp5MkKsrr45QDodZkZrx75g@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2014-01-08 1:11 UTC (permalink / raw)
To: Nicolas Ferre
Cc: Leilei Zhao, mark.roszko, mdeneen, linux-arm-kernel, linux-kernel,
linux-serial, stable
On Tue, Jan 07, 2014 at 11:45:08AM +0100, Nicolas Ferre wrote:
> From: Marek Roszko <mark.roszko@gmail.com>
>
> Something asks a tasklet to be scheduled when the uart port is closed.
What is that something? Shouldn't you track that down and find the real
problem here?
> Need to supress the kernel panic for now by checking if the port is NULL or
> not.
>
> Signed-off-by: Marek Roszko <mark.roszko@gmail.com>
> Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
> Cc: <stable@vger.kernel.org> # v3.12
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/tty/serial/atmel_serial.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index c421d11b3d4c..6e68486c83cb 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -1360,6 +1360,10 @@ static void atmel_tasklet_func(unsigned long data)
> unsigned int status;
> unsigned int status_change;
>
> + if(!port->state || !port->state->port.tty)
> + /* uart has been closed */
> + return;
Did you really test this?
How about running it through checkpatch?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 4/4] tty/serial: at91: reset rx_ring when port is shutdown
[not found] <cover.1389090437.git.nicolas.ferre@atmel.com>
` (2 preceding siblings ...)
2014-01-07 10:45 ` [PATCH 3/4] tty/serial: at91: prevent null dereference in tasklet function Nicolas Ferre
@ 2014-01-07 10:45 ` Nicolas Ferre
3 siblings, 0 replies; 6+ messages in thread
From: Nicolas Ferre @ 2014-01-07 10:45 UTC (permalink / raw)
To: gregkh
Cc: Leilei Zhao, mark.roszko, mdeneen, linux-arm-kernel, linux-kernel,
linux-serial, stable, Nicolas Ferre
From: Mark Deneen <mdeneen@gmail.com>
When using RX DMA, the driver won't pass any data to the uart layer
until the buffer is flipped. When the port is shutdown, the dma buffers
are unmapped, but the head and tail of the ring buffer are not reseted.
Since the serial console will keep the port open, this will only
present itself when the uart is not shared.
To reproduce the issue, with an unpatched driver, run a getty on /dev/ttyS0
with no serial console and exit. Getty will exit, and when the new one returns
you will be unable to log in. If you hold down a key long enough to fill the
DMA buffer and flip it, you can then log in.
Signed-off-by: Mark Deneen <mdeneen@gmail.com>
Acked-by: Leilei Zhao <leilei.zhao@atmel.com>
[nicolas.ferre@atmel.com: adapt to mainline kernel, handle !DMA case]
Cc: <stable@vger.kernel.org> # v3.12
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
drivers/tty/serial/atmel_serial.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 6e68486c83cb..2d925455c1ec 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1681,6 +1681,12 @@ static void atmel_shutdown(struct uart_port *port)
atmel_port->release_tx(port);
/*
+ * Reset ring buffer pointers
+ */
+ atmel_port->rx_ring.head = 0;
+ atmel_port->rx_ring.tail = 0;
+
+ /*
* Free the interrupt
*/
free_irq(port->irq, port);
--
1.8.2.2
^ permalink raw reply related [flat|nested] 6+ messages in thread