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

[Yu-Chien Peter Lin <peterlin@andestech.com>]

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