[PATCH V2] serial: pxa: add spin lock for console write

[PATCH V2] serial: pxa: add spin lock for console write

[Chao Xie]

at UP mode, when cpu want to print message in kernel, it will invoke
peempt_disable and disable irq. So it is safe for UP mode.
For SMP mode, it is not safe to protect the HW reigsters.
one CPU will run a program which will invoke printf.
another CPU will run a program in kernel that invoke printk.
So when second CPU is trying to printk, it will do
1. save ier register
2. enable uue bit of ier register
3. push buffer to uart fifo
4 .restore ier register
when first CPU want to printf, and it happens between 1 and 4, it will
enable thre bit of ier, and waiting for transmit intterupt. while step 4
will make the ier lost thre bit.
add spin lock here to protect the ier register for console write.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/tty/serial/pxa.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 5847a4b..aca62f6 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c
@@ -670,9 +670,19 @@ serial_pxa_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct uart_pxa_port *up = serial_pxa_ports[co->index];
 	unsigned int ier;
+	unsigned long flags;
+	int locked = 1;
 
 	clk_prepare_enable(up->clk);
 
+	local_irq_save(flags);
+	if (up->port.sysrq)
+		locked = 0;
+	else if (oops_in_progress)
+		locked = spin_trylock(&up->port.lock);
+	else
+		spin_lock(&up->port.lock);
+
 	/*
 	 *	First save the IER then disable the interrupts
 	 */
@@ -688,7 +698,12 @@ serial_pxa_console_write(struct console *co, const char *s, unsigned int count)
 	wait_for_xmitr(up);
 	serial_out(up, UART_IER, ier);
 
+	if (locked)
+		spin_unlock(&up->port.lock);
+	local_irq_restore(flags);
+
 	clk_disable_unprepare(up->clk);
+
 }
 
 static int __init
-- 
1.7.0.4

Re: [PATCH V2] serial: pxa: add spin lock for console write

[Greg KH]

On Tue, May 22, 2012 at 12:43:00PM +0800, Chao Xie wrote:
> at UP mode, when cpu want to print message in kernel, it will invoke
> peempt_disable and disable irq. So it is safe for UP mode.
> For SMP mode, it is not safe to protect the HW reigsters.
> one CPU will run a program which will invoke printf.
> another CPU will run a program in kernel that invoke printk.
> So when second CPU is trying to printk, it will do
> 1. save ier register
> 2. enable uue bit of ier register
> 3. push buffer to uart fifo
> 4 .restore ier register
> when first CPU want to printf, and it happens between 1 and 4, it will
> enable thre bit of ier, and waiting for transmit intterupt. while step 4
> will make the ier lost thre bit.
> add spin lock here to protect the ier register for console write.
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>

What is different from patch v1?

> ---
>  drivers/tty/serial/pxa.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
> index 5847a4b..aca62f6 100644
> --- a/drivers/tty/serial/pxa.c
> +++ b/drivers/tty/serial/pxa.c
> @@ -670,9 +670,19 @@ serial_pxa_console_write(struct console *co, const char *s, unsigned int count)
>  {
>  	struct uart_pxa_port *up = serial_pxa_ports[co->index];
>  	unsigned int ier;
> +	unsigned long flags;
> +	int locked = 1;
>  
>  	clk_prepare_enable(up->clk);
>  
> +	local_irq_save(flags);
> +	if (up->port.sysrq)
> +		locked = 0;
> +	else if (oops_in_progress)
> +		locked = spin_trylock(&up->port.lock);
> +	else
> +		spin_lock(&up->port.lock);
> +
>  	/*
>  	 *	First save the IER then disable the interrupts
>  	 */
> @@ -688,7 +698,12 @@ serial_pxa_console_write(struct console *co, const char *s, unsigned int count)
>  	wait_for_xmitr(up);
>  	serial_out(up, UART_IER, ier);
>  
> +	if (locked)
> +		spin_unlock(&up->port.lock);
> +	local_irq_restore(flags);
> +
>  	clk_disable_unprepare(up->clk);
> +
>  }

Why the extra line at the end of the function?

thanks,

greg k-h

RE: [PATCH V2] serial: pxa: add spin lock for console write

[Haojian Zhuang]

V2 moves local_irq_save() after clk_prepare_enable().

The extra empty line is unnecessary.

Greg,

Would you apply this patch?

Regards
Haojian
________________________________________
From: Greg KH [gregkh@linuxfoundation.org]
Sent: Wednesday, June 13, 2012 6:35 AM
To: Chao Xie
Cc: linux-serial@vger.kernel.org; Haojian Zhuang
Subject: Re: [PATCH V2] serial: pxa: add spin lock for console write

On Tue, May 22, 2012 at 12:43:00PM +0800, Chao Xie wrote:
> at UP mode, when cpu want to print message in kernel, it will invoke
> peempt_disable and disable irq. So it is safe for UP mode.
> For SMP mode, it is not safe to protect the HW reigsters.
> one CPU will run a program which will invoke printf.
> another CPU will run a program in kernel that invoke printk.
> So when second CPU is trying to printk, it will do
> 1. save ier register
> 2. enable uue bit of ier register
> 3. push buffer to uart fifo
> 4 .restore ier register
> when first CPU want to printf, and it happens between 1 and 4, it will
> enable thre bit of ier, and waiting for transmit intterupt. while step 4
> will make the ier lost thre bit.
> add spin lock here to protect the ier register for console write.
>
> Signed-off-by: Chao Xie <chao.xie@marvell.com>

What is different from patch v1?

> ---
>  drivers/tty/serial/pxa.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
> index 5847a4b..aca62f6 100644
> --- a/drivers/tty/serial/pxa.c
> +++ b/drivers/tty/serial/pxa.c
> @@ -670,9 +670,19 @@ serial_pxa_console_write(struct console *co, const char *s, unsigned int count)
>  {
>       struct uart_pxa_port *up = serial_pxa_ports[co->index];
>       unsigned int ier;
> +     unsigned long flags;
> +     int locked = 1;
>
>       clk_prepare_enable(up->clk);
>
> +     local_irq_save(flags);
> +     if (up->port.sysrq)
> +             locked = 0;
> +     else if (oops_in_progress)
> +             locked = spin_trylock(&up->port.lock);
> +     else
> +             spin_lock(&up->port.lock);
> +
>       /*
>        *      First save the IER then disable the interrupts
>        */
> @@ -688,7 +698,12 @@ serial_pxa_console_write(struct console *co, const char *s, unsigned int count)
>       wait_for_xmitr(up);
>       serial_out(up, UART_IER, ier);
>
> +     if (locked)
> +             spin_unlock(&up->port.lock);
> +     local_irq_restore(flags);
> +
>       clk_disable_unprepare(up->clk);
> +
>  }

Why the extra line at the end of the function?

thanks,

greg k-h

Re: [PATCH V2] serial: pxa: add spin lock for console write

[Greg KH]

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top


On Wed, Jun 13, 2012 at 08:14:20PM -0700, Haojian Zhuang wrote:
> V2 moves local_irq_save() after clk_prepare_enable().

Then say that in the patch description please.

> The extra empty line is unnecessary.

Great, please remove it :)

> Greg,
> 
> Would you apply this patch?

Please resend it with the above changes made.

thanks,

greg k-h