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]

Message-ID: <20200710161420.GA9019@e121166-lin.cambridge.arm.com>
Date:   Fri, 10 Jul 2020 17:14:20 +0100
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.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

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.

> > 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.

Lorenzo