Re: [PATCH v6 3/3] PCI: Add tango MSI controller support

[Marc Gonzalez]

On 13/06/2017 16:22, Marc Zyngier wrote:
> On 13/06/17 15:01, Marc Gonzalez wrote:
>> The MSI controller in Tango supports 256 message-signaled interrupts,
>> and a single doorbell address.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@xxxxxxxxxxxxxxxx>
>> ---
>> 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.