Re: [PATCH v2 14/16] xen/riscv: add external interrupt handling for hypervisor mode

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/15/25 11:54 AM, Jan Beulich wrote:

> On 06.05.2025 18:51, Oleksii Kurochko wrote:
> > +static void cf_check aplic_set_irq_type(struct irq_desc *desc, unsigned int type)
> > +{
> > +    /*
> > +    * Interrupt 0 isn't possible based on the spec:
> > +    *   Each of an APLIC’s interrupt sources has a fixed unique identity number in the range 1 to N,
> > +    *   where N is the total number of sources at the APLIC. The number zero is not a valid interrupt
> > +    *   identity number at an APLIC. The maximum number of interrupt sources an APLIC may support
> > +    *   is 1023.
> > +    *
> > +    * Thereby interrupt 1 will correspond to bit 0 in sourcecfg[] register,
> > +    * interrupt 2 ->sourcecfg[1] and so on.
> > +    *
> > +    * And that is the reason why we need -1.
> > +    */
> > +    unsigned int irq_bit = desc->irq - 1;
> > +
> > +    spin_lock(&aplic.lock);
> > +
> > +    switch(type)
> >       
> Nit: style
>
> > +    {
> > +    case IRQ_TYPE_EDGE_RISING:
> > +        writel(APLIC_SOURCECFG_SM_EDGE_RISE, &aplic.regs->sourcecfg[irq_bit]);
> > +        break;
> > +
> > +    case IRQ_TYPE_EDGE_FALLING:
> > +        writel(APLIC_SOURCECFG_SM_EDGE_FALL, &aplic.regs->sourcecfg[irq_bit]);
> > +        break;
> > +
> > +    case IRQ_TYPE_LEVEL_HIGH:
> > +        writel(APLIC_SOURCECFG_SM_LEVEL_HIGH, &aplic.regs->sourcecfg[irq_bit]);
> > +        break;
> > +
> > +    case IRQ_TYPE_LEVEL_LOW:
> > +        writel(APLIC_SOURCECFG_SM_LEVEL_LOW, &aplic.regs->sourcecfg[irq_bit]);
> > +        break;
> > +
> > +    case IRQ_TYPE_NONE:
> > +        fallthrough;
> >       
> This is pointless (and hampering readability) when there are no other statements.
>     
Oh, okay, it should be:
  case IRQ_TYPE_NONE:
  case IRQ_TYPE_INVALID:
I thought fallthrough should be used always in such cases.
Anyway, I'll drop it.

> With both taken care of:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>     
Thanks.

I am going also to add "ASSERT(spin_is_locked(&desc->lock));" at the start of this
function to be algined with other callbacks which uses spin_{un}lock(&aplic.lock).
~ Oleksii