On 12/05/14 04:12, Huang Shijie wrote:
于 2014年05月09日 23:19, dean_jenkins@xxxxxxxxxx 写道:
From: Andy Lowe<andy_lowe@xxxxxxxxxx>
The following deadlock has been observed:
imx_int() {
imx_txint() {
spin_lock_irqsave(&sport->port.lock,flags);
/* ^^^uart_port spinlock taken in imx_txint */
imx_transmit_buffer() {
uart_write_wakeup(&sport->port) {
tty_wakeup() {
hci_uart_tty_wakeup() {
hci_uart_tx_wakeup() {
uart_write() {
spin_lock_irqsave(&port->lock, flags);
/* ^^^deadlock here when spinlock is taken again */
.
.
.
spin_unlock_irqrestore(&port->lock, flags);
}
}
}
}
}
}
spin_unlock_irqrestore(&sport->port.lock,flags);
}
}
To correct this call uart_write_wakeup() at the end of imx_txint() after
the uart_port spinlock is unlocked.
Signed-off-by: Andy Lowe<andy_lowe@xxxxxxxxxx>
Signed-off-by: Dirk Behme<dirk.behme@xxxxxxxxxxxx>
---
drivers/tty/serial/imx.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index abe31ad..cc79706 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -636,8 +636,13 @@ static irqreturn_t imx_txint(int irq, void *dev_id)
imx_transmit_buffer(sport);
- if (uart_circ_chars_pending(xmit)< WAKEUP_CHARS)
+ if (uart_circ_chars_pending(xmit)< WAKEUP_CHARS) {
+ spin_unlock_irqrestore(&sport->port.lock, flags);
uart_write_wakeup(&sport->port);
+ } else
+ spin_unlock_irqrestore(&sport->port.lock, flags);
+
+ return IRQ_HANDLED;
out:
spin_unlock_irqrestore(&sport->port.lock, flags);
I think this patch :
https://lkml.org/lkml/2014/3/20/623
My analysis of this modification in the lkml suggests the following
undesirable side-effects have been introduced:
The addition of the work queue to split the IRQ interrupt context
handling from running hci_uart_tx_wakeup() or new hci_uart_write_work()
"fixes" the i.MX6 serial driver deadlock crash. However, this code is
being scheduled far too often so adds unnecessary processor loading.
There is an underlying flaw in the operation of the TTY_DO_WRITE_WAKEUP
bit which is set too early which causes the wakeup mechanism to trigger
when there are no pending characters to be written to the holding
circular buffer. For BCSP under normal operating conditions, I think the
wakeup mechanism is redundant because the BCSP frames are unable to
completely fill the holding circular buffer so no characters remain
pending. But currently, I think this work queue scheduling will occur
for EVERY transmission of a BCSP frame from the interrupt context and
again from the writing of the BCSP frame into the holding circular
buffer via hci_uart_send_frame(). eg. is scheduled twice per TX BCSP frame.
TTY_DO_WRITE_WAKEUP is tested in drivers/tty/tty_io.c: tty_wakeup() and
therefore if TTY_DO_WRITE_WAKEUP is in the clear state then
ld->ops->write_wakeup(tty) is not called so avoids running
hci_uart_tty_wakeup() so avoids the scheduling of the work queue.
Separate to the deadlock issue, is a contributing issue concerning the
setting of TTY_DO_WRITE_WAKEUP when it is known there are pending
characters to be sent when the holding circular buffer becomes full. The
problematic code is in drivers/bluetooth/hci_ldisc.c :
hci_uart_tx_wakeup() or new hci_uart_write_work() because
TTY_DO_WRITE_WAKEUP is ALWAYS set despite the writing of BCSP frames
usually not filling up the holding circular buffer. I do not see an easy
fix for this because the TTY_DO_WRITE_WAKEUP must be set BEFORE the TX
interrupts are set in the lower bound function tty->ops->write().
Perhaps a callback function pointer is needed that sets
TTY_DO_WRITE_WAKEUP when the write function fails to write all of the
characters into the holding circular buffer ?
An additional side effect of adding the work queue is that BCSP frame
hci_uart_send_frame() calls will also become delayed by the scheduling
and running of the work queue. This is undesirable because it adds
unnecessary processor loading. The work queue should only act on the
interrupt context program flow and not the normal kernel thread flow of
writing BCSP frames. I fear that the work queue is in the wrong place. A
better place would be in hci_uart_tty_wakeup() for the work queue so
that it only effects the interrupt context.
In other words, fixing TTY_DO_WRITE_WAKEUP prevents unnecessary TX
wakeup handling (probably no TX wakeups in BCSP operation) and this
reduces the chances of the original deadlock issue occurring due to the
lower rate of TX wakeup events, if any. The patch fixes the deadlock in
the i.MX6 UART driver without introducing a work-queue in the general code.
has fixed this deadlock.
Well, it has prevented the deadlock but fundamentally it is inefficient
due to increasing latency and processor loading as described above.
We can ignore this patch now.
This patch is compatible with the change in
https://lkml.org/lkml/2014/3/20/623
with the result that the deadlock is prevented in 2 places.
Regards,
Dean
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
