lkml.org 
[lkml]   [2006]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [patch, -rc5-mm3] fix IDE deadlock in error reporting code

* 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;
}

-
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/
\
 
 \ /
  Last update: 2006-06-05 10:40    [from the cache]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and my Meterkast|Read the blog