[PATCH] tty: serial: manage irq with spin_lock_irqsave in SiFive console

[Andy Chiu]

On Thu, Feb 1, 2024 at 3:21 PM Jiri Slaby <jirislaby at 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 at 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