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

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

[Andy Chiu]

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(-)

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)
-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Jiri Slaby]

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)

-- 
js
suse labs


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [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@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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv