[PATCH 0/2] tty: serial: imx: fix lockup and garbage on SMP

[PATCH 0/2] tty: serial: imx: fix lockup and garbage on SMP

[Dirk Behme]

The following two patches are a port from Freescale's patch

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/drivers/tty/serial/imx.c?h=imx_3.0.15_android&id=80e525d66d1818722db18e21dc7f1dc41f314156

Using a 3.6-rc3 kernel on a i.MX6 SabreLite board and increasing the
verbosity of the kernel's serial console output with some debug printk
like [1] results in two issues:

1. After some time the kernel output hangs [2]. The debugger tells us that
it hangs in drivers/tty/serial/imx.c at

...
/*
 *	Finally, wait for transmitter to become empty
 *	and restore UCR1/2/3
 */
while (!(readl(sport->port.membase + USR2) & USR2_TXDC));
...

with permanently reading 0x1000 from USR2 while it waits for bit USR2_TXDC
(0x8).

2. Even after applying the first patch, there happens to be some garbage in the
   output [3].

Both issues are gone applying these two patches.

Note: If these patches are fine, they should go to stable, too.

Best regards

Dirk

[1]

Index: a/mm/mmap.c
===================================================================
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1290,6 +1290,9 @@ munmap_back:
 	vma->vm_pgoff = pgoff;
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
 
+	printk(" => mmap_region: start: 0x%08X end: 0x%08X len: %d\n",
+		(unsigned int)vma->vm_start, (unsigned int)vma->vm_end, (int)len);
+
 	error = -EINVAL;	/* when rejecting VM_GROWSDOWN|VM_GROWSUP */
 
 	if (file) {

[2]
...
=> mmap_region: start: 0x76DBC000 end: 0x76DBE000 len: 8192
=> mmap_region: start: 0x76DBE000 end: 0x76DC0000 len: 8192
=> mmap_region: start: 0x76D5C000 end: 0x76DA0000 ??
<hang, no output any more>

[3]
...
=> mmap_region: start: 0x0011C000 end: 0x0011D000 len: 4096
=> mmap_region: start: 0x00008000 end: 0x000B500f??#.??????R????r????j???????}???????????? start: 0x000BD000 end: 0x000BE000 len: 4096
=> mmap_region: start: 0x000BD000 end: 0x000BE000 len: 4096
...

[PATCH 1/2] tty: serial: imx: console write routing is unsafe on SMP

[Dirk Behme]

From: Xinyu Chen <xinyu.chen@freescale.com>

The console feature's write routing is unsafe on SMP with
the startup/shutdown call.

There could be several consumers of the console
* the kernel printk
* the init process using /dev/kmsg to call printk to show log
* shell, which open /dev/console and write with sys_write()

The shell goes into the normal uart open/write routing,
but the other two go into the console operations.
The open routing calls imx serial startup, which will write USR1/2
register without any lock and critical with imx_console_write call.

Add a spin_lock for startup/shutdown/console_write routing.

This patch is a port from Freescale's Android kernel.

Signed-off-by: Xinyu Chen <xinyu.chen@freescale.com>
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
CC: Shawn Guo <shawn.guo@linaro.org>
CC: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d5c689d..908178f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -754,6 +754,7 @@ static int imx_startup(struct uart_port *port)
 		}
 	}
 
+	spin_lock_irqsave(&sport->port.lock, flags);
 	/*
 	 * Finally, clear and enable interrupts
 	 */
@@ -807,7 +808,6 @@ static int imx_startup(struct uart_port *port)
 	/*
 	 * Enable modem status interrupts
 	 */
-	spin_lock_irqsave(&sport->port.lock,flags);
 	imx_enable_ms(&sport->port);
 	spin_unlock_irqrestore(&sport->port.lock,flags);
 
@@ -837,10 +837,13 @@ static void imx_shutdown(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long temp;
+	unsigned long flags;
 
+	spin_lock_irqsave(&sport->port.lock, flags);
 	temp = readl(sport->port.membase + UCR2);
 	temp &= ~(UCR2_TXEN);
 	writel(temp, sport->port.membase + UCR2);
+	spin_unlock_irqrestore(&sport->port.lock, flags);
 
 	if (USE_IRDA(sport)) {
 		struct imxuart_platform_data *pdata;
@@ -869,12 +872,14 @@ static void imx_shutdown(struct uart_port *port)
 	 * Disable all interrupts, port and break condition.
 	 */
 
+	spin_lock_irqsave(&sport->port.lock, flags);
 	temp = readl(sport->port.membase + UCR1);
 	temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN);
 	if (USE_IRDA(sport))
 		temp &= ~(UCR1_IREN);
 
 	writel(temp, sport->port.membase + UCR1);
+	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
 static void
@@ -1217,6 +1222,9 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
 	struct imx_port *sport = imx_ports[co->index];
 	struct imx_port_ucrs old_ucr;
 	unsigned int ucr1;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sport->port.lock, flags);
 
 	/*
 	 *	First, save UCR1/2/3 and then disable interrupts
@@ -1242,6 +1250,8 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
 	while (!(readl(sport->port.membase + USR2) & USR2_TXDC));
 
 	imx_port_ucrs_restore(&sport->port, &old_ucr);
+
+	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
 /*
-- 
1.7.0.4

[PATCH 2/2] tty: serial: imx: don't reinit clock with enabled console

[Dirk Behme]

From: Xinyu Chen <xinyu.chen@freescale.com>

Remove the imx_setup_ufcr() call on startup when CONSOLE enabled,
as this will cause clock reinit, and output garbage.

This patch is a port from Freescale's Android kernel.

Signed-off-by: Xinyu Chen <xinyu.chen@freescale.com>
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
CC: Shawn Guo <shawn.guo@linaro.org>
CC: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 908178f..31ce414 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -695,7 +695,9 @@ static int imx_startup(struct uart_port *port)
 	int retval;
 	unsigned long flags, temp;
 
+#ifndef CONFIG_SERIAL_CORE_CONSOLE
 	imx_setup_ufcr(sport, 0);
+#endif
 
 	/* disable the DREN bit (Data Ready interrupt enable) before
 	 * requesting IRQs
-- 
1.7.0.4

[PATCH 2/2] tty: serial: imx: don't reinit clock with enabled console

[Troy Kisky]

On 8/27/2012 12:36 AM, Dirk Behme wrote:
> From: Xinyu Chen <xinyu.chen@freescale.com>
>
> Remove the imx_setup_ufcr() call on startup when CONSOLE enabled,
> as this will cause clock reinit, and output garbage.
>
> This patch is a port from Freescale's Android kernel.
>
> Signed-off-by: Xinyu Chen <xinyu.chen@freescale.com>
> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
> CC: Shawn Guo <shawn.guo@linaro.org>
> CC: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>   drivers/tty/serial/imx.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 908178f..31ce414 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -695,7 +695,9 @@ static int imx_startup(struct uart_port *port)
>   	int retval;
>   	unsigned long flags, temp;
>   
> +#ifndef CONFIG_SERIAL_CORE_CONSOLE
>   	imx_setup_ufcr(sport, 0);
> +#endif
>   
>   	/* disable the DREN bit (Data Ready interrupt enable) before
>   	 * requesting IRQs


I'd rather do something like this

static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
{
         unsigned int val;

         /* set receiver / transmitter trigger level. */
         val = readl(sport->port.membase + UFCR) & UFCR_RFDIV;
         val |= TXTL << 10 | RXTL;
         writel(val, sport->port.membase + UFCR);
         return 0;
}

There is no need for imx_setup_ufcr to change divisor.


Troy

[PATCH 0/2] tty: serial: imx: fix lockup and garbage on SMP

[Shawn Guo]

On Mon, Aug 27, 2012 at 09:36:50AM +0200, Dirk Behme wrote:
> Both issues are gone applying these two patches.
> 
> Note: If these patches are fine, they should go to stable, too.
> 
Then you should have Cc: <stable@vger.kernel.org> tag on these patches.

-- 
Regards,
Shawn

[PATCH 1/2] tty: serial: imx: console write routing is unsafe on SMP

[Shawn Guo]

On Mon, Aug 27, 2012 at 09:36:51AM +0200, Dirk Behme wrote:
> From: Xinyu Chen <xinyu.chen@freescale.com>
> 
> The console feature's write routing is unsafe on SMP with
> the startup/shutdown call.
> 
> There could be several consumers of the console
> * the kernel printk
> * the init process using /dev/kmsg to call printk to show log
> * shell, which open /dev/console and write with sys_write()
> 
> The shell goes into the normal uart open/write routing,
> but the other two go into the console operations.
> The open routing calls imx serial startup, which will write USR1/2
> register without any lock and critical with imx_console_write call.
> 
> Add a spin_lock for startup/shutdown/console_write routing.
> 
> This patch is a port from Freescale's Android kernel.
> 
> Signed-off-by: Xinyu Chen <xinyu.chen@freescale.com>
> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
> CC: Shawn Guo <shawn.guo@linaro.org>
> CC: Sascha Hauer <s.hauer@pengutronix.de>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

