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
- : send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
