[PATCH v2 0/1] tty: serial: samsung: add spin_lock in console_write

[PATCH v2 0/1] tty: serial: samsung: add spin_lock in console_write

[Jaewon Kim]

When console and printk log are printed at the same time,
they are called through tty driver and console driver concurrently.
In this case, this could lead to potintial issue that
data loss or fifo full.

This issue also occurred with other drivers and has been fixed.
"serial: amba-pl011: lock console writes against interrupts"
 - https://lkml.org/lkml/2012/2/1/495

---
Chnages since v1:
 - locked variable type changed bool from int
 - spin_lock() changed to spin_ock_irqsave()

Jaewon Kim (1):
  tty: serial: samsung: add spin_lock for interrupt and console_write

 drivers/tty/serial/samsung_tty.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.35.1

[PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jaewon Kim]

The console_write and IRQ handler can run concurrently.
Problems may occurs console_write is continuously executed while
the IRQ handler is running.

Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
---
 drivers/tty/serial/samsung_tty.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index e1585fbae909..9db479d728b5 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2480,12 +2480,24 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
 			     unsigned int count)
 {
 	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
+	unsigned long flags;
+	bool locked = 1;
 
 	/* not possible to xmit on unconfigured port */
 	if (!s3c24xx_port_configured(ucon))
 		return;
 
+	if (cons_uart->sysrq)
+		locked = 0;
+	else if (oops_in_progress)
+		locked = spin_trylock_irqsave(&cons_uart->lock, flags);
+	else
+		spin_lock_irqsave(&cons_uart->lock, flags);
+
 	uart_console_write(cons_uart, s, count, s3c24xx_serial_console_putchar);
+
+	if (locked)
+		spin_unlock_irqrestore(&cons_uart->lock, flags);
 }
 
 /* Shouldn't be __init, as it can be instantiated from other module */
-- 
2.35.1

Re: [PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Greg Kroah-Hartman]

On Wed, Apr 06, 2022 at 05:22:16PM +0900, Jaewon Kim wrote:
> The console_write and IRQ handler can run concurrently.
> Problems may occurs console_write is continuously executed while
> the IRQ handler is running.
> 
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index e1585fbae909..9db479d728b5 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -2480,12 +2480,24 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
>  			     unsigned int count)
>  {
>  	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
> +	unsigned long flags;
> +	bool locked = 1;

"1" is not a boolean :)

RE: [PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jaewon Kim]

Hello 

On 22. 4. 6. 17:21, Greg Kroah-Hartman wrote: 
> On Wed, Apr 06, 2022 at 05:22:16PM +0900, Jaewon Kim wrote:
> > The console_write and IRQ handler can run concurrently.
> > Problems may occurs console_write is continuously executed while the
> > IRQ handler is running.
> >
> > Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> > ---
> >  drivers/tty/serial/samsung_tty.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index e1585fbae909..9db479d728b5 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -2480,12 +2480,24 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
> >  			     unsigned int count)
> >  {
> >  	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
> > +	unsigned long flags;
> > +	bool locked = 1;
> 
> "1" is not a boolean :)

return value of spin_trylock() is 1 or 0.
It seems better to keep it as an int than to change it to bool.
I will return it to int.


Thanks
Jaewon Kim

Re: [PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jiri Slaby]

On 06. 04. 22, 10:39, Jaewon Kim wrote:
> On 22. 4. 6. 17:21, Greg Kroah-Hartman wrote:
>> On Wed, Apr 06, 2022 at 05:22:16PM +0900, Jaewon Kim wrote:
>>> The console_write and IRQ handler can run concurrently.
>>> Problems may occurs console_write is continuously executed while the
>>> IRQ handler is running.
>>>
>>> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
>>> ---
>>>   drivers/tty/serial/samsung_tty.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/samsung_tty.c
>>> b/drivers/tty/serial/samsung_tty.c
>>> index e1585fbae909..9db479d728b5 100644
>>> --- a/drivers/tty/serial/samsung_tty.c
>>> +++ b/drivers/tty/serial/samsung_tty.c
>>> @@ -2480,12 +2480,24 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
>>>   			     unsigned int count)
>>>   {
>>>   	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
>>> +	unsigned long flags;
>>> +	bool locked = 1;
>>
>> "1" is not a boolean :)
> 
> return value of spin_trylock() is 1 or 0.
> It seems better to keep it as an int than to change it to bool.
> I will return it to int.

Hi, no, do not that. Simply use bool/true/false.

thanks,
-- 
js
suse labs

RE: [PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jaewon Kim]

On 22. 4. 6. 18:48, Jiri Slaby wrote:> On 06. 04. 22, 10:39, Jaewon Kim wrote:
> > On 22. 4. 6. 17:21, Greg Kroah-Hartman wrote:
> >> On Wed, Apr 06, 2022 at 05:22:16PM +0900, Jaewon Kim wrote:
> >>> The console_write and IRQ handler can run concurrently.
> >>> Problems may occurs console_write is continuously executed while the
> >>> IRQ handler is running.
> >>>
> >>> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> >>> ---
> >>>   drivers/tty/serial/samsung_tty.c | 12 ++++++++++++
> >>>   1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/tty/serial/samsung_tty.c
> >>> b/drivers/tty/serial/samsung_tty.c
> >>> index e1585fbae909..9db479d728b5 100644
> >>> --- a/drivers/tty/serial/samsung_tty.c
> >>> +++ b/drivers/tty/serial/samsung_tty.c
> >>> @@ -2480,12 +2480,24 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
> >>>   			     unsigned int count)
> >>>   {
> >>>   	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
> >>> +	unsigned long flags;
> >>> +	bool locked = 1;
> >>
> >> "1" is not a boolean :)
> >
> > return value of spin_trylock() is 1 or 0.
> > It seems better to keep it as an int than to change it to bool.
> > I will return it to int.
> 
> Hi, no, do not that. Simply use bool/true/false.
> 

