[v5,02/16] irqchip/riscv-intc: Allow large non-standard interrupt number

Commit Message

Yu Chien Peter Lin Dec. 13, 2023, 7:02 a.m. UTC

Currently, the implementation of the RISC-V INTC driver uses the
interrupt cause as hardware interrupt number and has a limitation of
supporting a maximum of 64 interrupts. However, according to the
privileged spec, interrupt causes >= 16 are defined for platform use.

This limitation prevents to fully utilize the available local interrupt
sources. Additionally, the interrupt number used on RISC-V are sparse,
with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU
interrupt) being currently used for supervisor mode.

Switch to using irq_domain_create_tree() to create the radix tree
map, so a larger number of hardware interrupts can be handled.

Signed-off-by: Yu Chien Peter Lin <[e-mail]>
Reviewed-by: Charles Ci-Jyun Wu <[e-mail]>
Reviewed-by: Leo Yu-Chi Liang <[e-mail]>
---
Changes v1 -> v2:
  - Fixed irq mapping failure checking (suggested by Clément and Anup)
Changes v2 -> v3:
  - No change
Changes v3 -> v4: (Suggested by Thomas [1])
  - Use pr_warn_ratelimited instead
  - Fix coding style and commit message
Changes v4 -> v5: (Suggested by Thomas)
  - Fix commit message

[1] https://patchwork.kernel.org/project/linux-riscv/patch/[e-mail]/#25573085
---
 drivers/irqchip/irq-riscv-intc.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Anup Patel Dec. 13, 2023, 2:28 p.m. UTC | #1

On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin
<[e-mail]> wrote:
>
> Currently, the implementation of the RISC-V INTC driver uses the
> interrupt cause as hardware interrupt number and has a limitation of
> supporting a maximum of 64 interrupts. However, according to the
> privileged spec, interrupt causes >= 16 are defined for platform use.

I disagree with this patch.

Even though RISC-V priv sepc allows interrupt causes >= 16, we
still need CSRs to manage arbitrary local interrupts

Currently, we have following standard CSRs:
1) [m|s]ie and [m|s]ip which are XLEN wide
2) With AIA, we have [m|s]ieh and [m|s]iph for RV32

Clearly, we can only have a XLEN number of standard local
interrupts without AIA and 64 local interrupts with AIA.

Now for implementations with custom CSRs (such as Andes),
we still can't assume infinite local interrupts because HW will
have a finite number of custom CSRs.

