linux-kernel - Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

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]
Date:   Tue, 14 Jul 2020 18:27:24 +0900
From:   Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     Marc Zyngier <maz@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Rob Herring <robh+dt@...nel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        Jassi Brar <jaswinder.singh@...aro.org>
Subject: Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to
 invoke PME and AER

Hi Lorenzo,

On 2020/07/11 1:14, Lorenzo Pieralisi wrote:
> On Wed, Jul 01, 2020 at 11:18:29AM +0900, Kunihiko Hayashi wrote:
> 
> [...]
> 
>>>>>>    -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>>>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>>>>>>    {
>>>>>> -    struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>>>>>        struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>>>        struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>>>>>> -    struct irq_chip *chip = irq_desc_get_chip(desc);
>>>>>> -    unsigned long reg;
>>>>>> -    u32 val, bit, virq;
>>>>>> +    u32 val, virq;
>>>>>>    -    /* INT for debug */
>>>>>>        val = readl(priv->base + PCL_RCV_INT);
>>>>>>          if (val & PCL_CFG_BW_MGT_STATUS)
>>>>>>            dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>>>>>> +
>>>>>>        if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>>>>>            dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>>>>>> -    if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>>>> -        dev_dbg(pci->dev, "Root Error\n");
>>>>>> -    if (val & PCL_CFG_PME_MSI_STATUS)
>>>>>> -        dev_dbg(pci->dev, "PME Interrupt\n");
>>>>>> +
>>>>>> +    if (is_msi) {
>>>>>> +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>>>> +            dev_dbg(pci->dev, "Root Error Status\n");
>>>>>> +
>>>>>> +        if (val & PCL_CFG_PME_MSI_STATUS)
>>>>>> +            dev_dbg(pci->dev, "PME Interrupt\n");
>>>>>> +
>>>>>> +        if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>>>>>> +               PCL_CFG_PME_MSI_STATUS)) {
>>>>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>>>>> +            generic_handle_irq(virq);
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> Please have two handlers: one for interrupts that are from the RC,
>>>>> another for interrupts coming from the endpoints.
>>>> I assume that this handler treats interrupts from the RC only and
>>>> this is set on the member ".msi_host_isr" added in the patch 1/6.
>>>> I think that the handler for interrupts coming from endpoints should be
>>>> treated as a normal case (after calling .msi_host_isr in
>>>> dw_handle_msi_irq()).
>>>
>>> It looks pretty odd that you end-up dealing with both from the
>>> same "parent" interrupt. I guess this is in keeping with the
>>> rest of the DW PCIe hacks... :-/
>>
>> It might be odd, however, in case of UniPhier SoC,
>> both MSI interrupts from endpoints and PME/AER interrupts from RC are
>> asserted by same "parent" interrupt. In other words, PME/AER interrupts
>> are notified using the parent interrupt for MSI.
>>
>> MSI interrupts are treated as child interrupts with reference to
>> the status register in DW core. This is handled in a for-loop in
>> dw_handle_msi_irq().
>>
>> PME/AER interrupts are treated with reference to the status register
>> in UniPhier glue layer, however, this couldn't be handled in the same way
>> directly.
>>
>> So I'm trying to add .msi_host_isr function to handle this
>> with reference to the SoC-specific registers.
>>
>> This exported function asserts MSI-0 as a shared child interrupt.
>> As a result, PME/AER are registered like the followings in dmesg:
>>
>>     pcieport 0000:00:00.0: PME: Signaling with IRQ 25
>>     pcieport 0000:00:00.0: AER: enabled with IRQ 25
>>
>> And these interrupts are shared as MSI-0:
>>
>>     # cat /proc/interrupts | grep 25:
>>      25:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
>>
>> This might be a special case, though, I think that this is needed to handle
>> interrupts from RC sharing MSI parent.
> 
> Can you please send me (with this series *applied* of course and if
> possible with an endpoint MSI/MSI-X capable enabled):
> 
> - full dmesg log
> - lspci -vv output
> - cat /proc/interrupts
> 
> I need to understand how this system HW works before commenting any
> further.

Okay, I attach all the log to this mail.

This controller has MSI support only, and I attached R8169 ethernet card
to PCIe slot to enable the controller.

> 
>>> It is for Lorenzo to make up his mind about this anyway.
>>
>> I'd like to Lorenzo's opinion, too.
> 
> I am trying to understand how the HW is wired up (and that's what Marc
> asked as well) so first things first, please send the logs.

I hope this will help your understanding.

Thank you,

---
Best Regards
Kunihiko Hayashi

View attachment "pcie-uniphier.log" of type "text/plain" (29823 bytes)