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