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

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

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