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

login
register
mail settings
Submitter Gustavo Pimentel
Date Nov. 22, 2018, 12:03 p.m.
Message ID <9b63c20b-f928-7c40-2734-00ed783625f5@synopsys.com>
Download mbox | patch
Permalink /patch/662631/
State New
Headers show

Comments

Gustavo Pimentel - Nov. 22, 2018, 12:03 p.m.
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>
Gustavo Pimentel - Nov. 22, 2018, 4:07 p.m.
Hi Lorenzo,

Just to ease the job, please also include this:

Fixes: 7c5925afbc58 ("PCI: dwc: Move MSI IRQs allocation to IRQ domains
hierarchical API")

On 22/11/2018 12:03, 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.
> 
> 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);
>  };
>
Lorenzo Pieralisi - Nov. 22, 2018, 4:26 p.m.
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);
>  };
>
Marc Zyngier - Nov. 22, 2018, 4:38 p.m.
On 22/11/2018 16:26, Lorenzo Pieralisi wrote:
> On Thu, Nov 22, 2018 at 12:03:25PM +0000, Gustavo Pimentel wrote:

[...]

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

To add to Lorenzo's comment, we're trying hard to have a *minimal* fix
that can be easily backported. Changing variable and field names as well
as flipping the semantic of other bits of the driver makes it harder to
review, and certainly doesn't help getting things backported to stable
(see the stable kernel rules).

I'd suggest this kind of repainting is better kept as a separate patch
and merged separately.

Thanks,

	M.
Gustavo Pimentel - Nov. 22, 2018, 5:40 p.m.
On 22/11/2018 16:38, Marc Zyngier wrote:
> On 22/11/2018 16:26, Lorenzo Pieralisi wrote:
>> On Thu, Nov 22, 2018 at 12:03:25PM +0000, Gustavo Pimentel wrote:
> 
> [...]
> 
>>> 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.
> 
> To add to Lorenzo's comment, we're trying hard to have a *minimal* fix
> that can be easily backported. Changing variable and field names as well
> as flipping the semantic of other bits of the driver makes it harder to
> review, and certainly doesn't help getting things backported to stable
> (see the stable kernel rules).
> 
> I'd suggest this kind of repainting is better kept as a separate patch
> and merged separately.

Makes sense.

Gustavo

> 
> Thanks,
> 
> 	M.
>
Gustavo Pimentel - Nov. 22, 2018, 5:49 p.m.
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@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.

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);
>>  };
>>
Trent Piepho - Nov. 26, 2018, 3:52 p.m.
On Thu, 2018-11-22 at 12:03 +0000, Gustavo Pimentel wrote:
> 

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


This would be the patch that enables all MSI interrupts on driver
initialization?

I don't think that's a good idea.  I was under the impression Marc
thought that as well.  It would be better to enable them when they are
enabled, via enable and disable methods.

I've looked into the interplay with enable and mask some more. 
Previous versions of the driver used the mask registers to both
en/disable and un/mask MSIs.  Both enable and mask methods were
provided, but they were the same mask function in the driver.

Functions like irq_percpu_enable() will call the irq_enable method, but
fall-back to irq_unmask if the chip does not provide irq_enable.  So a
driver can be sloppy about the distinction between masking and disable
an irq and it isn't necessarily apparent.

Which is how keystone has ks_pcie_msi_set_irq() that is part of the
unmask method (now), but certainly looks like it should be part of
enable.

I think the right way to fix this driver would be to:
Ack the irq in the irq_ack method.
Write enable and disable methods that use the enable register
Write mask/unmask methods that use the mask register
Rename the msi_(set|clr)_irq methods to msi_(en|dis)able_irq
Call ops->msi_enable_irq() from dw_pci_bottom_enable()
irq_status should get renamed to irq_mask, since it's just a driver 
cache of the mask register.  Could also cache the enable register, but
that reg is used less often.  Could also use regcache to cache these
registers.
Trent Piepho - Nov. 26, 2018, 4:06 p.m.
On Thu, 2018-11-22 at 16:38 +0000, Marc Zyngier wrote:
> 

> To add to Lorenzo's comment, we're trying hard to have a *minimal* fix

> that can be easily backported. Changing variable and field names as well

> as flipping the semantic of other bits of the driver makes it harder to

> review, and certainly doesn't help getting things backported to stable

> (see the stable kernel rules).

> 

> I'd suggest this kind of repainting is better kept as a separate patch

> and merged separately.


If you want a minimal fix to backport, you should just use my original
patch for 4.15-4.19 stable series.  It's a one line change to fix the
real bug.

It'll be far easier to backport to < 4.17,  where the hierarchical irq
domain stuff wasn't used in this part of the driver.

If you follow the control flow of an irq, you'll see that it ACKs the
irq at the same point as putting the ack in a new irq_ack method does.

While the driver shouldn't enable or disable on mask, that's also a new
bug from 4.17 that doesn't need to be fixed in earlier versions.

It's unlikely to come up, in 4.17+, since the mask methods don't get
used in normal irq flow.  Still a bug.
Marc Zyngier - Nov. 27, 2018, 7:50 a.m.
On Mon, 26 Nov 2018 15:52:42 +0000,
Trent Piepho <tpiepho@impinj.com> wrote:
> 
> On Thu, 2018-11-22 at 12:03 +0000, Gustavo Pimentel wrote:
> > 
> > > 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)
> 
> This would be the patch that enables all MSI interrupts on driver
> initialization?
> 
> I don't think that's a good idea.  I was under the impression Marc
> thought that as well.  It would be better to enable them when they are
> enabled, via enable and disable methods.

