Re: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

From: Marc Zyngier
Date: Thu Jun 04 2020 - 06:11:39 EST


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