On 13/06/2017 16:22, Marc Zyngier wrote:

On 13/06/17 15:01, Marc Gonzalez wrote:

quoted

The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.

Signed-off-by: Marc Gonzalez <redacted>
---
Changes from v5 to v6
o Rename 'used' bitmap to 'used_msi'
o Rename 'lock' spinlock to 'used_msi_lock'
o Take lock in interrupt handler
o Remove irq_dom in error path
---
 drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..b06446b23bc8 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c

@@ -1,16 +1,228 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used_msi, MSI_MAX);
+	spinlock_t used_msi_lock;
 	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_enable;
+	phys_addr_t msi_doorbell;
+	struct irq_domain *irq_dom;
+	struct irq_domain *msi_dom;
+	int irq;
 };
 
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
+	unsigned long flags, status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);

You're already in interrupt context, so there is no need to disable
interrupts any further. spin_lock() should do the trick

Thanks for the hint.

I am confused, because Documentation/locking/spinlocks.txt states:

If you have a case where you have to protect a data structure across
several CPU's and you want to use spinlocks you can potentially use
cheaper versions of the spinlocks. IFF you know that the spinlocks are
never used in interrupt handlers, you can use the non-irq versions:

	spin_lock(&lock);
	...
	spin_unlock(&lock);

(and the equivalent read-write versions too, of course). The spinlock will
guarantee the same kind of exclusive access, and it will be much faster.
This is useful if you know that the data in question is only ever
manipulated from a "process context", ie no interrupts involved.

The reasons you mustn't use these versions if you have interrupts that
play with the spinlock is that you can get deadlocks:

	spin_lock(&lock);
	...
		<- interrupt comes in:
			spin_lock(&lock);

where an interrupt tries to lock an already locked variable. This is ok if
the other interrupt happens on another CPU, but it is _not_ ok if the
interrupt happens on the same CPU that already holds the lock, because the
lock will obviously never be released (because the interrupt is waiting
for the lock, and the lock-holder is interrupted by the interrupt and will
not continue until the interrupt has been processed).

(This is also the reason why the irq-versions of the spinlocks only need
to disable the _local_ interrupts - it's ok to use spinlocks in interrupts
on other CPU's, because an interrupt on another CPU doesn't interrupt the
CPU that holds the lock, so the lock-holder can continue and eventually
releases the lock).

Isn't this saying that it is not safe to call spin_lock() from
the interrupt handler? (Sorry if I misunderstood.)

Regards.