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