[PATCH 0/5] serial: imx: fixes

[PATCH 0/5] serial: imx: fixes

[dean_jenkins at mentor.com]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

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()

[dean_jenkins at mentor.com]

From: Dirk Behme <dirk.behme@de.bosch.com>

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.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Andy Lowe <andy_lowe@mentor.com>
---
 drivers/tty/serial/imx.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3b6c1a2..7b813b3 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -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)
-- 
1.7.9.5

[PATCH 1/5] serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()

[Huang Shijie]

? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
> 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.
I don't know why this patch needed?
What problem this patch fix or improve?

thanks
Huang Shijie

[PATCH 1/5] serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()

[Dirk Behme]

On 12.05.2014 05:40, Huang Shijie wrote:
> ? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
>> 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.
> 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()

[Huang Shijie]

? 2014?05?12? 13:45, Dirk Behme ??:
> On 12.05.2014 05:40, Huang Shijie wrote:
>> ? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
>>> 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.




>>> 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.
>> 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

[PATCH 1/5] serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()

[Dirk Behme]

On 12.05.2014 08:30, Huang Shijie wrote:
> ? 2014?05?12? 13:45, Dirk Behme ??:
>> On 12.05.2014 05:40, Huang Shijie wrote:
>>> ? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
>>>> 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.

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)'.

> 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.

> this patch makes the
> interrupt handler do more jobs.

... to get the locking in the correct order.

Best regards

Dirk

>>>> 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.
>>> 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.

[PATCH 1/5] serial: imx: remove unneeded imx_transmit_buffer() from imx_start_tx()

[Huang Shijie]

? 2014?05?12? 14:40, Dirk Behme ??:
> On 12.05.2014 08:30, Huang Shijie wrote:
>> ? 2014?05?12? 13:45, Dirk Behme ??:
>>> On 12.05.2014 05:40, Huang Shijie wrote:
>>>> ? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
>>>>> 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.
>
> 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?


>> 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.

>> 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()

[Dean Jenkins]

On 12/05/14 07:53, Huang Shijie wrote:
> ? 2014?05?12? 14:40, Dirk Behme ??:
>> On 12.05.2014 08:30, Huang Shijie wrote:
>>> ? 2014?05?12? 13:45, Dirk Behme ??:
>>>> On 12.05.2014 05:40, Huang Shijie wrote:
>>>>> ? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
>>>>>> 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.
>>
>> 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?
>
>
>>> 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.
>
>>> 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.
>
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()

[Huang Shijie]

? 2014?05?12? 16:03, Dean Jenkins ??:
> 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.
>
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()

[dean_jenkins at mentor.com]

From: Dirk Behme <dirk.behme@de.bosch.com>

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().

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Andy Lowe <andy_lowe@mentor.com>
---
 drivers/tty/serial/imx.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7b813b3..abe31ad 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -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);
 }
-- 
1.7.9.5

[PATCH 3/5] serial: imx: avoid spinlock recursion deadlock

[dean_jenkins at mentor.com]

From: Andy Lowe <andy_lowe@mentor.com>

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@mentor.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 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);
-- 
1.7.9.5

[PATCH 3/5] serial: imx: avoid spinlock recursion deadlock

[Huang Shijie]

? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
> From: Andy Lowe<andy_lowe@mentor.com>
>
> 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@mentor.com>
> Signed-off-by: Dirk Behme<dirk.behme@de.bosch.com>
> ---
>   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

has fixed this deadlock.

We can ignore this patch now.

thanks
Huang Shijie

[PATCH 3/5] serial: imx: avoid spinlock recursion deadlock

[Dean Jenkins]

On 12/05/14 04:12, Huang Shijie wrote:
> ? 2014?05?09? 23:19, dean_jenkins at mentor.com ??:
>> From: Andy Lowe<andy_lowe@mentor.com>
>>
>> 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@mentor.com>
>> Signed-off-by: Dirk Behme<dirk.behme@de.bosch.com>
>> ---
>>   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

[PATCH 4/5] serial: imx: move imx_transmit_buffer() into imx_txint()

[dean_jenkins at mentor.com]

From: Dirk Behme <dirk.behme@de.bosch.com>

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.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/tty/serial/imx.c |   31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index cc79706..7b0ef85 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -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);
-- 
1.7.9.5

[PATCH 5/5] serial: imx: clean up imx_poll_get_char()

[dean_jenkins at mentor.com]

From: Dirk Behme <dirk.behme@de.bosch.com>

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>

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 drivers/tty/serial/imx.c |   29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7b0ef85..9d59354 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -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)
-- 
1.7.9.5

[PATCH 0/5] serial: imx: fixes

[Greg KH]

On Fri, May 09, 2014 at 04:19:43PM +0100, dean_jenkins at mentor.com wrote:
> From: Dean Jenkins <Dean_Jenkins@mentor.com>
> 
> 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(-)

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

[PATCH 0/5] serial: imx: fixes

[Dirk Behme]

Am 28.05.2014 21:34, schrieb Greg KH:
> On Fri, May 09, 2014 at 04:19:43PM +0100, dean_jenkins at mentor.com wrote:
>> From: Dean Jenkins <Dean_Jenkins@mentor.com>
>>
>> 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(-)
>
> 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

[PATCH 0/5] serial: imx: fixes

[Huang Shijie]

On Thu, May 29, 2014 at 06:58:55AM +0200, Dirk Behme wrote:
> Am 28.05.2014 21:34, schrieb Greg KH:
> >On Fri, May 09, 2014 at 04:19:43PM +0100, dean_jenkins at mentor.com wrote:
> >>From: Dean Jenkins <Dean_Jenkins@mentor.com>
> >>
> >>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(-)
> >
> >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