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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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