serial: imx: fixes
Subsystems (2)
cover serial: imx: fixes
From: Dean Jenkins <[email protected]>
The following set of patches were tested on an i.MX6 multi-core platform.
Patches 1 to 4 were tested using a Bluetooth BCSP UART connection at 115.2kbps and 4Mbps. The fixes were needed because the i.MX6 UART driver caused spinlock deadlocks with the HCI TTY layer.
Patch 5 improves the i.MX6 UART driver for use with kdb.
Patch 1: serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx() Patch 2: serial: imx: remove uart_write_wakeup() from imx_transmit_buffer() Patch 3: serial: imx: avoid spinlock recursion deadlock
Patch 4: serial: imx: move imx_transmit_buffer() into imx_txint() Patch 5: serial: imx: clean up imx_poll_get_char()
The patches are based off 3.15-rc4.
----------------------------------------------------------------
Andy Lowe (1):
serial: imx: avoid spinlock recursion deadlock
Dirk Behme (4):
serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
serial: imx: remove uart_write_wakeup() from imx_transmit_buffer()
serial: imx: move imx_transmit_buffer() into imx_txint()
serial: imx: clean up imx_poll_get_char()
drivers/tty/serial/imx.c | 73 ++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 51 deletions(-)
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
From: Dirk Behme <[email protected]>
Use imx_start_tx() just to enable the TX interrupt. It's the job of the TX interrupt ISR to fill the transmit buffer, then. If the transmit buffer is empty, the TX interrupt should be executed as soon as the start_tx() enables the interrupt, so there is no reason for the extra imx_transmit_buffer() call, here. Remove it.
--- drivers/tty/serial/imx.c | 3 --- 1 file changed, 3 deletions(-)
drivers/tty/serial/imx.c +0 -3
@@ -600,9 +600,6 @@ static void imx_start_tx(struct uart_port *port)imx_dma_tx (sport);return ; } - -if (readl (sport->port.membase +uts_reg (sport)) &UTS_TXEMPTY ) -imx_transmit_buffer (sport); }static irqreturn_t imx_rtsint (int irq,void *dev_id)
Signature · 1 line
1.7.9.5
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
Quoted 5 lines · 于 2014年05月09日 23:19, [email protected] 写道:
Use
imx_start_tx()just to enable the TX interrupt. It's the job of the TX interrupt ISR to fill the transmit buffer, then. If the transmit buffer is empty, the TX interrupt should be executed as soon as thestart_tx()enables the interrupt, so there is no reason for the extraimx_transmit_buffer()call, here. Remove it.
I don't know why this patch needed?
What problem this patch fix or improve?
thanks
Huang Shijie
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
Quoted 8 lines · Huang Shijie
? 2014?05?09? 23:19,
dean_jenkinsat mentor.com ??:Quoted 5 lines
Use
imx_start_tx()just to enable the TX interrupt. It's the job of the TX interrupt ISR to fill the transmit buffer, then. If the transmit buffer is empty, the TX interrupt should be executed as soon as thestart_tx()enables the interrupt, so there is no reason for the extraimx_transmit_buffer()call, here. Remove it.I don't know why this patch needed?
What problem this patch fix or improve?
As stated in the commit message, the call of imx_transmit_buffer() isn't needed there, so it can be removed. I.e. remove unneeded code.
In the end, this cleans up the possible locking path, so that in the third patch the locking issue can be easily fixed.
Best regards
Dirk
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
Quoted 4 lines · Dirk Behme
Quoted 3 lines · Huang Shijie
Quoted 2 lines · 于 2014年05月09日 23:19, [email protected] 写道:
Use
imx_start_tx()just to enable the TX interrupt. It's the job of the TX interrupt ISR to fill the transmit buffer, then. If the transmit buffer
From the Documentation/serial/driver, we can see:
-----------------------------------------
start_tx(port)
Start transmitting characters.
-----------------------------------------
It tells us we can transmit data in the imx_start_tx.
But this patch moves it to the interrupt handler, this patch makes the interrupt handler do more jobs.
Quoted 11 lines
Quoted 5 lines
Quoted 3 lines
is empty, the TX interrupt should be executed as soon as the
start_tx()enables the interrupt, so there is no reason for the extraimx_transmit_buffer()call, here. Remove it.I don't know why this patch needed?
What problem this patch fix or improve?As stated in the commit message, the call of
imx_transmit_buffer()isn't needed there, so it can be removed. I.e. remove unneeded code.In the end, this cleans up the possible locking path, so that in the third patch the locking issue can be easily fixed.
The lock issue had been fixed already. Please try the latest linux-next. see my comment in the third patch.
thanks
Huang Shijie
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
Quoted 6 lines · Huang Shijie
? 2014?05?12? 13:45, Dirk Behme ??:
Quoted 5 lines · Huang Shijie
Quoted 4 lines
? 2014?05?09? 23:19,
dean_jenkinsat mentor.com ??:Quoted 3 lines
Use
imx_start_tx()just to enable the TX interrupt. It's the job of the TX interrupt ISR to fill the transmit buffer, then. If the transmit buffer
Quoted 7 lines
From the
Documentation/serial/driver, we can see:----------------------------------------- start_tx(port) Start transmitting characters. -----------------------------------------It tells us we can transmit data in the
imx_start_tx.
Well, it depends how you read 'Start transmitting', no?
It doesn't have to mean 'actually transmit data'. It talks about 'start'. And this could also mean 'start the transmission (by enabling the interrupt)'.
Quoted 1 line
But this patch moves it to the interrupt handler,
It doesn't 'move' any code to the interrupt handler. The code in the interrupt handler is already there.
Quoted 2 lines
this patch makes the
interrupt handler do more jobs.
... to get the locking in the correct order.
Best regards
Dirk
Quoted 15 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
is empty, the TX interrupt should be executed as soon as the
start_tx()enables the interrupt, so there is no reason for the extraimx_transmit_buffer()call, here. Remove it.I don't know why this patch needed?
What problem this patch fix or improve?Quoted 7 lines
As stated in the commit message, the call of
imx_transmit_buffer()isn't needed there, so it can be removed. I.e. remove unneeded code.In the end, this cleans up the possible locking path, so that in the third patch the locking issue can be easily fixed.
The lock issue had been fixed already. Please try the latest linux-next. see my comment in the third patch.
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
? 2014?05?12? 14:40, Dirk Behme ??:
Quoted 14 lines · Huang Shijie
Quoted 13 lines
? 2014?05?12? 13:45, Dirk Behme ??:
Quoted 6 lines · Huang Shijie
Quoted 5 lines
? 2014?05?09? 23:19,
dean_jenkinsat mentor.com ??:Quoted 4 lines
Use
imx_start_tx()just to enable the TX interrupt. It's the job of the
TX interrupt ISR to fill the transmit buffer, then. If the transmit bufferFrom the
Documentation/serial/driver, we can see:----------------------------------------- start_tx(port) Start transmitting characters. -----------------------------------------
Quoted 8 lines
Quoted 1 line
It tells us we can transmit data in the
imx_start_tx.Well, it depends how you read 'Start transmitting', no?
It doesn't have to mean 'actually transmit data'. It talks about 'start'. And this could also mean 'start the transmission (by enabling the interrupt)'.
Ok :)
I do not object this patch.
My opinion is : If the third patch is redundant, could this patch still needed?
Quoted 5 lines
Quoted 1 line
But this patch moves it to the interrupt handler,
It doesn't 'move' any code to the interrupt handler. The code in the interrupt handler is already there.
i knew it.
Quoted 5 lines
Quoted 2 lines
this patch makes the
interrupt handler do more jobs.... to get the locking in the correct order.
again, the third patch can be ignored.
The locking is correct for the imx.c.
thanks
Huang Shijie
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
Quoted 37 lines · Huang Shijie
? 2014?05?12? 14:40, Dirk Behme ??:
Quoted 14 lines · Huang Shijie
Quoted 13 lines
? 2014?05?12? 13:45, Dirk Behme ??:
Quoted 6 lines · Huang Shijie
Quoted 5 lines
? 2014?05?09? 23:19,
dean_jenkinsat mentor.com ??:Quoted 4 lines
Use
imx_start_tx()just to enable the TX interrupt. It's the job of the
TX interrupt ISR to fill the transmit buffer, then. If the transmit bufferFrom the
Documentation/serial/driver, we can see:----------------------------------------- start_tx(port) Start transmitting characters. -----------------------------------------Quoted 8 lines
Quoted 1 line
It tells us we can transmit data in the
imx_start_tx.Well, it depends how you read 'Start transmitting', no?
It doesn't have to mean 'actually transmit data'. It talks about 'start'. And this could also mean 'start the transmission (by enabling the interrupt)'.
Ok :)
I do not object this patch.My opinion is : If the third patch is redundant, could this patch still needed?
Quoted 5 lines
Quoted 1 line
But this patch moves it to the interrupt handler,
It doesn't 'move' any code to the interrupt handler. The code in the interrupt handler is already there.
i knew it.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
this patch makes the
interrupt handler do more jobs.... to get the locking in the correct order.
again, the third patch can be ignored.
The locking is correct for theimx.c.
Well, the locking might now be OK but due to an existing issue the TX will be woken up excessively with no new characters to transmit so there will be excessive context switching with the new work queue that results in no useful work done. There is an API issue in HCI LDISC because TTY_DO_WRITE_WAKEUP is set BEFORE checking whether any characters remain pending to be sent. This means most of the time hci_uart_tx_wakeup() is unnecessarily called will no chance of seeing any remain pending characters to be process. This weakness contributed to triggering lockup issue.
In my opinion, TTY_DO_WRITE_WAKEUP should only be set when the UART character circular holding buffer becomes full which for BCSP traffic is unlikely I think (4kB buffer versus traffic < 1kB BCSP frames in length). The current API needs fixing so that TTY_DO_WRITE_WAKEUP is set after filling up the holding buffer but BEFORE enabling the TX interrupts. I think TTY_DO_WRITE_WAKEUP needs to be set in a callback function called from the bound layer below HCI LDISC.
Best regards,
Dean
PATCH 1/5 serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()
? 2014?05?12? 16:03, Dean Jenkins ??:
Quoted 10 lines
Well, the locking might now be OK but due to an existing issue the TX will be woken up excessively with no new characters to transmit so there will be excessive context switching with the new work queue that results in no useful work done. There is an API issue in HCI LDISC because
TTY_DO_WRITE_WAKEUPis set BEFORE checking whether any characters remain pending to be sent. This means most of the timehci_uart_tx_wakeup()is unnecessarily called will no chance of seeing any remain pending characters to be process. This weakness contributed to triggering lockup issue.
You can send a patch to fix the Bluetooth code. :) I am happy if you can improve the Bluetooth.
thanks
Huang Shijie
PATCH 2/5 serial: imx: remove uart_write_wakeup() from imx_transmit_buffer()
In reply to dean_jenkins at mentor.comFrom: Dirk Behme <[email protected]>
At the moment we have
imx_transmit_buffer() {
...
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup();
...
}imx_txint() {
...
imx_transmit_buffer();
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup();
...
}With this, we are calling uart_write_wakeup() at the end of imx_transmit_buffer() and immediately again after returning from imx_transmit_buffer() in imx_txint(). Instead of calling uart_write_wakeup() two times this way, remove the first call and call uart_write_wakeup() only once in imx_txint().
--- drivers/tty/serial/imx.c | 3 --- 1 file changed, 3 deletions(-)
drivers/tty/serial/imx.c +0 -3
@@ -470,9 +470,6 @@ static inline void imx_transmit_buffer(struct imx_port *sport) sport->port.icount.tx++; } -if (uart_circ_chars_pending (xmit) <WAKEUP_CHARS ) -uart_write_wakeup (&sport->port); -if (uart_circ_empty (xmit))imx_stop_tx (&sport->port); }
Signature · 1 line
1.7.9.5
PATCH 3/5 serial: imx: avoid spinlock recursion deadlock
In reply to dean_jenkins at mentor.comFrom: Andy Lowe <[email protected]>
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.
--- drivers/tty/serial/imx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/tty/serial/imx.c +6 -1
@@ -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);
Signature · 1 line
1.7.9.5
PATCH 3/5 serial: imx: avoid spinlock recursion deadlock
? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
Quoted 48 lines
From: Andy Lowe<[email protected]>
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 ofimx_txint()after theuart_portspinlock is unlocked.Signed-off-by Andy Lowe<[email protected]>Signed-off-by Dirk Behme<[email protected]>--- drivers/tty/serial/imx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/tty/serial/imx.c +0
-1@@ -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 )
drivers/tty/serial/imx.c static irqreturn_t imx_txint(int irq, void *dev_id) · line 648
Quoted 10 lines
drivers/tty/serial/imx.c +6
-0+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
has fixed this deadlock.
We can ignore this patch now.
thanks
Huang Shijie
PATCH 3/5 serial: imx: avoid spinlock recursion deadlock
Quoted 57 lines · Huang Shijie
? 2014?05?09? 23:19,
dean_jenkinsat mentor.com ??:Quoted 56 lines
From: Andy Lowe<[email protected]>
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 ofimx_txint()after theuart_portspinlock is unlocked.Signed-off-by Andy Lowe<[email protected]>Signed-off-by Dirk Behme<[email protected]>--- drivers/tty/serial/imx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/tty/serial/imx.c +6
-1@@ -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 ;
Quoted 5 lines
drivers/tty/serial/imx.c static irqreturn_t imx_txint(int irq, void *dev_id) · line 648
Quoted 2 lines
drivers/tty/serial/imx.c +0
-0out:spin_unlock_irqrestore (&sport->port.lock, flags);I think this patch :
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.
Quoted 3 lines
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.
Quoted 2 lines
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
PATCH 4/5 serial: imx: move imx_transmit_buffer() into imx_txint()
In reply to dean_jenkins at mentor.comFrom: Dirk Behme <[email protected]>
Move the code block of imx_transmit_buffer() into imx_txint() because imx_transmit_buffer() is only called from a single place now. In other words, having the inline function imx_transmit_buffer() for a single call is unnecessary and code refactoring to eliminate the imx_transmit_buffer() function call makes the code easier to read.
No functional change.
--- drivers/tty/serial/imx.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)
drivers/tty/serial/imx.c +12 -19
@@ -456,24 +456,6 @@ static void imx_enable_ms(struct uart_port *port)mod_timer (&sport->timer, jiffies); } -static inline void imx_transmit_buffer (struct imx_port *sport) -{ -struct circ_buf *xmit = &sport->port.state->xmit; - -while (!uart_circ_empty (xmit) && - !(readl (sport->port.membase +uts_reg (sport)) - &UTS_TXFULL )) { -/* send xmit->buf[xmit->tail] -* out the port here */ -writel (xmit->buf[xmit->tail], sport->port.membase +URTX0 ); - xmit->tail = (xmit->tail +1 ) & (UART_XMIT_SIZE -1 ); - sport->port.icount.tx++; - } - -if (uart_circ_empty (xmit)) -imx_stop_tx (&sport->port); -} -static void dma_tx_callback (void *data) {struct imx_port *sport = data;
@@ -634,7 +616,18 @@ static irqreturn_t imx_txint(int irq, void *dev_id) goto out; } -imx_transmit_buffer (sport); +while (!uart_circ_empty (xmit) && + !(readl (sport->port.membase +uts_reg (sport)) + &UTS_TXFULL )) { +/* send xmit->buf[xmit->tail] +* out the port here */ +writel (xmit->buf[xmit->tail], sport->port.membase +URTX0 ); + xmit->tail = (xmit->tail +1 ) & (UART_XMIT_SIZE -1 ); + sport->port.icount.tx++; + } + +if (uart_circ_empty (xmit)) +imx_stop_tx (&sport->port);if (uart_circ_chars_pending (xmit) <WAKEUP_CHARS ) {spin_unlock_irqrestore (&sport->port.lock, flags);
Signature · 1 line
1.7.9.5
PATCH 5/5 serial: imx: clean up imx_poll_get_char()
In reply to dean_jenkins at mentor.comFrom: Dirk Behme <[email protected]>
Looking at the get_poll_char() function of the 8250.c serial driver, we learn:
* poll_get_char() doesn't have to save/disable/restore the interrupt registers. No interrupt handling is needed in this function at all. Remove it.
* Don't block in case there is no data available. So instead blocking in the do {} while loop, just return with NO_POLL_CHAR, immediately .
Additionally, while the i.MX6 register URXD[7-0] contain the RX_DATA, the upper bits of this register (URXD[15-10]) might contain some control flags. To ensure that these are not returned with the data read, just mask out URXD[7-0].
These changes fix the 'hang' working with kdb:
$ echo ttymxc3 > /sys/module/kgdboc/parameters/kgdboc $ echo g >/proc/sysrq-trigger [0]kdb> help ... <hang>
--- drivers/tty/serial/imx.c | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-)
drivers/tty/serial/imx.c +4 -25
@@ -80,6 +80,7 @@#define URXD_FRMERR (1<<12)#define URXD_BRK (1<<11)#define URXD_PRERR (1<<10) +#define URXD_RX_DATA (0xFF<<0)#define UCR1_ADEN (1<<15)/* Auto detect interrupt */ #define UCR1_ADBR (1<<14)/* Auto detect baud rate */ #define UCR1_TRDYEN (1<<13)/* Transmitter ready interrupt enable */
@@ -1502,32 +1503,10 @@ imx_verify_port(struct uart_port *port, struct serial_struct *ser)#if defined(CONFIG_CONSOLE_POLL )static int imx_poll_get_char (struct uart_port *port) { -struct imx_port_ucrs old_ucr; -unsigned int status; -unsigned char c; - -/* save control registers */ -imx_port_ucrs_save (port, &old_ucr); - -/* disable interrupts */ -writel (UCR1_UARTEN , port->membase +UCR1 ); -writel (old_ucr.ucr2 & ~(UCR2_ATEN |UCR2_RTSEN |UCR2_ESCI ), - port->membase +UCR2 ); -writel (old_ucr.ucr3 & ~(UCR3_DCD |UCR3_RI |UCR3_DTREN ), - port->membase +UCR3 ); - -/* poll */ -do { - status =readl (port->membase +USR2 ); - }while (~status &USR2_RDR ); - -/* read */ - c =readl (port->membase +URXD0 ); - -/* restore control registers */ -imx_port_ucrs_restore (port, &old_ucr); +if (!(readl (port->membase +USR2 ) &USR2_RDR )) +return NO_POLL_CHAR ; -return c; +return readl (port->membase +URXD0 ) &URXD_RX_DATA ; }static void imx_poll_put_char (struct uart_port *port,unsigned char c)
Signature · 1 line
1.7.9.5
Greg KH
maintainercover serial: imx: fixes
In reply to dean_jenkins at mentor.comQuoted 22 lines · On Fri, May 09, 2014 at 04:19:43PM +0100, [email protected] wrote:
From: Dean Jenkins <[email protected]>
The following set of patches were tested on an i.MX6 multi-core platform.
Patches 1 to 4 were tested using a Bluetooth BCSP UART connection at 115.2kbps and 4Mbps. The fixes were needed because the i.MX6 UART driver caused spinlock deadlocks with the HCI TTY layer.
Patch 5 improves the i.MX6 UART driver for use with kdb.
Patch 1: serial: imx: remove unneeded
imx_transmit_buffer()fromimx_start_tx()Patch 2: serial: imx: removeuart_write_wakeup()fromimx_transmit_buffer()Patch 3: serial: imx: avoid spinlock recursion deadlock
Patch 4: serial: imx: moveimx_transmit_buffer()intoimx_txint()Patch 5: serial: imx: clean upimx_poll_get_char()The patches are based off 3.15-rc4.
----------------------------------------------------------------
Andy Lowe (1):
serial: imx: avoid spinlock recursion deadlock
Quoted 8 lines
Dirk Behme (4):
serial: imx: remove unneededimx_transmit_buffer()fromimx_start_tx()
serial: imx: removeuart_write_wakeup()fromimx_transmit_buffer()
serial: imx: moveimx_transmit_buffer()intoimx_txint()
serial: imx: clean upimx_poll_get_char()drivers/tty/serial/imx.c | 73 ++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 51 deletions(-)
Given the rather confusing threads based on these patches, I don't know what to apply.
Dean, can you respin what patches you want applied and resend them so I'm not so confused?
thanks,
greg k-h
cover serial: imx: fixes
Am 28.05.2014 21:34, schrieb Greg KH:
Quoted 29 lines · dean_jenkins at mentor.com
Quoted 28 lines
From: Dean Jenkins <[email protected]>
The following set of patches were tested on an i.MX6 multi-core platform.
Patches 1 to 4 were tested using a Bluetooth BCSP UART connection at 115.2kbps and 4Mbps. The fixes were needed because the i.MX6 UART driver caused spinlock deadlocks with the HCI TTY layer.
Patch 5 improves the i.MX6 UART driver for use with kdb.
Patch 1: serial: imx: remove unneeded
imx_transmit_buffer()fromimx_start_tx()Patch 2: serial: imx: removeuart_write_wakeup()fromimx_transmit_buffer()Patch 3: serial: imx: avoid spinlock recursion deadlock
Patch 4: serial: imx: moveimx_transmit_buffer()intoimx_txint()Patch 5: serial: imx: clean upimx_poll_get_char()The patches are based off 3.15-rc4.
----------------------------------------------------------------
Andy Lowe (1):
serial: imx: avoid spinlock recursion deadlockDirk Behme (4):
serial: imx: remove unneededimx_transmit_buffer()fromimx_start_tx()
serial: imx: removeuart_write_wakeup()fromimx_transmit_buffer()
serial: imx: moveimx_transmit_buffer()intoimx_txint()
serial: imx: clean upimx_poll_get_char()
Quoted 7 lines
Quoted 2 lines
drivers/tty/serial/imx.c | 73 ++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 51 deletions(-)Given the rather confusing threads based on these patches, I don't know what to apply.
Dean, can you respin what patches you want applied
Well, we think patches #1-#4 are correct and should be applied, but Huang has the opinion that this is fixed already in the BT stack. If Huang doesn't change his mind, then let's drop #1-#4.
Patch #5 addresses an other topic and therefore should be resent, then.
Best regards
Dirk
cover serial: imx: fixes
Quoted 33 lines · Dirk Behme
Am 28.05.2014 21:34, schrieb Greg KH:
Quoted 32 lines · dean_jenkins at mentor.com
Quoted 30 lines
From: Dean Jenkins <[email protected]>
The following set of patches were tested on an i.MX6 multi-core platform.
Patches 1 to 4 were tested using a Bluetooth BCSP UART connection at 115.2kbps and 4Mbps. The fixes were needed because the i.MX6 UART driver caused spinlock deadlocks with the HCI TTY layer.
Patch 5 improves the i.MX6 UART driver for use with kdb.
Patch 1: serial: imx: remove unneeded
imx_transmit_buffer()fromimx_start_tx()Patch 2: serial: imx: removeuart_write_wakeup()fromimx_transmit_buffer()Patch 3: serial: imx: avoid spinlock recursion deadlock
Patch 4: serial: imx: moveimx_transmit_buffer()intoimx_txint()Patch 5: serial: imx: clean upimx_poll_get_char()The patches are based off 3.15-rc4.
----------------------------------------------------------------
Andy Lowe (1):
serial: imx: avoid spinlock recursion deadlockDirk Behme (4):
serial: imx: remove unneededimx_transmit_buffer()fromimx_start_tx()
serial: imx: removeuart_write_wakeup()fromimx_transmit_buffer()
serial: imx: moveimx_transmit_buffer()intoimx_txint()
serial: imx: clean upimx_poll_get_char()drivers/tty/serial/imx.c | 73 ++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 51 deletions(-)
Quoted 8 lines
Quoted 4 lines
Given the rather confusing threads based on these patches, I don't know what to apply.
Dean, can you respin what patches you want applied
Well, we think patches #1-#4 are correct and should be applied, but Huang has the opinion that this is fixed already in the BT stack. If Huang doesn't change his mind, then let's drop #1-#4.
Yes, the BT stack has fixed it. :)
we can drop the #1-#4.
thanks
Huang Shijie