What gain does this bring? Frankly, I see exactly zero advantage of
doing so. It may look cosmetically appealing in the sense that it
makes use of of a register that the IP offers, but for Linux the
advantage is basically null.

Thanks,

	M.
Marc Zyngier - Nov. 27, 2018, 7:51 a.m.
On Mon, 26 Nov 2018 16:06:54 +0000,
Trent Piepho <tpiepho@impinj.com> wrote:
> 
> On Thu, 2018-11-22 at 16:38 +0000, Marc Zyngier wrote:
> > 
> > To add to Lorenzo's comment, we're trying hard to have a *minimal* fix
> > that can be easily backported. Changing variable and field names as well
> > as flipping the semantic of other bits of the driver makes it harder to
> > review, and certainly doesn't help getting things backported to stable
> > (see the stable kernel rules).
> > 
> > I'd suggest this kind of repainting is better kept as a separate patch
> > and merged separately.
> 
> If you want a minimal fix to backport, you should just use my original
> patch for 4.15-4.19 stable series.  It's a one line change to fix the
> real bug.
> 
> It'll be far easier to backport to < 4.17,  where the hierarchical irq
> domain stuff wasn't used in this part of the driver.

The right fix is to create an irq_ack method and put the relevant code
there.

Thanks,

	M.
Trent Piepho - Nov. 27, 2018, 5:23 p.m.
On Tue, 2018-11-27 at 07:51 +0000, Marc Zyngier wrote:
> On Mon, 26 Nov 2018 16:06:54 +0000,

> Trent Piepho <tpiepho@impinj.com> wrote:

> > 

> > On Thu, 2018-11-22 at 16:38 +0000, Marc Zyngier wrote:

> > > 

> > > To add to Lorenzo's comment, we're trying hard to have a

> > > *minimal* fix

> > > that can be easily backported. Changing variable and field names

> > > as well

> > > as flipping the semantic of other bits of the driver makes it

> > > harder to

> > > review, and certainly doesn't help getting things backported to

> > > stable

> > > (see the stable kernel rules).

> > > 

> > > I'd suggest this kind of repainting is better kept as a separate

> > > patch

> > > and merged separately.

> > 

> > If you want a minimal fix to backport, you should just use my original

> > patch for 4.15-4.19 stable series.  It's a one line change to fix the

> > real bug.

> > 

> > It'll be far easier to backport to < 4.17,  where the hierarchical irq

> > domain stuff wasn't used in this part of the driver.

> 

> The right fix is to create an irq_ack method and put the relevant code

> there.


Fine.

But the irq domain stuff should go into the stable 4.15 and 4.16
kernels.  It's not a fix and it changes behavior in how the interrupts
look in /proc significantly.
Trent Piepho - Nov. 27, 2018, 6:12 p.m.
On Tue, 2018-11-27 at 07:50 +0000, Marc Zyngier wrote:
> On Mon, 26 Nov 2018 15:52:42 +0000,

> Trent Piepho <tpiepho@impinj.com> wrote:

> > 

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

> > 

> > This would be the patch that enables all MSI interrupts on driver

> > initialization?

> > 

> > I don't think that's a good idea.  I was under the impression Marc

> > thought that as well.  It would be better to enable them when they are

> > enabled, via enable and disable methods.

> 

> What gain does this bring? Frankly, I see exactly zero advantage of

> doing so. It may look cosmetically appealing in the sense that it

> makes use of of a register that the IP offers, but for Linux the

> advantage is basically null.


Here's why:

1. It's a big change in driver behavior to enable all MSIs.  There will
certainly be hardware that writes to an MSI-X address, perhaps to
generate a MSI, perhaps not, where that MSI is disabled.  Now that
hardware will start generating interrupts.  That could be a big deal. 
The MSI might well have been not enabled very intentionally!  No reason
to create that change in behavior and also very much not a good idea to
backport to stable kernels.

2. Existing code is not clear about the difference between mask and
disable, getting it wrong in some places and causing bugs.  Creating
both mask and disable will make it clear.  It's the same reasoning you
use to reject my simple patch to put the irq ack at the correct time
and instead also put it in the semantically correct location.

3. A platform, keystone, has provided enable/disable methods for the
dwc driver.  But they are (now) called from the mask/unmask routines?!
That's not right; it'll drop irqs if it's really an enable/disable
feature in keystone.  Without dwc enable/disable methods, where will
the platform enable/disable methods be called from?  These methods are
getting randomly moved from mask to disable and back, like the ack
getting moved around, and clear distinction between disable and mask
will help stop that.
Gustavo Pimentel - Dec. 7, 2018, 4:16 p.m.
Hi all,

(snip)

I was able to reproduce Trent's problem in my configuration while developing the
eDMA driver, always having lost the second interrupt when an interrupt burst
occurs (observed in 50 trials).

After applying Marc's patch series, I stopped losing the second interrupt when
an interrupt burst occurs (I ran about 50 trials).

I can say with greater certainty that this fix solves the reported problem.

Regards,
Gustavo

Patch

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