>
> This limitation prevents to fully utilize the available local interrupt
> sources. Additionally, the interrupt number used on RISC-V are sparse,
> with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU
> interrupt) being currently used for supervisor mode.
>
> Switch to using irq_domain_create_tree() to create the radix tree
> map, so a larger number of hardware interrupts can be handled.
>
> Signed-off-by: Yu Chien Peter Lin <[e-mail]>
> Reviewed-by: Charles Ci-Jyun Wu <[e-mail]>
> Reviewed-by: Leo Yu-Chi Liang <[e-mail]>
> ---
> Changes v1 -> v2:
>   - Fixed irq mapping failure checking (suggested by Clément and Anup)
> Changes v2 -> v3:
>   - No change
> Changes v3 -> v4: (Suggested by Thomas [1])
>   - Use pr_warn_ratelimited instead
>   - Fix coding style and commit message
> Changes v4 -> v5: (Suggested by Thomas)
>   - Fix commit message
>
> [1] https://patchwork.kernel.org/project/linux-riscv/patch/[e-mail]/#25573085
> ---
>  drivers/irqchip/irq-riscv-intc.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index e8d01b14ccdd..2fdd40f2a791 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -24,10 +24,9 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
>  {
>         unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG;
>
> -       if (unlikely(cause >= BITS_PER_LONG))
> -               panic("unexpected interrupt cause");
> -
> -       generic_handle_domain_irq(intc_domain, cause);
> +       if (generic_handle_domain_irq(intc_domain, cause))
> +               pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n",
> +                                   cause);
>  }
>
>  /*
> @@ -117,8 +116,7 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
>  {
>         int rc;
>
> -       intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> -                                              &riscv_intc_domain_ops, NULL);
> +       intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL);

I disagree with this change based on the reasoning above.

Instead of this, we should determine the number of local interrupts
based on the type of RISC-V intc:
1) For standard INTC without AIA, we have XLEN (or BITS_PER_LONG)
    local interrupts
2) For standart INTC with AIA, we have 64 local interrupts
3) For custom INTC (such as Andes), the number of local interrupt
    should be custom (Andes specific) which can be determined based
    on compatible string.

Also, creating a linear domain with a fixed number of local interrupts
ensures that drivers can't map a local interrupt beyond the availability
of CSRs to manage it.

>         if (!intc_domain) {
>                 pr_err("unable to add IRQ domain\n");
>                 return -ENXIO;
> @@ -132,8 +130,6 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
>
>         riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
>
> -       pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> -

Same as above, we should definitely advertise the type of INTC and
number of local interrupts mapped.

Regards,
Anup

>         return 0;
>  }
>
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Anup Patel Dec. 13, 2023, 3:19 p.m. UTC | #2

On Wed, Dec 13, 2023 at 7:58 PM Anup Patel <[e-mail]> wrote:
>
> On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin
> <[e-mail]> wrote:
> >
> > Currently, the implementation of the RISC-V INTC driver uses the
> > interrupt cause as hardware interrupt number and has a limitation of
> > supporting a maximum of 64 interrupts. However, according to the
> > privileged spec, interrupt causes >= 16 are defined for platform use.
>
> I disagree with this patch.
>
> Even though RISC-V priv sepc allows interrupt causes >= 16, we
> still need CSRs to manage arbitrary local interrupts
>
> Currently, we have following standard CSRs:
> 1) [m|s]ie and [m|s]ip which are XLEN wide
> 2) With AIA, we have [m|s]ieh and [m|s]iph for RV32
>
> Clearly, we can only have a XLEN number of standard local
> interrupts without AIA and 64 local interrupts with AIA.
>
> Now for implementations with custom CSRs (such as Andes),
> we still can't assume infinite local interrupts because HW will
> have a finite number of custom CSRs.
>
> >
> > This limitation prevents to fully utilize the available local interrupt
> > sources. Additionally, the interrupt number used on RISC-V are sparse,
> > with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU
> > interrupt) being currently used for supervisor mode.
> >
> > Switch to using irq_domain_create_tree() to create the radix tree
> > map, so a larger number of hardware interrupts can be handled.
> >
> > Signed-off-by: Yu Chien Peter Lin <[e-mail]>
> > Reviewed-by: Charles Ci-Jyun Wu <[e-mail]>
> > Reviewed-by: Leo Yu-Chi Liang <[e-mail]>
> > ---
> > Changes v1 -> v2:
> >   - Fixed irq mapping failure checking (suggested by Clément and Anup)
> > Changes v2 -> v3:
> >   - No change
> > Changes v3 -> v4: (Suggested by Thomas [1])
> >   - Use pr_warn_ratelimited instead
> >   - Fix coding style and commit message
> > Changes v4 -> v5: (Suggested by Thomas)
> >   - Fix commit message
> >
> > [1] https://patchwork.kernel.org/project/linux-riscv/patch/[e-mail]/#25573085
> > ---
> >  drivers/irqchip/irq-riscv-intc.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > index e8d01b14ccdd..2fdd40f2a791 100644
> > --- a/drivers/irqchip/irq-riscv-intc.c
> > +++ b/drivers/irqchip/irq-riscv-intc.c
> > @@ -24,10 +24,9 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
> >  {
> >         unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG;
> >
> > -       if (unlikely(cause >= BITS_PER_LONG))
> > -               panic("unexpected interrupt cause");
> > -
> > -       generic_handle_domain_irq(intc_domain, cause);
> > +       if (generic_handle_domain_irq(intc_domain, cause))
> > +               pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n",
> > +                                   cause);
> >  }
> >
> >  /*
> > @@ -117,8 +116,7 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> >  {
> >         int rc;
> >
> > -       intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> > -                                              &riscv_intc_domain_ops, NULL);
> > +       intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL);
>
> I disagree with this change based on the reasoning above.
>
> Instead of this, we should determine the number of local interrupts
> based on the type of RISC-V intc:
> 1) For standard INTC without AIA, we have XLEN (or BITS_PER_LONG)
>     local interrupts
> 2) For standart INTC with AIA, we have 64 local interrupts
> 3) For custom INTC (such as Andes), the number of local interrupt
>     should be custom (Andes specific) which can be determined based
>     on compatible string.
>
> Also, creating a linear domain with a fixed number of local interrupts
> ensures that drivers can't map a local interrupt beyond the availability
> of CSRs to manage it.

Thinking about this more. We do have a problem because Andes local
interrupts are really sparse which is not the case for standard local
interrupts

I have an alternate suggestion which goes as follows ...

We use irq_domain_create_tree() in-place of irq_domain_create_linear()
and enforce checks on hwirq in riscv_intc_domain_alloc() to ensure that
we only allow hwirq for which we have corresponding standard or custom
CSR.

To achieve this, riscv_intc_init_common() will have to save the following
as static global variables:
1) riscv_intc_nr_irqs: Number of standard local interrupts
2) riscv_intc_custom_base and riscv_intc_custom_nr_irqs: Base and
    number of custom local interrupts.

Using the above static global variables, the riscv_intc_domain_alloc()
can return error if one of the following conditions are met:
1) riscv_intc_nr_irqs<= hwirq && hwirq < riscv_intc_custom_base
2) (riscv_intc_custom_base + riscv_intc_custom_nr_irqs) <= hwirq

For standard INTC, we can set the static global variable as follows:
riscv_intc_nr_irqs = XLEN or BITS_PER_LONG
riscv_intc_custom_base = riscv_intc_nr_irqs
riscv_intc_custom_nr_irqs = 0

For Andes INTC, we can set the static global variables as follows:
riscv_intc_nr_irqs = XLEN or BITS_PER_LONG
riscv_intc_custom_base = 256
riscv_intc_custom_nr_irqs = XLEN or BITS_PER_LONG

Regards,
Anup

>
> >         if (!intc_domain) {
> >                 pr_err("unable to add IRQ domain\n");
> >                 return -ENXIO;
> > @@ -132,8 +130,6 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> >
> >         riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> >
> > -       pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> > -
>
> Same as above, we should definitely advertise the type of INTC and
> number of local interrupts mapped.
>
> Regards,
> Anup
>
> >         return 0;
> >  }
> >
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Yu Chien Peter Lin Dec. 19, 2023, 7:43 a.m. UTC | #3

Hi Anup,

On Wed, Dec 13, 2023 at 08:49:23PM +0530, Anup Patel wrote:
> On Wed, Dec 13, 2023 at 7:58 PM Anup Patel <[e-mail]> wrote:
> >
> > On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin
> > <[e-mail]> wrote:
> > >
> > > Currently, the implementation of the RISC-V INTC driver uses the
> > > interrupt cause as hardware interrupt number and has a limitation of
> > > supporting a maximum of 64 interrupts. However, according to the
> > > privileged spec, interrupt causes >= 16 are defined for platform use.
> >
> > I disagree with this patch.
> >
> > Even though RISC-V priv sepc allows interrupt causes >= 16, we
> > still need CSRs to manage arbitrary local interrupts
> >
> > Currently, we have following standard CSRs:
> > 1) [m|s]ie and [m|s]ip which are XLEN wide
> > 2) With AIA, we have [m|s]ieh and [m|s]iph for RV32
> >
> > Clearly, we can only have a XLEN number of standard local
> > interrupts without AIA and 64 local interrupts with AIA.
> >
> > Now for implementations with custom CSRs (such as Andes),
> > we still can't assume infinite local interrupts because HW will
> > have a finite number of custom CSRs.
> >
> > >
> > > This limitation prevents to fully utilize the available local interrupt
> > > sources. Additionally, the interrupt number used on RISC-V are sparse,
> > > with only interrupt numbers 1, 5 and 9 (plus Sscofpmf or T-Head's PMU
> > > interrupt) being currently used for supervisor mode.
> > >
> > > Switch to using irq_domain_create_tree() to create the radix tree
> > > map, so a larger number of hardware interrupts can be handled.
> > >
> > > Signed-off-by: Yu Chien Peter Lin <[e-mail]>
> > > Reviewed-by: Charles Ci-Jyun Wu <[e-mail]>
> > > Reviewed-by: Leo Yu-Chi Liang <[e-mail]>
> > > ---
> > > Changes v1 -> v2:
> > >   - Fixed irq mapping failure checking (suggested by Clément and Anup)
> > > Changes v2 -> v3:
> > >   - No change
> > > Changes v3 -> v4: (Suggested by Thomas [1])
> > >   - Use pr_warn_ratelimited instead
> > >   - Fix coding style and commit message
> > > Changes v4 -> v5: (Suggested by Thomas)
> > >   - Fix commit message
> > >
> > > [1] https://patchwork.kernel.org/project/linux-riscv/patch/[e-mail]/#25573085
> > > ---
> > >  drivers/irqchip/irq-riscv-intc.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > > index e8d01b14ccdd..2fdd40f2a791 100644
> > > --- a/drivers/irqchip/irq-riscv-intc.c
> > > +++ b/drivers/irqchip/irq-riscv-intc.c
> > > @@ -24,10 +24,9 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
> > >  {
> > >         unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG;
> > >
> > > -       if (unlikely(cause >= BITS_PER_LONG))
> > > -               panic("unexpected interrupt cause");
> > > -
> > > -       generic_handle_domain_irq(intc_domain, cause);
> > > +       if (generic_handle_domain_irq(intc_domain, cause))
> > > +               pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n",
> > > +                                   cause);
> > >  }
> > >
> > >  /*
> > > @@ -117,8 +116,7 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> > >  {
> > >         int rc;
> > >
> > > -       intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> > > -                                              &riscv_intc_domain_ops, NULL);
> > > +       intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL);
> >
> > I disagree with this change based on the reasoning above.
> >
> > Instead of this, we should determine the number of local interrupts
> > based on the type of RISC-V intc:
> > 1) For standard INTC without AIA, we have XLEN (or BITS_PER_LONG)
> >     local interrupts
> > 2) For standart INTC with AIA, we have 64 local interrupts
> > 3) For custom INTC (such as Andes), the number of local interrupt
> >     should be custom (Andes specific) which can be determined based
> >     on compatible string.
> >
> > Also, creating a linear domain with a fixed number of local interrupts
> > ensures that drivers can't map a local interrupt beyond the availability
> > of CSRs to manage it.
> 
> Thinking about this more. We do have a problem because Andes local
> interrupts are really sparse which is not the case for standard local
> interrupts
> 
> I have an alternate suggestion which goes as follows ...
> 
> We use irq_domain_create_tree() in-place of irq_domain_create_linear()
> and enforce checks on hwirq in riscv_intc_domain_alloc() to ensure that
> we only allow hwirq for which we have corresponding standard or custom
> CSR.
> 
> To achieve this, riscv_intc_init_common() will have to save the following
> as static global variables:
> 1) riscv_intc_nr_irqs: Number of standard local interrupts
> 2) riscv_intc_custom_base and riscv_intc_custom_nr_irqs: Base and
>     number of custom local interrupts.
> 
> Using the above static global variables, the riscv_intc_domain_alloc()
> can return error if one of the following conditions are met:
> 1) riscv_intc_nr_irqs<= hwirq && hwirq < riscv_intc_custom_base
> 2) (riscv_intc_custom_base + riscv_intc_custom_nr_irqs) <= hwirq
> 
> For standard INTC, we can set the static global variable as follows:
> riscv_intc_nr_irqs = XLEN or BITS_PER_LONG
> riscv_intc_custom_base = riscv_intc_nr_irqs
> riscv_intc_custom_nr_irqs = 0
> 
> For Andes INTC, we can set the static global variables as follows:
> riscv_intc_nr_irqs = XLEN or BITS_PER_LONG
> riscv_intc_custom_base = 256
> riscv_intc_custom_nr_irqs = XLEN or BITS_PER_LONG
> 
> Regards,
> Anup

Thank you for offering your help on this.
I will rework the patch accordingly.

Best regards,
Peter Lin

> >
> > >         if (!intc_domain) {
> > >                 pr_err("unable to add IRQ domain\n");
> > >                 return -ENXIO;
> > > @@ -132,8 +130,6 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> > >
> > >         riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> > >
> > > -       pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> > > -
> >
> > Same as above, we should definitely advertise the type of INTC and
> > number of local interrupts mapped.
> >
> > Regards,
> > Anup
> >
> > >         return 0;
> > >  }
> > >
> > > --
> > > 2.34.1
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [e-mail]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Patch

diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
index e8d01b14ccdd..2fdd40f2a791 100644
--- a/drivers/irqchip/irq-riscv-intc.c
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -24,10 +24,9 @@  static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
 {
 	unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG;
 
-	if (unlikely(cause >= BITS_PER_LONG))
-		panic("unexpected interrupt cause");
-
-	generic_handle_domain_irq(intc_domain, cause);
+	if (generic_handle_domain_irq(intc_domain, cause))
+		pr_warn_ratelimited("Failed to handle interrupt (cause: %ld)\n",
+				    cause);
 }
 
 /*
@@ -117,8 +116,7 @@  static int __init riscv_intc_init_common(struct fwnode_handle *fn)
 {
 	int rc;
 
-	intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
-					       &riscv_intc_domain_ops, NULL);
+	intc_domain = irq_domain_create_tree(fn, &riscv_intc_domain_ops, NULL);
 	if (!intc_domain) {
 		pr_err("unable to add IRQ domain\n");
 		return -ENXIO;
@@ -132,8 +130,6 @@  static int __init riscv_intc_init_common(struct fwnode_handle *fn)
 
 	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
 
-	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
-
 	return 0;
 }