Messages in this thread Patch in this message |  | | Date | Sun, 11 Nov 2001 17:37:21 -0800 | From | Andrew Morton <> | Subject | sbp2.c on SMP |
| |
Guys,
drivers/ieee1394/sbp2.c deadlocks immediately on SMP, because io_request_lock is not held over its call to scsi_old_done().
I don't know why scsi_old_done() actually requires io_request_lock; perhaps Jens could comment on whether I've taken the lock in the appropriate place?
With the appended patch I can confirm that the driver happily runs `dbench 40' for half an hour on dual x86. Even when you kick the disk onto the floor (sorry, HJ).
The games which scsi_old_done() plays with spinlocks and interrupt enabling are really foul. If someone calls it with interrupts disabled (sbp2 does this) then scsi_old_done() will enable interrupts for you. If, for example, you call scsi_old_done() under spin_lock_irqsave(), the reenabling of interrupts will expose you to deadlocks. Perhaps scsi_old_done() should just use spin_unlock()/spin_lock()?
I tried enabling SBP2_USE_REAL_SPINLOCKS in sbp2.c and that appears to work just fine, although I haven't left that change in place here.
You don't actually _need_ SMP hardware to test SMP code, BTW. You can just build an SMP kernel and run that on a uniprocessor box. This will still catch a wide range of bugs - it certainly detects the lockup which was occurring because of the scsi_old_done() thing.
Incidentally, it would be nice to be able to get this driver working properly when linked into the kernel - it makes debugging much easier :)
--- linux-2.4.15-pre2/drivers/ieee1394/sbp2.c Wed Oct 17 14:19:20 2001 +++ linux-akpm/drivers/ieee1394/sbp2.c Sun Nov 11 17:22:47 2001 @@ -2767,7 +2767,9 @@ static void sbp2scsi_complete_command(st /* * Tell scsi stack that we're done with this command */ + spin_lock_irq(&io_request_lock); done (SCpnt); + spin_unlock_irq(&io_request_lock); return; } - - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |