fix ide-scsi EH locking
PATCH 1/1 fix ide-scsi EH locking
SCSI's error handling hooks are called inside
spin_lock_irqsave(host_lock, flags) ... spin_unlock_irqrestore(host_lock, flags)
ide-scsi's SCSI EH functions, which operate inside the above lock, wrap several operations inside
spin_lock_irq(ide_lock) ... spin_unlock_irq(ide_lock)
Use of the unconditional spin_lock_irq(), as opposed to spin_lock_irqsave(), corrupts the irq context.
Attached patch (against latest git) updates ide-scsi to simply use the spin_lock() variant, since we know we are already inside of spin_lock_irqsave().
Patch untested, but at least the code isn't obviously wrong now...
drivers/scsi/ide-scsi.c +8 -6
@@ -46,6 +46,7 @@#include <linux/slab.h> #include <linux/ide.h> #include <linux/scatterlist.h> +#include <linux/delay.h> #include <asm/io.h> #include <asm/bitops.h>
@@ -959,7 +960,8 @@ static int idescsi_eh_abort (struct scsiif (test_bit (IDESCSI_LOG_CMD , &scsi->log))printk (KERN_WARNING "ide-scsi: drive did%s become ready\n" , busy?" not" :"" ); -spin_lock_irq (&ide_lock); +/* remember, we are inside spin_lock_irq() already */ +spin_lock (&ide_lock);/* If there is no pc running we're done (our interrupt took care of it) */ if (!scsi->pc) {
@@ -985,7 +987,7 @@ static int idescsi_eh_abort (struct scsi } ide_unlock: -spin_unlock_irq (&ide_lock); +spin_unlock (&ide_lock); no_drive:if (test_bit (IDESCSI_LOG_CMD , &scsi->log))printk (KERN_WARNING "ide-scsi: abort returns %s\n" , ret ==SUCCESS ?"success" :"failed" );
@@ -1012,7 +1014,8 @@ static int idescsi_eh_reset (struct scsireturn FAILED ; } -spin_lock_irq (&ide_lock); +/* remember, we are inside spin_lock_irq() already */ +spin_lock (&ide_lock);if (!scsi->pc || (req = scsi->pc->rq) !=HWGROUP (drive)->rq || !HWGROUP (drive)->handler) {printk (KERN_WARNING "ide-scsi: No active request in idescsi_eh_reset\n" );
@@ -1038,16 +1041,15 @@ static int idescsi_eh_reset (struct scsiHWGROUP (drive)->rq =NULL ;HWGROUP (drive)->handler =NULL ;HWGROUP (drive)->busy =1 ;/* will set this to zero when ide reset finished */ -spin_unlock_irq (&ide_lock); +spin_unlock (&ide_lock);ide_do_reset (drive);/* ide_do_reset starts a polling handler which restarts itself every 50ms until the reset finishes */ do { -set_current_state (TASK_UNINTERRUPTIBLE );spin_unlock_irq (cmd->device->host->host_lock); -schedule_timeout (HZ /20 ); +msleep (50 );spin_lock_irq (cmd->device->host->host_lock); }while (HWGROUP (drive)->handler );
Quoted 1 line · Jeff Garzik
Patch untested, but at least the code isn't obviously wrong now...
The abort code check of cmd->serial_number is the only thing I can see that needs care and that looks safe by the time we hit eh_abort because the mid level has quiesced the request queue.
eh_reset is whacking on requests but they are the IDE layer requests so I suspect you want to simply drop the scsi locks for the eh functions much earlier and use spin_lock_irqsave/restore on the ide lock ?