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

[Marc Zyngier]

Hi Ard,

On 25/02/2019 12:45, Ard Biesheuvel wrote:
> On Sun, 24 Feb 2019 at 15:08, Marc Zyngier <marc.zyngier@xxxxxxx> 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...