Re: [PATCH 0/3] PCI: designware: Fixing MSI handling flow

[Trent Piepho <tpiepho@impinj.com>]

On Wed, 2018-11-14 at 22:01 +0000, Marc Zyngier wrote:
> On Wed, 14 Nov 2018 19:19:27 +0000,
> Trent Piepho <tpiepho@impinj.com> wrote:
> > 
> > On Wed, 2018-11-14 at 09:54 +0000, Marc Zyngier wrote:
> > >         /* Initialize IRQ Status array */
> > > -       for (ctrl = 0; ctrl < num_ctrls; ctrl++)
> > > -               dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
> > > +       for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> > > +               dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK +
> > >                                         (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> > > -                                   4, &pp->irq_status[ctrl]);
> > > +                                   4, ~0);
> > > +               dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
> > > +                                       (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> > > +                                   4, ~0);
> > > +               pp->irq_status[ctrl] = 0;
> > > +       }
> > >  
> > 
> > I tested yesterday before this patch was sent and fixed this issue
> > another way.  I pretty sure this would work as well, though it's not
> > clear to me it's more correct.
> 
> Given that we don't have a spec or any form of useful documentation
> (except for the information that Gustavo gave us), I don't think
> *anything* we'll write here has a remote chance of being
> correct. We're simply poking in the dark.

Should all MSIs start enabled, or should they start disabled and be
enabled via the irq_enable method of the irq_chip, seems like a Linux
design decision to me.  Decide that, then try to figure out how to make
the hardware do what Linux expects it to do.

Starting disabled seems like the right design to me.  So here's my
attempt to make the driver do this.  Works in my tests.  I've not
tracked down all uses of irq_status outside the driver to determine how
it's supposed to work.

From dfc015f9821f5105cbcf9686d360105ffbac4ffb Mon Sep 17 00:00:00 2001
From: Trent Piepho <tpiepho@impinj.com>
Date: Wed, 14 Nov 2018 14:12:47 -0800
Subject: [PATCH] PCI: dwc: Allow enabling MSIs and start with disabled

Add irq_enable callbacks to let MSIs be enabled.

Previously the driver would leave any MSIs enabled when it initialized
that way.  Rather than that, disable them all.

Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 37
++++++++++++++++++++---
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index f06e67c60593..e7770fb1ced8 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -61,11 +61,17 @@ static void dw_msi_unmask_irq(struct irq_data *d)
 	irq_chip_unmask_parent(d);
 }
 
+static void dw_msi_enable_irq(struct irq_data *d)
+{
+	irq_chip_enable_parent(d);
+}
+
 static struct irq_chip dw_pcie_msi_irq_chip = {
 	.name = "PCI-MSI",
 	.irq_ack = dw_msi_ack_irq,
 	.irq_mask = dw_msi_mask_irq,
 	.irq_unmask = dw_msi_unmask_irq,
+	.irq_enable = dw_msi_enable_irq,
 };
 
 static struct msi_domain_info dw_pcie_msi_domain_info = {
@@ -215,6 +221,26 @@ static void dw_pci_bottom_ack(struct irq_data *d)
 	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
+static void dw_pci_bottom_enable(struct irq_data *data)
+{
+	struct pcie_port *pp = irq_data_get_irq_chip_data(data);
+	unsigned int res, bit, ctrl;
+	unsigned long flags;
+	u32 enable;
+
+	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
+	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
+	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
+
+	raw_spin_lock_irqsave(&pp->lock, flags);
+
+	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
&enable);
+	enable |= BIT(bit);
+	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
enable);
+
+	raw_spin_unlock_irqrestore(&pp->lock, flags);
+}
+
 static struct irq_chip dw_pci_msi_bottom_irq_chip = {
 	.name = "DWPCI-MSI",
 	.irq_ack = dw_pci_bottom_ack,
@@ -222,6 +248,7 @@ static struct irq_chip dw_pci_msi_bottom_irq_chip =
{
 	.irq_set_affinity = dw_pci_msi_set_affinity,
 	.irq_mask = dw_pci_bottom_mask,
 	.irq_unmask = dw_pci_bottom_unmask,
+	.irq_enable = dw_pci_bottom_enable,
 };
 
 static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
@@ -663,11 +690,13 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
 
-	/* Initialize IRQ Status array */
-	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
-		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
+	/* Disable all MSIs and initialize IRQ Status array */
+	for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
+		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
 					(ctrl *
MSI_REG_CTRL_BLOCK_SIZE),
-				    4, &pp->irq_status[ctrl]);
+				    4, 0);
+		pp->irq_status[ctrl] = 0;
+	}
 
 	/* Setup RC BARs */
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004);
-- 
2.14.4


> > I speculated that the previous behavior was trying to work with an MSI
> > enabled by the bootloader, ACPI firmware, etc. that should be left
> > alone.  Or perhaps there was no good reason not to disable everything
> > on initialization and that code just got copied from somewhere else and
> > no one thought about it.  There's certainly evidence of that in this
> > driver.
> 
> As you said, you're speculating. Nonetheless, there is no reason to
> start with anything enabled the first place.

Normally I would have dived into the git history before sending a
patch, to see if I could find the source of that behavior:
intentionally done, copied from elsewhere and does not make sense here,
or an original concept whose purpose remains a mystery.