[PATCH 0/2] tty: serial: imx: fix lockup and garbage on SMP

[Greg Kroah-Hartman]

On Tue, Aug 28, 2012 at 06:31:35AM +0800, Shawn Guo wrote:
> On Mon, Aug 27, 2012 at 09:36:50AM +0200, Dirk Behme wrote:
> > Both issues are gone applying these two patches.
> > 
> > Note: If these patches are fine, they should go to stable, too.
> > 
> Then you should have Cc: <stable@vger.kernel.org> tag on these patches.

On both of them?  I can do that if needed.  How far back should they go?

greg k-h

[PATCH 0/2] tty: serial: imx: fix lockup and garbage on SMP

[Shawn Guo]

On Mon, Aug 27, 2012 at 03:51:42PM -0700, Greg Kroah-Hartman wrote:
> On both of them?  I can do that if needed.  How far back should they go?
> 
It should be okay to apply on 3.4 and 3.5.  But patch #2 is still in
question.  I do not like the #ifdef.

-- 
Regards,
Shawn

[PATCH 0/2] tty: serial: imx: fix lockup and garbage on SMP

[Dirk Behme]

On 28.08.2012 01:37, Shawn Guo wrote:
> On Mon, Aug 27, 2012 at 03:51:42PM -0700, Greg Kroah-Hartman wrote:
>> On both of them?  I can do that if needed.  How far back should they go?
>>
> It should be okay to apply on 3.4 and 3.5. 

For patch #1/2: Yes, thanks!

> But patch #2 is still in
> question.  I do not like the #ifdef.

For patch #2/2: Yes, understood. I'll look at Troy's proposal and will 
update #2/2.

Many thanks and best regards

Dirk

[PATCH 2/2] tty: serial: imx: don't reinit clock with enabled console

[Dirk Behme]

On 27.08.2012 20:20, Troy Kisky wrote:
> On 8/27/2012 12:36 AM, Dirk Behme wrote:
>> From: Xinyu Chen <xinyu.chen@freescale.com>
>>
>> Remove the imx_setup_ufcr() call on startup when CONSOLE enabled,
>> as this will cause clock reinit, and output garbage.
>>
>> This patch is a port from Freescale's Android kernel.
>>
>> Signed-off-by: Xinyu Chen <xinyu.chen@freescale.com>
>> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
>> CC: Shawn Guo <shawn.guo@linaro.org>
>> CC: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>   drivers/tty/serial/imx.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 908178f..31ce414 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -695,7 +695,9 @@ static int imx_startup(struct uart_port *port)
>>   	int retval;
>>   	unsigned long flags, temp;
>>   
>> +#ifndef CONFIG_SERIAL_CORE_CONSOLE
>>   	imx_setup_ufcr(sport, 0);
>> +#endif
>>   
>>   	/* disable the DREN bit (Data Ready interrupt enable) before
>>   	 * requesting IRQs
> 
> 
> I'd rather do something like this
> 
> static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
> {
>          unsigned int val;
> 
>          /* set receiver / transmitter trigger level. */
>          val = readl(sport->port.membase + UFCR) & UFCR_RFDIV;

Shouldn't it be

... & (UFCR_RFDIV | UFCR_DCEDTE);

then? My i.MX6 manual has DCEDTE as bit 6, which we don't want to touch, 
too? We only want to touch TXTL and RXTL?

>          val |= TXTL << 10 | RXTL;
>          writel(val, sport->port.membase + UFCR);
>          return 0;
> }
> 
> There is no need for imx_setup_ufcr to change divisor.

Ok

Thanks

Dirk

[PATCH 2/2] tty: serial: imx: don't reinit clock with enabled console

[Troy Kisky]