Okay, Thanks to review.
I will fix 1 to bool in next v3 patch.

> thanks,
> --
> js
> suse labs

Thanks
Jaewon Kim

Re: [PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jiri Slaby]

On 06. 04. 22, 13:19, Jaewon Kim wrote:
> Okay, Thanks to review.
> I will fix 1 to bool in next v3 patch.

(And 0 to false.)

-- 
js
suse labs

[PATCH 0/1] tty: serial: samsung: add spin_lock in console_write

[Jaewon Kim]

When console and printk log are printed at the same time,
they are called through tty driver and console driver concurrently.
In this case, this could lead to potintial issue that
data loss or fifo full.

This issue also occurred with other drivers and has been fixed.
"serial: amba-pl011: lock console writes against interrupts"


Jaewon Kim (1):
  tty: serial: samsung: add spin_lock for interrupt and console_write

 drivers/tty/serial/samsung_tty.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.35.1

[PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jaewon Kim]

The console_write and IRQ handler can run concurrently.
Problems may occurs console_write is continuously executed while
the IRQ handler is running.

Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
---
 drivers/tty/serial/samsung_tty.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index e1585fbae909..d362e8e114f1 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -2480,12 +2480,26 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
 			     unsigned int count)
 {
 	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
+	unsigned long flags;
+	int locked = 1;
 
 	/* not possible to xmit on unconfigured port */
 	if (!s3c24xx_port_configured(ucon))
 		return;
 
+	local_irq_save(flags);
+	if (cons_uart->sysrq)
+		locked = 0;
+	else if (oops_in_progress)
+		locked = spin_trylock(&cons_uart->lock);
+	else
+		spin_lock(&cons_uart->lock);
+
 	uart_console_write(cons_uart, s, count, s3c24xx_serial_console_putchar);
+
+	if (locked)
+		spin_unlock(&cons_uart->lock);
+	local_irq_restore(flags);
 }
 
 /* Shouldn't be __init, as it can be instantiated from other module */
-- 
2.35.1

Re: [PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Greg Kroah-Hartman]

On Tue, Apr 05, 2022 at 12:38:54PM +0900, Jaewon Kim wrote:
> The console_write and IRQ handler can run concurrently.
> Problems may occurs console_write is continuously executed while
> the IRQ handler is running.
> 
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

What commit does this fix?

> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index e1585fbae909..d362e8e114f1 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -2480,12 +2480,26 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
>  			     unsigned int count)
>  {
>  	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
> +	unsigned long flags;
> +	int locked = 1;

bool?

>  
>  	/* not possible to xmit on unconfigured port */
>  	if (!s3c24xx_port_configured(ucon))
>  		return;
>  
> +	local_irq_save(flags);
> +	if (cons_uart->sysrq)
> +		locked = 0;
> +	else if (oops_in_progress)
> +		locked = spin_trylock(&cons_uart->lock);
> +	else
> +		spin_lock(&cons_uart->lock);
> +
>  	uart_console_write(cons_uart, s, count, s3c24xx_serial_console_putchar);
> +
> +	if (locked)
> +		spin_unlock(&cons_uart->lock);
> +	local_irq_restore(flags);

Why is irq_save required as well as a spinlock?

thanks,

greg k-h

RE: [PATCH 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jaewon Kim]

Hello

On 22. 4. 5. 14:01, Greg Kroah-Hartman wrote:
> On Tue, Apr 05, 2022 at 12:38:54PM +0900, Jaewon Kim wrote:
> > The console_write and IRQ handler can run concurrently.
> > Problems may occurs console_write is continuously executed while the
> > IRQ handler is running.
> >
> > Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> > ---
> >  drivers/tty/serial/samsung_tty.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> 
> What commit does this fix?

This is not an issue caused by anohter commits.
There was potential issue from the beginning.

Other drivers were fixed, but samsung_tty was not.
PL011 patch : https://lkml.org/lkml/2012/2/1/495


> 
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index e1585fbae909..d362e8e114f1 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -2480,12 +2480,26 @@ s3c24xx_serial_console_write(struct console *co, const char *s,
> >  			     unsigned int count)
> >  {
> >  	unsigned int ucon = rd_regl(cons_uart, S3C2410_UCON);
> > +	unsigned long flags;
> > +	int locked = 1;
> 
> bool?

It is return value of spin_trylock()
I used int because mose drivers used int.
If you guide to change int to bool, I will change it.

> 
> >
> >  	/* not possible to xmit on unconfigured port */
> >  	if (!s3c24xx_port_configured(ucon))
> >  		return;
> >
> > +	local_irq_save(flags);
> > +	if (cons_uart->sysrq)
> > +		locked = 0;
> > +	else if (oops_in_progress)
> > +		locked = spin_trylock(&cons_uart->lock);
> > +	else
> > +		spin_lock(&cons_uart->lock);
> > +
> >  	uart_console_write(cons_uart, s, count,
> > s3c24xx_serial_console_putchar);
> > +
> > +	if (locked)
> > +		spin_unlock(&cons_uart->lock);
> > +	local_irq_restore(flags);
> 
> Why is irq_save required as well as a spinlock?

No special reason.
I will change spin_trylock() -? spin_trylock_irqsave().
spin_lock -> spin_lock_irqsave().
And, remove local_irq_save/restore.
It looks more clean.


> 
> thanks,
> 
> greg k-h

Thanks
Jaewon Kim