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

[PATCH v3 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

---
Changes since v2:
 - value of lock is chanaged to true/false

Changes since v1:
 - locked variable type changed bool from int
 - spin_lock() changed to spin_lock_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 v3 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..8af5aceb9f4e 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 = true;
 
 	/* not possible to xmit on unconfigured port */
 	if (!s3c24xx_port_configured(ucon))
 		return;
 
+	if (cons_uart->sysrq)
+		locked = false;
+	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 v3 1/1] tty: serial: samsung: add spin_lock for interrupt and console_write

[Jiri Slaby]

On 07. 04. 22, 9:16, 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.

 From the patch POV:

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

But given this is a v3 with no version changelog below "---", you've 
just kicked the Greg's bot to wake up :P.

> 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..8af5aceb9f4e 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 = true;
>   
>   	/* not possible to xmit on unconfigured port */
>   	if (!s3c24xx_port_configured(ucon))
>   		return;
>   
> +	if (cons_uart->sysrq)
> +		locked = false;
> +	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 */


-- 
js
suse labs

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

[Krzysztof Kozlowski]

On 07/04/2022 09:46, Jiri Slaby wrote:
> On 07. 04. 22, 9:16, 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.
> 
>  From the patch POV:
> 
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
> 
> But given this is a v3 with no version changelog below "---", you've 
> just kicked the Greg's bot to wake up :P.
> 

There was a cover letter with such changelog:
https://lore.kernel.org/all/20220407071619.102249-1-jaewon02.kim@samsung.com/

It's indeed easy to miss...

Best regards,
Krzysztof

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

[Jiri Slaby]

On 07. 04. 22, 9:58, Krzysztof Kozlowski wrote:
> On 07/04/2022 09:46, Jiri Slaby wrote:
>> On 07. 04. 22, 9:16, 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.
>>
>>   From the patch POV:
>>
>> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
>>
>> But given this is a v3 with no version changelog below "---", you've
>> just kicked the Greg's bot to wake up :P.
>>
> 
> There was a cover letter with such changelog:
> https://lore.kernel.org/all/20220407071619.102249-1-jaewon02.kim@samsung.com/
> 
> It's indeed easy to miss...

Ah, OK, my bad -- I skipped it as usual -- I don't consider cover 
letters that useful. And in this case, even more provided it is for a 
single patch.

thanks,
-- 
js
suse labs