diff mbox series

[RFT] sh: mach-r2d: Handle virq offset in cascaded IRL demux

Message ID 2c99d5df41c40691f6c407b7b6a040d406bc81ac.1688901306.git.geert+renesas@glider.be
State New
Headers
Series [RFT] sh: mach-r2d: Handle virq offset in cascaded IRL demux |

Commit Message

Geert Uytterhoeven July 9, 2023, 11:15 a.m. UTC 
  When booting rts7751r2dplus_defconfig on QEMU, the system hangs due to
an interrupt storm on IRQ 20.  IRQ 20 aka event 0x280 is a cascaded IRL
interrupt, which maps to IRQ_VOYAGER, the interrupt used by the Silicon
Motion SM501 multimedia companion chip.  As rts7751r2d_irq_demux() does
not take into account the new virq offset, the interrupt is no longer
translated, leading to an unhandled interrupt.

Fix this by taking into account the virq offset when translating
cascaded IRL interrupts.

Fixes: a8ac2961148e8c72 ("sh: Avoid using IRQ0 on SH3 and SH4")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Closes: https://lore.kernel.org/r/fbfea3ad-d327-4ad5-ac9c-648c7ca3fe1f@roeck-us.net
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Highlander and Dreamcast probably have the same issue.
I'll send patches for these later...
---
 arch/sh/boards/mach-r2d/irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

John Paul Adrian Glaubitz July 9, 2023, 11:52 a.m. UTC | #1 
Hi Geert!