On 8/28/2012 3:46 AM, Dirk Behme wrote:
> On 27.08.2012 20:20, Troy Kisky wrote:
>> On 8/27/2012 12:36 AM, Dirk Behme wrote:
>>> From: Xinyu Chen <xinyu.chen@freescale.com>
>>>
>>> Remove the imx_setup_ufcr() call on startup when CONSOLE enabled,
>>> as this will cause clock reinit, and output garbage.
>>>
>>> This patch is a port from Freescale's Android kernel.
>>>
>>> Signed-off-by: Xinyu Chen <xinyu.chen@freescale.com>
>>> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
>>> CC: Shawn Guo <shawn.guo@linaro.org>
>>> CC: Sascha Hauer <s.hauer@pengutronix.de>
>>> ---
>>>   drivers/tty/serial/imx.c |    2 ++
>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>> index 908178f..31ce414 100644
>>> --- a/drivers/tty/serial/imx.c
>>> +++ b/drivers/tty/serial/imx.c
>>> @@ -695,7 +695,9 @@ static int imx_startup(struct uart_port *port)
>>>       int retval;
>>>       unsigned long flags, temp;
>>>   +#ifndef CONFIG_SERIAL_CORE_CONSOLE
>>>       imx_setup_ufcr(sport, 0);
>>> +#endif
>>>         /* disable the DREN bit (Data Ready interrupt enable) before
>>>        * requesting IRQs
>>
>>
>> I'd rather do something like this
>>
>> static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
>> {
>>          unsigned int val;
>>
>>          /* set receiver / transmitter trigger level. */
>>          val = readl(sport->port.membase + UFCR) & UFCR_RFDIV;
>
> Shouldn't it be
>
> ... & (UFCR_RFDIV | UFCR_DCEDTE);
>
> then? My i.MX6 manual has DCEDTE as bit 6, which we don't want to 
> touch, too? We only want to touch TXTL and RXTL?
>

If you do that, you should also change imx_set_termios
to possibly clear the bit

new_ufcr = (old_ufcr & (~UFCR_RFDIV)) | UFCR_RFDIV_REG(div);

to

new_ufcr = (old_ufcr & (~(UFCR_RFDIV | UFCR_DCEDTE))) | UFCR_RFDIV_REG(div);

>>          val |= TXTL << 10 | RXTL;
>>          writel(val, sport->port.membase + UFCR);
>>          return 0;
>> }
>>
>> There is no need for imx_setup_ufcr to change divisor.
>
> Ok
>
> Thanks
>
> Dirk
>

[PATCH 2/2] tty: serial: imx: don't reinit clock with enabled console

[Troy Kisky]

On 8/28/2012 11:27 AM, Troy Kisky wrote:
> On 8/28/2012 3:46 AM, Dirk Behme wrote:
>> On 27.08.2012 20:20, Troy Kisky wrote:
>>> On 8/27/2012 12:36 AM, Dirk Behme wrote:
>>>> From: Xinyu Chen <xinyu.chen@freescale.com>
>>>>
>>>> Remove the imx_setup_ufcr() call on startup when CONSOLE enabled,
>>>> as this will cause clock reinit, and output garbage.
>>>>
>>>> This patch is a port from Freescale's Android kernel.
>>>>
>>>> Signed-off-by: Xinyu Chen <xinyu.chen@freescale.com>
>>>> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>> CC: Shawn Guo <shawn.guo@linaro.org>
>>>> CC: Sascha Hauer <s.hauer@pengutronix.de>
>>>> ---
>>>>   drivers/tty/serial/imx.c |    2 ++
>>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>>> index 908178f..31ce414 100644
>>>> --- a/drivers/tty/serial/imx.c
>>>> +++ b/drivers/tty/serial/imx.c
>>>> @@ -695,7 +695,9 @@ static int imx_startup(struct uart_port *port)
>>>>       int retval;
>>>>       unsigned long flags, temp;
>>>>   +#ifndef CONFIG_SERIAL_CORE_CONSOLE
>>>>       imx_setup_ufcr(sport, 0);
>>>> +#endif
>>>>         /* disable the DREN bit (Data Ready interrupt enable) before
>>>>        * requesting IRQs
>>>
>>>
>>> I'd rather do something like this
>>>
>>> static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
>>> {
>>>          unsigned int val;
>>>
>>>          /* set receiver / transmitter trigger level. */
>>>          val = readl(sport->port.membase + UFCR) & UFCR_RFDIV;
>>
>> Shouldn't it be
>>
>> ... & (UFCR_RFDIV | UFCR_DCEDTE);
>>
>> then? My i.MX6 manual has DCEDTE as bit 6, which we don't want to 
>> touch, too? We only want to touch TXTL and RXTL?
>>
>
> If you do that, you should also change imx_set_termios
> to possibly clear the bit
>
> new_ufcr = (old_ufcr & (~UFCR_RFDIV)) | UFCR_RFDIV_REG(div);
>
> to
>
> new_ufcr = (old_ufcr & (~(UFCR_RFDIV | UFCR_DCEDTE))) | 
> UFCR_RFDIV_REG(div);
>

Sorry, I was looking at the wrong tree. Mainline does not have

sport->use_dcedte. I'm fine either way.