Date:   Mon, 27 Dec 2021 10:16:01 +0000
Message-ID: <87tueuz732.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Sander Vanheule <sander@svanheule.net>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        Birger Koblitz <mail@birger-koblitz.de>,
        Bert Vermeulen <bert@biot.com>,
        John Crispin <john@phrozen.org>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 2/5] irqchip/realtek-rtl: fix off-by-one in routing
In-Reply-To: <2235a7748b8f7689a96b1e0f91461e36a946a4ef.1640548009.git.sander@svanheule.net>
References: <cover.1640548009.git.sander@svanheule.net>
        <2235a7748b8f7689a96b1e0f91461e36a946a4ef.1640548009.git.sander@svanheule.net>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.