Re: [PATCH] qla2xxx: fix bad locking during eh_abort

Jeff Garzik
2005-05-27T22:47:10Z

Andrew Vasquez wrote:
> On Fri, 27 May 2005, Jeff Garzik wrote:
>
>
>>James Bottomley wrote:
>>
>>>On Fri, 2005-05-27 at 16:30 -0400, Jeff Garzik wrote:
>>>
>>>
>>>>The ha->hardware_lock is obtained without spin_lock_irq(), so that's
>>>>correct.
>>>
>>>
>>>Yes, that was my confusion
>>>
>>>But ... I wish you hadn't pointed it out. Now I look at the driver, the
>>>ha->hardware_lock is taken at interrupt level (in the interrupt
>>>routines).
>>>
>>>However, this sequence of code:
>>>
>>> spin_unlock_irq(ha->host->host_lock);
>>> spin_lock(&ha->hardware_lock);
>>>
>>>Enables interrupts then takes this lock. If we ever get a qla interrupt
>>>before we drop the ha->hardware_lock again, that will be a classic
>>>deadlock.
>>
>>Agreed, this was my analysis as well.
>>
>>This driver appears to get locking -backwards-:
>>
>>it uses spin_lock_irqsave() in interrupt handler.
>
>
> Actually, the ISR function is used as a polling function during
> hardware initialization. But since at that point we've never
> registered it via request_irq(), it's safe to not use the
> _irqsave()/_irqrestore() variants.

I see what's going on, thanks for explaining.

Note using _irqsave in the interrupt handler is not recommended and
quite unusual, for performance reasons if nothing else. The fast,
common path [interrupt handler] should use the least expensive spinlock
variant: spin_lock(). This implies that other [slow] paths need to be
changed to call local_irq_save() or spin_lock_irq() or whatnot.

Jeff