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

[Gustavo Pimentel <gustavo.pimentel@synopsys.com>]

Hi all,


> 
> I just realised (at 1am!) that the first patch is awfully buggy:
> - ENABLE and MASK have opposite polarities
> - There is nothing initialising the ENABLE and MASK registers
> 
> I've stashed the following fix-up on top of the existing series:
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f06e67c60593..0fa9e8fdce66 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -166,7 +166,7 @@ static void dw_pci_bottom_mask(struct irq_data *data)

(snip)

I used your patch and made it more perceptible in my opinion, (basically I
transformed the variable irq_mask (previous irq_status) into a mirror of MASK
registers, which removed the need for the *NOT* operation and forced the mask
operation swap in the callbacks)

Lorenzo can you apply this fix-up in your branch test/pci-dwc-msi? Maybe merging
with eca44651920c ("PCI: designware: Move interrupt acking into the proper
callback"), perhaps.

I've tested Marc's patch series (with and without my fix-up and both) with a
NVme EP that generates a burst of 19 MSI-X interrupts. Both approaches are
working fine.

Just a couple of suggestions Lorenzo, maybe you could exchange the *designware*
by *dwc* on all patch series titles and on eca44651920c("PCI: designware: Move
interrupt acking into the proper callback") replace *acking* by *ACKing* like
previous patch has.

Marc thanks for this patch fix! :)

Tested-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 0fa9e8f..a5132b3 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -164,9 +164,9 @@ static void dw_pci_bottom_mask(struct irq_data *data)
                res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
                bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;

-               pp->irq_status[ctrl] &= ~(1 << bit);
+               pp->irq_mask[ctrl] |= BIT(bit);
                dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
-                                   ~pp->irq_status[ctrl]);
+                                   pp->irq_mask[ctrl]);
        }

        raw_spin_unlock_irqrestore(&pp->lock, flags);
@@ -187,30 +187,30 @@ static void dw_pci_bottom_unmask(struct irq_data *data)
                res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
                bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;

-               pp->irq_status[ctrl] |= 1 << bit;
+               pp->irq_mask[ctrl] &= ~BIT(bit);
                dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
-                                   ~pp->irq_status[ctrl]);
+                                   pp->irq_mask[ctrl]);
        }

        raw_spin_unlock_irqrestore(&pp->lock, flags);
 }

-static void dw_pci_bottom_ack(struct irq_data *d)
+static void dw_pci_bottom_ack(struct irq_data *data)
 {
-       struct pcie_port *pp  = irq_data_get_irq_chip_data(d);
+       struct pcie_port *pp = irq_data_get_irq_chip_data(data);
        unsigned int res, bit, ctrl;
        unsigned long flags;

-       ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL;
+       ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
        res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
-       bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL;
+       bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;

        raw_spin_lock_irqsave(&pp->lock, flags);

-       dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);
+       dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, BIT(bit));

        if (pp->ops->msi_irq_ack)
-               pp->ops->msi_irq_ack(d->hwirq, pp);
+               pp->ops->msi_irq_ack(data->hwirq, pp);

        raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
@@ -665,13 +665,13 @@ void dw_pcie_setup_rc(struct pcie_port *pp)

        /* Initialize IRQ Status array */
        for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
+               pp->irq_mask[ctrl] = ~0;
                dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK +
                                        (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
-                                   4, ~0);
+                                   4, pp->irq_mask[ctrl]);
                dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
                                        (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
                                    4, ~0);
-               pp->irq_status[ctrl] = 0;
        }

        /* Setup RC BARs */
diff --git a/drivers/pci/controller/dwc/pcie-designware.h
b/drivers/pci/controller/dwc/pcie-designware.h
index 0989d88..9d1614a 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -169,7 +169,7 @@ struct pcie_port {
        struct irq_domain       *msi_domain;
        dma_addr_t              msi_data;
        u32                     num_vectors;
-       u32                     irq_status[MAX_MSI_CTRLS];
+       u32                     irq_mask[MAX_MSI_CTRLS];
        raw_spinlock_t          lock;
        DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
 };