Re: [RFC PATCH v2 2/5] irqchip/realtek-rtl: fix off-by-one in routing

[Marc Zyngier <maz@kernel.org>]

On Sun, 26 Dec 2021 19:59:25 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
> 
> There is an offset between routing values (1..6) and the connected MIPS
> CPU interrupts (2..7), but no distinction was made between these two
> values.
> 
> This issue was previously hidden during testing, because an interrupt
> mapping was used where for each required interrupt another (unused)
> routing was configured, with an offset of +1.

Where does this 'other routing' come from?

> 
> Offset the CPU IRQ numbers by -1 to retrieve the correct routing value.
> 
> Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt controller")
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  drivers/irqchip/irq-realtek-rtl.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
> index d6788dd93c7b..568614edd88f 100644
> --- a/drivers/irqchip/irq-realtek-rtl.c
> +++ b/drivers/irqchip/irq-realtek-rtl.c
> @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *desc)
>   * SoC interrupts are cascaded to MIPS CPU interrupts according to the
>   * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for
>   * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts
> - * thus go into 4 IRRs.
> + * thus go into 4 IRRs. A routing value of '0' means the interrupt is left
> + * disconnected. Routing values {1..15} connect to output lines {0..14}.
>   */
>  static int __init map_interrupts(struct device_node *node, struct irq_domain *domain)
>  {
> @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
>  		of_node_put(cpu_ictl);
>  
>  		cpu_int = be32_to_cpup(imap + 2);
> -		if (cpu_int > 7)
> +		if (cpu_int > 7 || cpu_int < 2)

How many output lines do you have? The comment above says something
about having 15 output lines, but you limit it to 7...

>  			return -EINVAL;
>  
>  		if (!(mips_irqs_set & BIT(cpu_int))) {
> @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
>  			mips_irqs_set |= BIT(cpu_int);
>  		}
>  
> -		regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
> +		/* Use routing values (1..6) for CPU interrupts (2..7) */
> +		regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
>  		imap += 3;
>  	}
>  

What I don't understand is how this worked so far if all mappings were
off my one. Or the mapping really doesn't matter, because this is all
under SW control?

	M.

-- 
Without deviation from the norm, progress is not possible.