| Message ID | 20181113225734.8026-3-marc.zyngier@arm.com |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: designware: Fixing MSI handling flow | expand |
On Tue, 2018-11-13 at 22:57 +0000, Marc Zyngier wrote: > Bizarrely, there is no lock taken in the irq_ack helper. This > puts the ack callback provided by a specific platform in a awkward > situation where there is no synchronization that would be expected > on other callback. > > Introduce the required lock, giving some level of uniformity among > callbacks. The locking strategy here does not entirely make sense to me. The only call path I see that can reach this function is via the generic_handle_irq() call in dw_handle_msi_irq(). Is there another path I missed? If that is the only path, then how could this method be called reentrantly on the same port? It would require that dw_handle_msi_irq() was called reentrantly. But look at that function, it's clearly not reentrant. It will race with itself trying to handle the same status bits. > @@ static void dw_pci_bottom_ack(struct irq_data *d) > { > struct msi_desc *msi = irq_data_get_msi_desc(d); > struct pcie_port *pp; > + unsigned long flags; > > pp = msi_desc_to_pci_sysdata(msi); > > + raw_spin_lock_irqsave(&pp->lock, flags); > + > if (pp->ops->msi_irq_ack) > pp->ops->msi_irq_ack(d->hwirq, pp); > + > + raw_spin_unlock_irqrestore(&pp->lock, flags); >
On Tue, Nov 13, 2018 at 10:57:33PM +0000, Marc Zyngier wrote: > Bizarrely, there is no lock taken in the irq_ack helper. This > puts the ack callback provided by a specific platform in a awkward > situation where there is no synchronization that would be expected > on other callback. > > Introduce the required lock, giving some level of uniformity among > callbacks. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index c3aa8b5fb51d..0a76948ed49e 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -202,11 +202,16 @@ static void dw_pci_bottom_ack(struct irq_data *d) > { > struct msi_desc *msi = irq_data_get_msi_desc(d); > struct pcie_port *pp; > + unsigned long flags; > > pp = msi_desc_to_pci_sysdata(msi); > > + raw_spin_lock_irqsave(&pp->lock, flags); > + > if (pp->ops->msi_irq_ack) > pp->ops->msi_irq_ack(d->hwirq, pp); > + > + raw_spin_unlock_irqrestore(&pp->lock, flags); > } > > static struct irq_chip dw_pci_msi_bottom_irq_chip = { Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
On 13/11/2018 22:57, Marc Zyngier wrote: > Bizarrely, there is no lock taken in the irq_ack helper. This > puts the ack callback provided by a specific platform in a awkward > situation where there is no synchronization that would be expected > on other callback. > > Introduce the required lock, giving some level of uniformity among > callbacks. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index c3aa8b5fb51d..0a76948ed49e 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -202,11 +202,16 @@ static void dw_pci_bottom_ack(struct irq_data *d) > { > struct msi_desc *msi = irq_data_get_msi_desc(d); > struct pcie_port *pp; > + unsigned long flags; > > pp = msi_desc_to_pci_sysdata(msi); > > + raw_spin_lock_irqsave(&pp->lock, flags); > + > if (pp->ops->msi_irq_ack) > pp->ops->msi_irq_ack(d->hwirq, pp); > + > + raw_spin_unlock_irqrestore(&pp->lock, flags); > } > > static struct irq_chip dw_pci_msi_bottom_irq_chip = { > 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 c3aa8b5fb51d..0a76948ed49e 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -202,11 +202,16 @@ static void dw_pci_bottom_ack(struct irq_data *d) { struct msi_desc *msi = irq_data_get_msi_desc(d); struct pcie_port *pp; + unsigned long flags; pp = msi_desc_to_pci_sysdata(msi); + raw_spin_lock_irqsave(&pp->lock, flags); + if (pp->ops->msi_irq_ack) pp->ops->msi_irq_ack(d->hwirq, pp); + + raw_spin_unlock_irqrestore(&pp->lock, flags); } static struct irq_chip dw_pci_msi_bottom_irq_chip = {
Bizarrely, there is no lock taken in the irq_ack helper. This puts the ack callback provided by a specific platform in a awkward situation where there is no synchronization that would be expected on other callback. Introduce the required lock, giving some level of uniformity among callbacks. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/pci/controller/dwc/pcie-designware-host.c | 5 +++++ 1 file changed, 5 insertions(+)