| Message ID | 20240201065932.19899-1-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | tty: serial: manage irq with spin_lock_irqsave in SiFive console | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On 01. 02. 24, 7:59, Andy Chiu wrote: > It is not safe to call spin_lock() with irq disabled on RT-linux. > Instead, transfer the code segment to spin_lock_irqsave to make it work > on both RT and non-RT linux. Hi, have you investigated what is protected by the local_irq_save() in there? The lock is not always taken, OTOH the interrupts are always disabled. I believe the fix is not as easy as is presented below. > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > --- > drivers/tty/serial/sifive.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > index fa4c9336924f..3f0ddf8bfa7b 100644 > --- a/drivers/tty/serial/sifive.c > +++ b/drivers/tty/serial/sifive.c > @@ -788,13 +788,12 @@ static void sifive_serial_console_write(struct console *co, const char *s, > if (!ssp) > return; > > - local_irq_save(flags); > if (ssp->port.sysrq) > locked = 0; > else if (oops_in_progress) > - locked = spin_trylock(&ssp->port.lock); > + locked = spin_trylock_irqsave(&ssp->port.lock, flags); > else > - spin_lock(&ssp->port.lock); > + spin_lock_irqsave(&ssp->port.lock, flags); > > ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS); > __ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp); > @@ -804,8 +803,7 @@ static void sifive_serial_console_write(struct console *co, const char *s, > __ssp_writel(ier, SIFIVE_SERIAL_IE_OFFS, ssp); > > if (locked) > - spin_unlock(&ssp->port.lock); > - local_irq_restore(flags); > + spin_unlock_irqrestore(&ssp->port.lock, flags); > } > > static int __init sifive_serial_console_setup(struct console *co, char *options)
On Thu, Feb 1, 2024 at 3:21 PM Jiri Slaby <jirislaby@kernel.org> wrote: > > On 01. 02. 24, 7:59, Andy Chiu wrote: > > It is not safe to call spin_lock() with irq disabled on RT-linux. > > Instead, transfer the code segment to spin_lock_irqsave to make it work > > on both RT and non-RT linux. > > Hi, > > have you investigated what is protected by the local_irq_save() in > there? The lock is not always taken, OTOH the interrupts are always > disabled. I was referencing some serial drivers (omap, pl011) changes on the linux-rt patch series and provide logically the same change here. For all oops cases I've found, panic() itself disables irq before calling `bust_spinlocks(1)`. Architecture dependent `die()` on riscv also masks off irq with a spin_lock_irq() before calling bust_spinlocks(1). Should we make SERIAL_SIFIVE depend on RISCV in Kconfig for this? However, I am not very certain about the sysrq part here. According to the patch on linux-rt, it says irqs are already disabled if the console_write comes from sysrq handling. One difference I noticed is that the sifive console does not support magic sysrq, while the others do support. And it seems like the condition check `ssp->port.sysrq` in sifive serial is never true, but maybe I just overlooked something. + Thomas Hi Thomas, do you think the sifive console driver needs the change here for RT? Do you have any suggestions otherwise? > > I believe the fix is not as easy as is presented below. > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > --- > > drivers/tty/serial/sifive.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > > index fa4c9336924f..3f0ddf8bfa7b 100644 > > --- a/drivers/tty/serial/sifive.c > > +++ b/drivers/tty/serial/sifive.c > > @@ -788,13 +788,12 @@ static void sifive_serial_console_write(struct console *co, const char *s, > > if (!ssp) > > return; > > > > - local_irq_save(flags); > > if (ssp->port.sysrq) > > locked = 0; > > else if (oops_in_progress) > > - locked = spin_trylock(&ssp->port.lock); > > + locked = spin_trylock_irqsave(&ssp->port.lock, flags); > > else > > - spin_lock(&ssp->port.lock); > > + spin_lock_irqsave(&ssp->port.lock, flags); > > > > ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS); > > __ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp); > > @@ -804,8 +803,7 @@ static void sifive_serial_console_write(struct console *co, const char *s, > > __ssp_writel(ier, SIFIVE_SERIAL_IE_OFFS, ssp); > > > > if (locked) > > - spin_unlock(&ssp->port.lock); > > - local_irq_restore(flags); > > + spin_unlock_irqrestore(&ssp->port.lock, flags); > > } > > > > static int __init sifive_serial_console_setup(struct console *co, char *options) > > -- > js > suse labs > Regards, Andy
diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index fa4c9336924f..3f0ddf8bfa7b 100644 --- a/drivers/tty/serial/sifive.c +++ b/drivers/tty/serial/sifive.c @@ -788,13 +788,12 @@ static void sifive_serial_console_write(struct console *co, const char *s, if (!ssp) return; - local_irq_save(flags); if (ssp->port.sysrq) locked = 0; else if (oops_in_progress) - locked = spin_trylock(&ssp->port.lock); + locked = spin_trylock_irqsave(&ssp->port.lock, flags); else - spin_lock(&ssp->port.lock); + spin_lock_irqsave(&ssp->port.lock, flags); ier = __ssp_readl(ssp, SIFIVE_SERIAL_IE_OFFS); __ssp_writel(0, SIFIVE_SERIAL_IE_OFFS, ssp); @@ -804,8 +803,7 @@ static void sifive_serial_console_write(struct console *co, const char *s, __ssp_writel(ier, SIFIVE_SERIAL_IE_OFFS, ssp); if (locked) - spin_unlock(&ssp->port.lock); - local_irq_restore(flags); + spin_unlock_irqrestore(&ssp->port.lock, flags); } static int __init sifive_serial_console_setup(struct console *co, char *options)
It is not safe to call spin_lock() with irq disabled on RT-linux. Instead, transfer the code segment to spin_lock_irqsave to make it work on both RT and non-RT linux. Signed-off-by: Andy Chiu <andy.chiu@sifive.com> --- drivers/tty/serial/sifive.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)