mbox series

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

Message ID 20220405033854.110374-1-jaewon02.kim@samsung.com
Headers show
Series tty: serial: samsung: add spin_lock in console_write | expand

Message

Jaewon Kim April 5, 2022, 3:38 a.m. UTC 
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(+)

Comments

Jaewon Kim April 5, 2022, 6:40 a.m. UTC | #1 
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