Date:   Tue, 28 Dec 2021 10:59:02 +0000
Message-ID: <87k0fpyozt.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: <48164a669fee54e2dc58cdbbde25c600d88d93f9.camel@svanheule.net>
References: <cover.1640548009.git.sander@svanheule.net>
        <2235a7748b8f7689a96b1e0f91461e36a946a4ef.1640548009.git.sander@svanheule.net>
        <87tueuz732.wl-maz@kernel.org>
        <48164a669fee54e2dc58cdbbde25c600d88d93f9.camel@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=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 28 Dec 2021 10:13:26 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
>=20
> Hi Marc,
>=20
> On Mon, 2021-12-27 at 10:16 +0000, Marc Zyngier wrote:
> > On Sun, 26 Dec 2021 19:59:25 +0000,
> > Sander Vanheule <sander@svanheule.net> wrote:
> > >=20
> > > There is an offset between routing values (1..6) and the connected MI=
PS
> > > CPU interrupts (2..7), but no distinction was made between these two
> > > values.
> > >=20
> > > 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.
> >=20
> > Where does this 'other routing' come from?
>=20
> When I reported the interrupt-map issue with this binding/driver, I
> tried to reduce the mapping/routing to something as small as
> possible, but I couldn't get away with anything less than the
> following:
>=20
> 	interrupt-map =3D
> 		<31 &cpuintc 2>, /* UART0 */
> 		<20 &cpuintc 3>, /* SWCORE */
> 		<19 &cpuintc 4>, /* WDT IP1 */
> 		<18 &cpuintc 5>; /* WDT IP2 */
>=20
> The UART0 and WDT_IP1 mappings were required. These would cause the
> driver to assign chained handlers to <&cpuint 2> and <&cpuint 4>,
> and also write values "2" and "4" to the respective routing
> registers.
>=20
> The SWCORE mapping was not required for any configured features, but
> it was required to get the console to work. This is because a
> routing register value of "2", actually causes that interrupt input
> to be (electrically) cascaded into to <&cpuintc 3>. But <&cpuintc 3>
> would only be assigned a chained handler because of the SWCORE
> mapping.
>=20
> By then assigning the same handler to all parent interrupts, and not
> caring about which parent interrupt caused the handler to be called,
> this ended up actually working.

Urgh... Pure luck, then.

>=20
> > >=20
> > > Offset the CPU IRQ numbers by -1 to retrieve the correct routing valu=
e.
> > >=20
> > > Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839=
x interrupt
> > > controller")
> > > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > > ---
> > > =C2=A0drivers/irqchip/irq-realtek-rtl.c | 8 +++++---
> > > =C2=A01 file changed, 5 insertions(+), 3 deletions(-)
> > >=20
> > > 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 *d=
esc)
> > > =C2=A0 * SoC interrupts are cascaded to MIPS CPU interrupts according=
 to the
> > > =C2=A0 * interrupt-map in the device tree. Each SoC interrupt gets 4 =
bits for
> > > =C2=A0 * the CPU interrupt in an Interrupt Routing Register. Max 32 S=
oC interrupts
> > > - * thus go into 4 IRRs.
> > > + * thus go into 4 IRRs. A routing value of '0' means the interrupt i=
s left
> > > + * disconnected. Routing values {1..15} connect to output lines {0..=
14}.
> > > =C2=A0 */
> > > =C2=A0static int __init map_interrupts(struct device_node *node, stru=
ct irq_domain *domain)
> > > =C2=A0{
> > > @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_no=
de *node, struct
> > > irq_domain *do
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0of_node_put(cpu_ictl);
> > > =C2=A0
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0cpu_int =3D be32_to_cpup(imap + 2);
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if (cpu_int > 7)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if (cpu_int > 7 || cpu_int < 2)
> >=20
> > How many output lines do you have? The comment above says something
> > about having 15 output lines, but you limit it to 7...
>=20
> The SoCs we are using with this interrupt controller, connect their
> output lines to CPU IRQ 2..7. If "interrupt-map" specifies parent HW
> IRQ numbers, the driver needs to verify those numbers are valid. If
> they are, it can derive the routing register values from that.
>=20
> On the other hand, routing register values have four bits. "0"
> appears to disconnect an input interrupt, so that leaves 15
> potential interrupt outputs (in theory, we don't have actual
> hardware descriptions). Only 6 outputs are used here, but that could
> be an implementation detail for these SoCs, rather than a limitation
> of the interrupt router.

It would be good to find out if there are more CPU interrupts that can
be targeted. My impression is that this is a copy/paste effect from
the original BSP, and that nobody actually checked. But maybe that's
the wrong impression.

>=20
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
return -EINVAL;
> > > =C2=A0
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!(mips_irqs_set & BIT(cpu_int))) {
> > > @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_no=
de *node, struct
> > > irq_domain *do
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
mips_irqs_set |=3D BIT(cpu_int);
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > > =C2=A0
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0regs[(soc_int * 4) / 32] |=3D cpu_int << (soc_int *=
 4) % 32;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0/* Use routing values (1..6) for CPU interrupts (2.=
.7) */
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0regs[(soc_int * 4) / 32] |=3D (cpu_int - 1) << (soc=
_int * 4) % 32;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0imap +=3D 3;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > > =C2=A0
> >=20
> > 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?
>=20
> The reason this worked, was due to a number of issues compensating
> for each other, like I tried to explain above.
>=20
> The mapping is indeed arbitrary, and not designed into the
> hardware. So it could (should?)  just be put in the driver, instead
> of the devicetree.

Indeed. Which is what I was advocating for the first place. What I
understand from the HW is that it is able to freely route any input to
any output (muxing them as required), and to map each output to any
CPU interrupt line.

If my understanding is correct, the only thing you need from the DT is
a description of which input an endpoint is targeting (the interrupt
specifier), and a list of CPU interrupt lines. Everything else can be
decided at runtime.

Anyway, I'll probably end-up queuing the first two patches for 5.17,
and a Cc: to stable. The rest we can discuss ad-nauseam.

	M.

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