tty: serial: imx: fix lockup and garbage on SMP

PATCH New Dirk Behme 4 participants 12 messages Aug 27, 2012 – Aug 28, 2012

Subsystems (4)

Memory management Arm i.MX and Layerscape TTY layer and serial drivers Memory mapping

lore

Share & tools

lore

Dirk Behme

author

2012-08-27 07:36 UTC #0 lore raw

cover tty: serial: imx: fix lockup and garbage on SMP

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]

mm/mmap.c +3 -0

Index: a/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
...

Dirk Behme

author

2012-08-27 07:36 UTC #1 lore raw

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

From: Xinyu Chen <[email protected]>

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 <[email protected]>

Tested-by Dirk Behme <[email protected]>

CC Shawn Guo <[email protected]>

CC Sascha Hauer <[email protected]>

---
 drivers/tty/serial/imx.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

drivers/tty/serial/imx.c +11 -1

index d5c689d..908178f 100644
@@ -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);
 }
 
 /*

Signature · 1 line

1.7.0.4

Shawn Guo

2012-08-27 22:33 UTC #2 lore raw

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

Quoted 15 lines · Dirk Behme

From: Xinyu Chen <[email protected]>
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.

Quoted 8 lines

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 <[email protected]>
Tested-by Dirk Behme <[email protected]>
CC Shawn Guo <[email protected]>
CC Sascha Hauer <[email protected]>

Acked-by Shawn Guo <[email protected]>

Dirk Behme

author

2012-08-27 07:36 UTC #3 lore raw

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

In reply to Dirk Behme

From: Xinyu Chen <[email protected]>

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 <[email protected]>

Tested-by Dirk Behme <[email protected]>

CC Shawn Guo <[email protected]>

CC Sascha Hauer <[email protected]>

---
 drivers/tty/serial/imx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/tty/serial/imx.c +2 -0

index 908178f..31ce414 100644
@@ -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

Signature · 1 line

1.7.0.4

Troy Kisky

2012-08-27 18:20 UTC #4 lore raw

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

Quoted 20 lines · Dirk Behme

From: Xinyu Chen <[email protected]>
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 <[email protected]>
Tested-by Dirk Behme <[email protected]>
CC Shawn Guo <[email protected]>
CC Sascha Hauer <[email protected]>
---
  drivers/tty/serial/imx.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
drivers/tty/serial/imx.c +0 -0
index 908178f..31ce414 100644

drivers/tty/serial/imx.c static int imx_startup(struct uart_port *port) · line 703

Quoted 9 lines

drivers/tty/serial/imx.c +2 -0

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

Dirk Behme

author

2012-08-28 10:46 UTC #5 lore raw

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

Quoted 34 lines · Troy Kisky

Quoted 20 lines · Dirk Behme
From: Xinyu Chen <[email protected]>
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 <[email protected]>
Tested-by Dirk Behme <[email protected]>
CC Shawn Guo <[email protected]>
CC Sascha Hauer <[email protected]>
---
  drivers/tty/serial/imx.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
drivers/tty/serial/imx.c +0 -0
index 908178f..31ce414 100644
drivers/tty/serial/imx.c static int imx_startup(struct uart_port *port) · line 703
Quoted 9 lines
drivers/tty/serial/imx.c +2 -0
@@ -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

Quoted 6 lines

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?

Quoted 6 lines

         val |= TXTL << 10 | RXTL;
         writel(val, sport->port.membase + UFCR);
         return 0;
}

There is no need for imx_setup_ufcr to change divisor.

Thanks

Dirk

Troy Kisky

2012-08-28 18:27 UTC #6 lore raw

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

Quoted 40 lines · Dirk Behme

Quoted 32 lines · Troy Kisky
Quoted 20 lines · Dirk Behme
From: Xinyu Chen <[email protected]>
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 <[email protected]>
Tested-by Dirk Behme <[email protected]>
CC Shawn Guo <[email protected]>
CC Sascha Hauer <[email protected]>
---
  drivers/tty/serial/imx.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
drivers/tty/serial/imx.c +0 -0
index 908178f..31ce414 100644
drivers/tty/serial/imx.c static int imx_startup(struct uart_port *port) · line 701
Quoted 7 lines
drivers/tty/serial/imx.c +1 -0
@@ -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
Quoted 6 lines
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;

Quoted 7 lines

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

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

Quoted 13 lines

Quoted 6 lines
         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

Troy Kisky

2012-08-28 18:45 UTC #7 lore raw

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

Quoted 52 lines · Troy Kisky

Quoted 40 lines · Dirk Behme
Quoted 32 lines · Troy Kisky
Quoted 20 lines · Dirk Behme
From: Xinyu Chen <[email protected]>
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 <[email protected]>
Tested-by Dirk Behme <[email protected]>
CC Shawn Guo <[email protected]>
CC Sascha Hauer <[email protected]>
---
  drivers/tty/serial/imx.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
drivers/tty/serial/imx.c +0 -0
index 908178f..31ce414 100644
drivers/tty/serial/imx.c static int imx_startup(struct uart_port *port) · line 701
Quoted 7 lines
drivers/tty/serial/imx.c +1 -0
@@ -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
Quoted 6 lines
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;
Quoted 7 lines
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

Quoted 7 lines

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.

Shawn Guo

2012-08-27 22:31 UTC #8 lore raw

cover tty: serial: imx: fix lockup and garbage on SMP

In reply to Dirk Behme

Quoted 4 lines · Dirk Behme

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: <[email protected]> tag on these patches.

Signature · 2 lines

Regards,
Shawn

Greg Kroah-Hartman

maintainer

2012-08-27 22:51 UTC #9 lore raw

cover tty: serial: imx: fix lockup and garbage on SMP

Quoted 6 lines · Shawn Guo

Quoted 4 lines · Dirk Behme
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: <[email protected]> tag on these patches.

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

greg k-h

Shawn Guo

2012-08-27 23:37 UTC #10 lore raw

cover tty: serial: imx: fix lockup and garbage on SMP

Quoted 2 lines · Greg Kroah-Hartman

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.

Signature · 2 lines

Regards,
Shawn

Dirk Behme

author

2012-08-28 06:03 UTC #11 lore raw

cover tty: serial: imx: fix lockup and garbage on SMP

Quoted 4 lines · Shawn Guo

Quoted 2 lines · Greg Kroah-Hartman
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!

Quoted 2 lines

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