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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Thu, Nov 22, 2018 at 12:03:25PM +0000, Gustavo Pimentel wrote:
> 
> 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.

I want to see more testing before getting this series merged upstream,
especially for the platform configurations on which this bug was
reported.

I am a bit annoyed with DWC platform maintainers in this regard.

> 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);

Changes in this hunk are unrelated, I won't squash them in.

Lorenzo

> 
>         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);
>  };
>