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

[Anup Patel]

On Wed, Dec 13, 2023 at 12:34 PM Yu Chien Peter Lin
<peterlin@xxxxxxxxxxxxx> 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 <peterlin@xxxxxxxxxxxxx>
> Reviewed-by: Charles Ci-Jyun Wu <dminus@xxxxxxxxxxxxx>
> Reviewed-by: Leo Yu-Chi Liang <ycliang@xxxxxxxxxxxxx>
> ---
> 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/20231023004100.2663486-3-peterlin@xxxxxxxxxxxxx/#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
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv