linux-kernel - Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes

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: <5310b73b-4821-6dff-b9c0-34c59fb7fd72@arm.com>
Date:   Mon, 25 Feb 2019 14:52:58 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     Amitkumar Karwar <amitkarwar@...il.com>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Ganapathi Bhat <gbhat@...vell.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Kalle Valo <kvalo@...eaurora.org>,
        Nishant Sarmukadam <nishants@...vell.com>,
        Rob Herring <robh+dt@...nel.org>,
        Xinming Hu <huxinming820@...il.com>,
        Devicetree List <devicetree@...r.kernel.org>,
        "<netdev@...r.kernel.org>" <netdev@...r.kernel.org>,
        "<linux-wireless@...r.kernel.org>" <linux-wireless@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-rockchip@...ts.infradead.org,
        "David S. Miller" <davem@...emloft.net>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes

Hi Ard,

On 25/02/2019 12:45, Ard Biesheuvel wrote:
> On Sun, 24 Feb 2019 at 15:08, Marc Zyngier <marc.zyngier@....com> wrote:
>>
>> For quite some time, I wondered why the PCI mwifiex device built in my
>> Chromebook was unable to use the good old legacy interrupts. But as MSIs
>> were working fine, I never really bothered investigating. I finally had a
>> look, and the result isn't very pretty.
>>
>> On this machine (rk3399-based kevin), the wake-up interrupt is described as
>> such:
>>
>> &pci_rootport {
>>         mvl_wifi: wifi@0,0 {
>>                 compatible = "pci1b4b,2b42";
>>                 reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>>                        0x83010000 0x0 0x00100000 0x0 0x00100000>;
>>                 interrupt-parent = <&gpio0>;
>>                 interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>>                 pinctrl-names = "default";
>>                 pinctrl-0 = <&wlan_host_wake_l>;
>>                 wakeup-source;
>>         };
>> };
>>
>> Note how the interrupt is part of the properties directly attached to the
>> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
>> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
>> altogether (Yay for the broken design!). This is in total violation of the
>> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
>> specifiers describe the PCI device interrupts, and must obey the
>> INT-{A,B,C,D} mapping. Oops!
>>
> 
> The mapping of legacy PCIe INTx interrupts onto wired system
> interrupts is a property of the PCIe host controller, not of a
> particular PCIe device. So I would argue that the code is broken here
> as well: it should never attempt to interpret 'interrupt' properties
> at the PCI device level as having any bearing on how legacy interrupts
> are routed.

OpenFirmware says that this node contains the interrupt number of the
device (4.1.1. Open Firmware-defined Properties for Child Nodes). The
trick is that this property is generated *from* the device, and not set
in stone.

DT, on the other hand, takes whatever is described there and uses it as
the gospel to configure the OS, no matter how the PCI device is actually
configured. If the two don't match (like in this case), things break.
This is the "DT describes the HW" mantra, for (sometimes) better or
(more generally) worse.

What the DT code does is to interpret the whole interrupt specifier,
*including the interrupt-parent*. And that feels wrong. It should always
be in the context of the host controller. But on the other side, the DT
code is not in the business of validating the DT either...

It outlines one thing: If you have to interpret per-device PCI
properties from DT, you're in for serious trouble. I should get some
better HW.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...