linux-kernel - Re: [PATCH v3 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]

Message-ID: <9cbfdacba32c5e351fd9e14444768666@kernel.org>
Date:   Thu, 04 Jun 2020 11:11:34 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....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 v3 2/6] PCI: uniphier: Add misc interrupt handler to
 invoke PME and AER

On 2020-06-04 10:43, Kunihiko Hayashi wrote:

[...]

>>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp)
>>>  {
>>> -    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 (pci_msi_enabled()) {
>> 
>> This checks whether the kernel supports MSIs. Not that they are
>> enabled in your controller. Is that really what you want to do?
> 
> The below two status bits are valid when the interrupt for MSI is 
> asserted.
> That is, pci_msi_enabled() is wrong.
> 
> I'll modify the function to check the two bits only if this function is
> called from MSI handler.
> 
>> 
>>> +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
>>> +            dev_dbg(pci->dev, "Root Error Status\n");
>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>> +            generic_handle_irq(virq);
>>> +        }
>>> +
>>> +        if (val & PCL_CFG_PME_MSI_STATUS) {
>>> +            dev_dbg(pci->dev, "PME Interrupt\n");
>>> +            virq = irq_linear_revmap(pp->irq_domain, 0);
>>> +            generic_handle_irq(virq);
>>> +        }
>> 
>> These two cases do the exact same thing, calling the same interrupt.
>> What is the point of dealing with them independently?
> 
> Both PME and AER are asserted from MSI-0, and each handler checks its 
> own
> status bit in the PCIe register (aer_irq() in pcie/aer.c and 
> pcie_pme_irq()
> in pcie/pme.c).
> So I think this handler calls generic_handle_irq() for the same MSI-0.

So what is wrong with

         if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
                    PCL_CFG_PME_MSI_STATUS)) {
                 // handle interrupt
         }

?

If you have two handlers for the same interrupt, this is a shared
interrupt and each handler will be called in turn.

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