On Sun, 2023-07-09 at 13:15 +0200, Geert Uytterhoeven wrote:
> When booting rts7751r2dplus_defconfig on QEMU, the system hangs due to
> an interrupt storm on IRQ 20.  IRQ 20 aka event 0x280 is a cascaded IRL
> interrupt, which maps to IRQ_VOYAGER, the interrupt used by the Silicon
> Motion SM501 multimedia companion chip.  As rts7751r2d_irq_demux() does
> not take into account the new virq offset, the interrupt is no longer
> translated, leading to an unhandled interrupt.
> 
> Fix this by taking into account the virq offset when translating
> cascaded IRL interrupts.
> 
> Fixes: a8ac2961148e8c72 ("sh: Avoid using IRQ0 on SH3 and SH4")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/r/fbfea3ad-d327-4ad5-ac9c-648c7ca3fe1f@roeck-us.net
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Highlander and Dreamcast probably have the same issue.
> I'll send patches for these later...
> ---
>  arch/sh/boards/mach-r2d/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
> index e34f81e9ae813b8d..c37b40398c5bc83e 100644
> --- a/arch/sh/boards/mach-r2d/irq.c
> +++ b/arch/sh/boards/mach-r2d/irq.c
> @@ -117,10 +117,10 @@ static unsigned char irl2irq[R2D_NR_IRL];
>  
>  int rts7751r2d_irq_demux(int irq)
>  {
> -	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
> +	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
>  		return irq;
>  
> -	return irl2irq[irq];
> +	return irl2irq[irq - 16];
>  }
>  
>  /*

Funny, this is actually almost what I did myself when trying to fix this
issue. Only difference was that I applied the offset of 16 only to one
of the instances at a time and it never occurred to me that it needs to
be applied to all instances.

Thanks for fixing this!

Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Adrian
John Paul Adrian Glaubitz July 9, 2023, 11:53 a.m. UTC | #2 
Hi Geert!

On Sun, 2023-07-09 at 13:15 +0200, Geert Uytterhoeven wrote:
> When booting rts7751r2dplus_defconfig on QEMU, the system hangs due to
> an interrupt storm on IRQ 20.  IRQ 20 aka event 0x280 is a cascaded IRL
> interrupt, which maps to IRQ_VOYAGER, the interrupt used by the Silicon
> Motion SM501 multimedia companion chip.  As rts7751r2d_irq_demux() does
> not take into account the new virq offset, the interrupt is no longer
> translated, leading to an unhandled interrupt.
> 
> Fix this by taking into account the virq offset when translating
> cascaded IRL interrupts.
> 
> Fixes: a8ac2961148e8c72 ("sh: Avoid using IRQ0 on SH3 and SH4")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/r/fbfea3ad-d327-4ad5-ac9c-648c7ca3fe1f@roeck-us.net
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Highlander and Dreamcast probably have the same issue.
> I'll send patches for these later...
> ---
>  arch/sh/boards/mach-r2d/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
> index e34f81e9ae813b8d..c37b40398c5bc83e 100644
> --- a/arch/sh/boards/mach-r2d/irq.c
> +++ b/arch/sh/boards/mach-r2d/irq.c
> @@ -117,10 +117,10 @@ static unsigned char irl2irq[R2D_NR_IRL];
>  
>  int rts7751r2d_irq_demux(int irq)
>  {
> -	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
> +	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
>  		return irq;
>  
> -	return irl2irq[irq];
> +	return irl2irq[irq - 16];
>  }
>  
>  /*

I can also confirm that this fixes the hang on QEMU.

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Adrian
John Paul Adrian Glaubitz July 9, 2023, 11:58 a.m. UTC | #3 
Hi!

On Sun, 2023-07-09 at 13:15 +0200, Geert Uytterhoeven wrote:
> diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
> index e34f81e9ae813b8d..c37b40398c5bc83e 100644
> --- a/arch/sh/boards/mach-r2d/irq.c
> +++ b/arch/sh/boards/mach-r2d/irq.c
> @@ -117,10 +117,10 @@ static unsigned char irl2irq[R2D_NR_IRL];
>  
>  int rts7751r2d_irq_demux(int irq)
>  {
> -	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
> +	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
>  		return irq;
>  
> -	return irl2irq[irq];
> +	return irl2irq[irq - 16];
>  }
>  
>  /*

Btw, I think this needs to be adjusted to test for "ret <= 0" since IRQs cannot
be zero anymore, correct?

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/sm501.c#n1389

Adrian
Sergey Shtylyov July 9, 2023, 12:50 p.m. UTC | #4 
On 7/9/23 2:58 PM, John Paul Adrian Glaubitz wrote:
[...]
>> diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
>> index e34f81e9ae813b8d..c37b40398c5bc83e 100644
>> --- a/arch/sh/boards/mach-r2d/irq.c
>> +++ b/arch/sh/boards/mach-r2d/irq.c
>> @@ -117,10 +117,10 @@ static unsigned char irl2irq[R2D_NR_IRL];
>>  
>>  int rts7751r2d_irq_demux(int irq)
>>  {
>> -	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
>> +	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
>>  		return irq;
>>  
>> -	return irl2irq[irq];
>> +	return irl2irq[irq - 16];
>>  }
>>  
>>  /*
> 
> Btw, I think this needs to be adjusted to test for "ret <= 0" since IRQs cannot
> be zero anymore, correct?
> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/sm501.c#n1389

   No, just ignore IRQ0 from now on, it can't be returned. Else you'd just complicate
your code as you'd have to add a separate check for IRQ0 in order to return -EINVAL
in this case (you can't return 0 from probe in case of ret == 0 as that would mean
successful probe when it's not).  My patch to platfrom_get_irq() ensures that IRQ0
check in its users is never needed, in order to avoid the (badly scaling) checks)...

> Adrian

MBR, Sergey
Guenter Roeck July 9, 2023, 1:51 p.m. UTC | #5 
On Sun, Jul 09, 2023 at 01:15:49PM +0200, Geert Uytterhoeven wrote:
> When booting rts7751r2dplus_defconfig on QEMU, the system hangs due to
> an interrupt storm on IRQ 20.  IRQ 20 aka event 0x280 is a cascaded IRL
> interrupt, which maps to IRQ_VOYAGER, the interrupt used by the Silicon
> Motion SM501 multimedia companion chip.  As rts7751r2d_irq_demux() does
> not take into account the new virq offset, the interrupt is no longer
> translated, leading to an unhandled interrupt.
> 
> Fix this by taking into account the virq offset when translating
> cascaded IRL interrupts.
> 
> Fixes: a8ac2961148e8c72 ("sh: Avoid using IRQ0 on SH3 and SH4")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/r/fbfea3ad-d327-4ad5-ac9c-648c7ca3fe1f@roeck-us.net
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
> Highlander and Dreamcast probably have the same issue.
> I'll send patches for these later...
> ---
>  arch/sh/boards/mach-r2d/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
> index e34f81e9ae813b8d..c37b40398c5bc83e 100644
> --- a/arch/sh/boards/mach-r2d/irq.c
> +++ b/arch/sh/boards/mach-r2d/irq.c
> @@ -117,10 +117,10 @@ static unsigned char irl2irq[R2D_NR_IRL];
>  
>  int rts7751r2d_irq_demux(int irq)
>  {
> -	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
> +	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
>  		return irq;
>  
> -	return irl2irq[irq];
> +	return irl2irq[irq - 16];
>  }
>  
>  /*
> -- 
> 2.34.1
>
Guenter Roeck July 10, 2023, 1:13 a.m. UTC | #6 
On 7/9/23 04:15, Geert Uytterhoeven wrote:
> When booting rts7751r2dplus_defconfig on QEMU, the system hangs due to
> an interrupt storm on IRQ 20.  IRQ 20 aka event 0x280 is a cascaded IRL
> interrupt, which maps to IRQ_VOYAGER, the interrupt used by the Silicon
> Motion SM501 multimedia companion chip.  As rts7751r2d_irq_demux() does
> not take into account the new virq offset, the interrupt is no longer
> translated, leading to an unhandled interrupt.
> 
> Fix this by taking into account the virq offset when translating
> cascaded IRL interrupts.
> 
> Fixes: a8ac2961148e8c72 ("sh: Avoid using IRQ0 on SH3 and SH4")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/r/fbfea3ad-d327-4ad5-ac9c-648c7ca3fe1f@roeck-us.net
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Highlander and Dreamcast probably have the same issue.
> I'll send patches for these later...

dreamcast doesn't build in linux-next, just in case you didn't notice.

Guenter

> ---
>   arch/sh/boards/mach-r2d/irq.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
> index e34f81e9ae813b8d..c37b40398c5bc83e 100644
> --- a/arch/sh/boards/mach-r2d/irq.c
> +++ b/arch/sh/boards/mach-r2d/irq.c
> @@ -117,10 +117,10 @@ static unsigned char irl2irq[R2D_NR_IRL];
>   
>   int rts7751r2d_irq_demux(int irq)
>   {
> -	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
> +	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
>   		return irq;
>   
> -	return irl2irq[irq];
> +	return irl2irq[irq - 16];
>   }
>   
>   /*
Geert Uytterhoeven July 10, 2023, 7:03 a.m. UTC | #7 
Hi Günter,

On Mon, Jul 10, 2023 at 3:13 AM Guenter Roeck <linux@roeck-us.net> wrote:
> On 7/9/23 04:15, Geert Uytterhoeven wrote:
> > When booting rts7751r2dplus_defconfig on QEMU, the system hangs due to
> > an interrupt storm on IRQ 20.  IRQ 20 aka event 0x280 is a cascaded IRL
> > interrupt, which maps to IRQ_VOYAGER, the interrupt used by the Silicon
> > Motion SM501 multimedia companion chip.  As rts7751r2d_irq_demux() does
> > not take into account the new virq offset, the interrupt is no longer
> > translated, leading to an unhandled interrupt.
> >
> > Fix this by taking into account the virq offset when translating
> > cascaded IRL interrupts.
> >
> > Fixes: a8ac2961148e8c72 ("sh: Avoid using IRQ0 on SH3 and SH4")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Closes: https://lore.kernel.org/r/fbfea3ad-d327-4ad5-ac9c-648c7ca3fe1f@roeck-us.net
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Highlander and Dreamcast probably have the same issue.
> > I'll send patches for these later...
>
> dreamcast doesn't build in linux-next, just in case you didn't notice.

Indeed, I hadn't tested that.
My current tree isn't based on linux-next, but did have a build
failure in the cdrom code, for which I had found your fix (thanks!) in
linux-next...

Gr{oetje,eeting}s,

                        Geert
John Paul Adrian Glaubitz July 10, 2023, 7:23 a.m. UTC | #8 
Hi Guenter!

On Sun, 2023-07-09 at 18:13 -0700, Guenter Roeck wrote:
> dreamcast doesn't build in linux-next, just in case you didn't notice.

Didn't the person who introduced the regression a notification from the CI?

Maybe we could configure the CI to send a mail to the linux-sh mailing list
in case of such failures, so that the people involved in the SH port are
being notified.

Adrian
John Paul Adrian Glaubitz July 10, 2023, 7:44 a.m. UTC | #9 
Hi Geert!

On Mon, 2023-07-10 at 09:03 +0200, Geert Uytterhoeven wrote:
> Indeed, I hadn't tested that.
> My current tree isn't based on linux-next, but did have a build
> failure in the cdrom code, for which I had found your fix (thanks!) in
> linux-next...

So, there is a patch for this already? Is it going to be included for 6.5?

Adrian
Geert Uytterhoeven July 10, 2023, 7:54 a.m. UTC | #10 
Hi Adrian,

On Mon, Jul 10, 2023 at 9:44 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Mon, 2023-07-10 at 09:03 +0200, Geert Uytterhoeven wrote:
> > Indeed, I hadn't tested that.
> > My current tree isn't based on linux-next, but did have a build
> > failure in the cdrom code, for which I had found your fix (thanks!) in
> > linux-next...
>
> So, there is a patch for this already? Is it going to be included for 6.5?

The cdrom fix is commit a587b046ce921cc1 ("cdrom/gdrom: Fix build
error") in v6.5-rc1, which builds dreamcast_defconfig fine.

That config is still broken in linux-next, but the breakage hasn't\
entered v6.5-rc1 (yet?).

Gr{oetje,eeting}s,

                        Geert
John Paul Adrian Glaubitz July 10, 2023, 7:57 a.m. UTC | #11 
Hi Geert!

On Mon, 2023-07-10 at 09:54 +0200, Geert Uytterhoeven wrote:
> Hi Adrian,
> 
> On Mon, Jul 10, 2023 at 9:44 AM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
> > On Mon, 2023-07-10 at 09:03 +0200, Geert Uytterhoeven wrote:
> > > Indeed, I hadn't tested that.
> > > My current tree isn't based on linux-next, but did have a build
> > > failure in the cdrom code, for which I had found your fix (thanks!) in
> > > linux-next...
> > 
> > So, there is a patch for this already? Is it going to be included for 6.5?
> 
> The cdrom fix is commit a587b046ce921cc1 ("cdrom/gdrom: Fix build
> error") in v6.5-rc1, which builds dreamcast_defconfig fine.
> 
> That config is still broken in linux-next, but the breakage hasn't\
> entered v6.5-rc1 (yet?).

OK, so we're talking about two different regressions here?

Adrian
Geert Uytterhoeven July 10, 2023, 8:18 a.m. UTC | #12 
On Mon, Jul 10, 2023 at 9:57 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Mon, 2023-07-10 at 09:54 +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 10, 2023 at 9:44 AM John Paul Adrian Glaubitz
> > <glaubitz@physik.fu-berlin.de> wrote:
> > > On Mon, 2023-07-10 at 09:03 +0200, Geert Uytterhoeven wrote:
> > > > Indeed, I hadn't tested that.
> > > > My current tree isn't based on linux-next, but did have a build
> > > > failure in the cdrom code, for which I had found your fix (thanks!) in
> > > > linux-next...
> > >
> > > So, there is a patch for this already? Is it going to be included for 6.5?
> >
> > The cdrom fix is commit a587b046ce921cc1 ("cdrom/gdrom: Fix build
> > error") in v6.5-rc1, which builds dreamcast_defconfig fine.
> >
> > That config is still broken in linux-next, but the breakage hasn't\
> > entered v6.5-rc1 (yet?).
>
> OK, so we're talking about two different regressions here?

Yes.

https://lore.kernel.org/r/CAMuHMdWmv-Jdvi7a04JGXuA2QARj8c8mpUvY7TOcetPkG4pW7A@mail.gmail.com

Gr{oetje,eeting}s,

                        Geert
John Paul Adrian Glaubitz July 10, 2023, 8:20 a.m. UTC | #13 
Hi Geert!

On Mon, 2023-07-10 at 10:18 +0200, Geert Uytterhoeven wrote:
> > OK, so we're talking about two different regressions here?
> 
> Yes.
> 
> https://lore.kernel.org/r/CAMuHMdWmv-Jdvi7a04JGXuA2QARj8c8mpUvY7TOcetPkG4pW7A@mail.gmail.com

Link doesn't work for me, unfortunately.

Adrian
Geert Uytterhoeven July 10, 2023, 8:25 a.m. UTC | #14 
Hi Adrian,

On Mon, Jul 10, 2023 at 10:20 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Mon, 2023-07-10 at 10:18 +0200, Geert Uytterhoeven wrote:
> > > OK, so we're talking about two different regressions here?
> > https://lore.kernel.org/r/CAMuHMdWmv-Jdvi7a04JGXuA2QARj8c8mpUvY7TOcetPkG4pW7A@mail.gmail.com
>
> Link doesn't work for me, unfortunately.

-EAGAIN ;-)

Lore-archiving is not instantaneous. Please retry.

Gr{oetje,eeting}s,

                        Geert
John Paul Adrian Glaubitz July 13, 2023, 6:54 a.m. UTC | #15 
On Sun, 2023-07-09 at 13:15 +0200, Geert Uytterhoeven wrote:
> When booting rts7751r2dplus_defconfig on QEMU, the system hangs due to
> an interrupt storm on IRQ 20.  IRQ 20 aka event 0x280 is a cascaded IRL
> interrupt, which maps to IRQ_VOYAGER, the interrupt used by the Silicon
> Motion SM501 multimedia companion chip.  As rts7751r2d_irq_demux() does
> not take into account the new virq offset, the interrupt is no longer
> translated, leading to an unhandled interrupt.
> 
> Fix this by taking into account the virq offset when translating
> cascaded IRL interrupts.
> 
> Fixes: a8ac2961148e8c72 ("sh: Avoid using IRQ0 on SH3 and SH4")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/r/fbfea3ad-d327-4ad5-ac9c-648c7ca3fe1f@roeck-us.net
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Highlander and Dreamcast probably have the same issue.
> I'll send patches for these later...
> ---
>  arch/sh/boards/mach-r2d/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
> index e34f81e9ae813b8d..c37b40398c5bc83e 100644
> --- a/arch/sh/boards/mach-r2d/irq.c
> +++ b/arch/sh/boards/mach-r2d/irq.c
> @@ -117,10 +117,10 @@ static unsigned char irl2irq[R2D_NR_IRL];
>  
>  int rts7751r2d_irq_demux(int irq)
>  {
> -	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
> +	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
>  		return irq;
>  
> -	return irl2irq[irq];
> +	return irl2irq[irq - 16];
>  }
>  
>  /*

Applied to my for-linus branch.

Thanks,
Adrian
diff mbox series

Patch

diff --git a/arch/sh/boards/mach-r2d/irq.c b/arch/sh/boards/mach-r2d/irq.c
index e34f81e9ae813b8d..c37b40398c5bc83e 100644
--- a/arch/sh/boards/mach-r2d/irq.c
+++ b/arch/sh/boards/mach-r2d/irq.c
@@ -117,10 +117,10 @@  static unsigned char irl2irq[R2D_NR_IRL];
 
 int rts7751r2d_irq_demux(int irq)
 {
-	if (irq >= R2D_NR_IRL || irq < 0 || !irl2irq[irq])
+	if (irq >= 16 + R2D_NR_IRL || irq < 16 || !irl2irq[irq - 16])
 		return irq;
 
-	return irl2irq[irq];
+	return irl2irq[irq - 16];
 }
 
 /*