Re: [patch, -rc5-mm3] fix IDE deadlock in error reporting code

[Ingo Molnar <mingo@elte.hu>]

* Ingo Molnar <mingo@elte.hu> wrote:

> ah. That's a real deadlock scenario. Does the patch below fix it? If 
> yes then i think this is a candidate for 2.6.17 merging too.

actually, the replacement patch below is better i think - it moves the 
ide_lock taking to outside the printing section. That should still be OK 
as we dont call other functions from within the section, and it should 
also result in slightly more robust printing, as the whole printing code 
will be atomic under ide_lock.

	Ingo

---------
Subject: [patch, -rc5-mm3] fix IDE deadlock in error reporting code
From: Ingo Molnar <mingo@elte.hu>

Michal Piotrowski reported the following validator assert:

 hdd: set_drive_speed_status: status=0x51 { DriveReady SeekComplete Error }
 hdd: set_drive_speed_status: error=0xb4 { AbortedCommand LastFailedSense=0x0b }

 ============================
 [ BUG: illegal lock usage! ]
 ----------------------------
 illegal {in-hardirq-W} -> {hardirq-on-W} usage.
 hdparm/1821 [HC0[0]:SC0[0]:HE1:SE1] takes:
  (ide_lock){++..}, at: [<c0268388>] ide_dump_opcode+0x13/0x9b

 [...]

 stack backtrace:
  [<c0104513>] show_trace+0x1b/0x20
  [<c01045f1>] dump_stack+0x1f/0x24
  [<c013976c>] print_usage_bug+0x1a5/0x1b1
  [<c0139e90>] mark_lock+0x2ca/0x4f7
  [<c013aa96>] __lockdep_acquire+0x47e/0xaa4
  [<c013b536>] lockdep_acquire+0x67/0x7f
  [<c030552d>] _spin_lock+0x24/0x32
  [<c0268388>] ide_dump_opcode+0x13/0x9b
  [<c02688b6>] ide_dump_status+0x4a6/0x4cc
  [<c0267ae6>] ide_config_drive_speed+0x32a/0x33a
  [<c0262dc5>] piix_tune_chipset+0x2ed/0x2f8
  [<c0262e31>] piix_config_drive_xfer_rate+0x61/0xb5
  [<c0263a82>] set_using_dma+0x2f/0x60
  [<c0263bee>] ide_write_setting+0x4a/0xc3
  [<c02647ca>] generic_ide_ioctl+0x8a/0x47f
  [<f886003a>] idecd_ioctl+0xfd/0x133 [ide_cd]
  [<c01f1fff>] blkdev_driver_ioctl+0x4b/0x5f
  [<c01f2783>] blkdev_ioctl+0x770/0x7bd
  [<c017dc0d>] block_ioctl+0x1f/0x21
  [<c0189353>] do_ioctl+0x27/0x6e
  [<c0189604>] vfs_ioctl+0x26a/0x280
  [<c0189667>] sys_ioctl+0x4d/0x7e
  [<c0305ed2>] sysenter_past_esp+0x63/0xa1

in ide_dump_opcode() takes the ide_lock in an irq-unsafe manner,
i.e. this function expects to be called with irqs disabled. But
ide_dump_ata[pi]_status() doesnt do that - it enables interrupts
specifically. That is a no-no - what guarantees that another IDE
port couldnt generate an IDE interrupt while we are dumping this
error? The fix is to turn the irq-enabling in these functions into
irq-disabling.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/ide/ide-lib.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Index: linux/drivers/ide/ide-lib.c
===================================================================
--- linux.orig/drivers/ide/ide-lib.c
+++ linux/drivers/ide/ide-lib.c
@@ -471,11 +471,9 @@ static void ide_dump_opcode(ide_drive_t 
 	u8 opcode = 0;
 	int found = 0;
 
-	spin_lock(&ide_lock);
 	rq = NULL;
 	if (HWGROUP(drive))
 		rq = HWGROUP(drive)->rq;
-	spin_unlock(&ide_lock);
 	if (!rq)
 		return;
 	if (rq->flags & (REQ_DRIVE_CMD | REQ_DRIVE_TASK)) {
@@ -506,7 +504,7 @@ static u8 ide_dump_ata_status(ide_drive_
 	unsigned long flags;
 	u8 err = 0;
 
-	local_irq_set(flags);
+	spin_lock_irqsave(&ide_lock, flags);
 	printk("%s: %s: status=0x%02x { ", drive->name, msg, stat);
 	if (stat & BUSY_STAT)
 		printk("Busy ");
@@ -566,7 +564,8 @@ static u8 ide_dump_ata_status(ide_drive_
 		printk("\n");
 	}
 	ide_dump_opcode(drive);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&ide_lock, flags);
+
 	return err;
 }
 
@@ -588,7 +587,9 @@ static u8 ide_dump_atapi_status(ide_driv
 
 	status.all = stat;
 	error.all = 0;
-	local_irq_set(flags);
+
+	spin_lock_irqsave(&ide_lock, flags);
+
 	printk("%s: %s: status=0x%02x { ", drive->name, msg, stat);
 	if (status.b.bsy)
 		printk("Busy ");
@@ -614,7 +615,8 @@ static u8 ide_dump_atapi_status(ide_driv
 		printk("}\n");
 	}
 	ide_dump_opcode(drive);
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&ide_lock, flags);
+
 	return error.all;
 }