On 22/11/2018 16:26, Lorenzo Pieralisi wrote:
> 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.
Yes, of course.
>
> 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@xxxxxxxxxxxx>
>>
>> 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.
Following Marc's suggestion, this can go in a separate patch.
>
> 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);
>> };
>>
