linux-kernel - Re: [RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bvv98e.r4rs9b.2rw3hx-qmf@polaris.svanheule.net>
Date:   Mon, 27 Dec 2021 10:39:10 +0000
From:   Sander Vanheule <sander@...nheule.net>
To:     tglx@...utronix.de, maz@...nel.org, robh+dt@...nel.org,
        devicetree@...r.kernel.org, mail@...ger-koblitz.de
Cc:     linux-kernel@...r.kernel.org, bert@...t.com, john@...ozen.org
Subject: Re: [RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

Hi Birger,

On Monday, 27 December 2021, Birger Koblitz wrote:
> Hi,
> 
> I don't think the IRQ routing has an off-by one error. This was chosen
> by John to correspond to Realtek's own "documentation" and to
> take account of the special meaning of IRQs 0, 1 for VSMP and 6 and 7
> for the Realtek SoCs. In any case it would break the ABI as the meaning
> of these values changes and I don't think the change in range actually
> gives any additional functionality.

Realtek's SDK provides routing register values. I would have to check to see what CPU IRQs it then binds to, to service those interrupts. The binding wouldn't have to change, but we could fix the driver and devicetrees.

The binding specifies that interrupt-map provides a mapping of interrupt inputs to parent interrupt. The driver then takes these values, but doesn't check what the parent interrupt controller actually is, and finally assigns a chained handler to a hardware IRQ index (instead of a VIRQ).

You can try limiting the interrupt-map to only the mapping for UART0 with the current driver, and you will find that you end up with a broken system.

CPU IRQs 0 and 1 are indeed special (IPI for VSMP), yet we have interrupt mappings that contain <... &cpuintc 1>. Furthermore, if you specify a mapping of <... &cpuint 6> for an active interrupt source, you will get spurious timer (CPU IRQ 7) interrupts. This can't be correct. 

> With regards to the RTL8390, that SoC actually has two IRQ controllers
> to allow VSMP. The changes in parent routing have a good chance of breaking
> VSMP on the RTL8390 targets. Did you stress test this new logic under VSMP?

I haven't tested this with VSMP, because it is out of scope for this series. For the binding, I expect that would only require N register ranges instead of one; one per CPU. I think the driver should then be able to perform the IRQ balancing based on that information alone, given that the parent IRQs are available at each CPU.

Best,
Sander