qd65xx: fix deadlock on error handling

PATCH NAK Bartlomiej Zolnierkiewicz 3 participants 7 messages Oct 1, 2007 – Oct 3, 2007 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2007-10-01 21:39 UTC #0 lore raw

PATCH 3/15 qd65xx: fix deadlock on error handling

Stop abusing ide_lock lock (switch to a private locking).

Fixes same issue as fixed by Alan Cox in atiixp host driver with commit 6c5f8cc33eb2e10b6ab788bbe259fc142a068627.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/legacy/qd65xx.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

drivers/ide/legacy/qd65xx.c +9 -8

Index: b/drivers/ide/legacy/qd65xx.c
===================================================================
@@ -89,13 +89,15 @@
 
 static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
 
+static DEFINE_SPINLOCK(qd65xx_lock);
+
 static void qd_write_reg (u8 content, unsigned long reg)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&qd65xx_lock, flags);
 	outb(content,reg);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&qd65xx_lock, flags);
 }
 
 static u8 __init qd_read_reg (unsigned long reg)

@@ -103,9 +105,9 @@ static u8 __init qd_read_reg (unsigned l
 	unsigned long flags;
 	u8 read;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&qd65xx_lock, flags);
 	read = inb(reg);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&qd65xx_lock, flags);
 	return read;
 }

@@ -301,16 +303,15 @@ static void qd6580_set_pio_mode(ide_driv
 
 static int __init qd_testreg(int port)
 {
-	u8 savereg;
-	u8 readreg;
 	unsigned long flags;
+	u8 savereg, readreg;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&qd65xx_lock, flags);
 	savereg = inb_p(port);
 	outb_p(QD_TESTVAL, port);	/* safe value */
 	readreg = inb_p(port);
 	outb(savereg, port);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&qd65xx_lock, flags);
 
 	if (savereg == QD_TESTVAL) {
 		printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");

Sergei Shtylyov

2007-10-02 12:59 UTC #1 lore raw

Quoted 30 lines · Bartlomiej Zolnierkiewicz

Stop abusing ide_lock lock (switch to a private locking).
Fixes same issue as fixed by Alan Cox in atiixp host driver with commit 6c5f8cc33eb2e10b6ab788bbe259fc142a068627.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Index: b/drivers/ide/legacy/qd65xx.c
===================================================================
drivers/ide/legacy/qd65xx.c +4 -2
@@ -89,13 +89,15 @@
 
 static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
 
+static DEFINE_SPINLOCK(qd65xx_lock);
+
 static void qd_write_reg (u8 content, unsigned long reg)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&qd65xx_lock, flags);
 	outb(content,reg);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&qd65xx_lock, flags);
 }
 
 static u8 __init qd_read_reg (unsigned long reg)

drivers/ide/legacy/qd65xx.c static u8 __init qd_read_reg (unsigned l · line 112

Quoted 10 lines

drivers/ide/legacy/qd65xx.c +2 -2

@@ -103,9 +105,9 @@ static u8 __init qd_read_reg (unsigned l
 	unsigned long flags;
 	u8 read;
 
-	spin_lock_irqsave(&ide_lock, flags);
+	spin_lock_irqsave(&qd65xx_lock, flags);
 	read = inb(reg);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&qd65xx_lock, flags);
 	return read;
 }

I don't see why all the locking above is needed at all -- isn't these
atomic functions? :-/

Quoted 10 lines

drivers/ide/legacy/qd65xx.c +1 -3

@@ -301,16 +303,15 @@ static void qd6580_set_pio_mode(ide_driv
 
 static int __init qd_testreg(int port)
 {
-	u8 savereg;
-	u8 readreg;
 	unsigned long flags;
+	u8 savereg, readreg;
 
-	spin_lock_irqsave(&ide_lock, flags);

drivers/ide/legacy/qd65xx.c static void qd6580_set_pio_mode(ide_driv · line 317

Quoted 10 lines

drivers/ide/legacy/qd65xx.c +2 -1

+	spin_lock_irqsave(&qd65xx_lock, flags);
 	savereg = inb_p(port);
 	outb_p(QD_TESTVAL, port);	/* safe value */
 	readreg = inb_p(port);
 	outb(savereg, port);
-	spin_unlock_irqrestore(&ide_lock, flags);
+	spin_unlock_irqrestore(&qd65xx_lock, flags);
 
 	if (savereg == QD_TESTVAL) {
 		printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");

Heh, 486 with VLB hardly needed spinlocks at all... although there *were*
486 SMP machines -- with external LAPICs.

MBR, Sergei

Jeff Garzik

2007-10-02 13:04 UTC #2 lore raw

Quoted 36 lines · Sergei Shtylyov

Quoted 34 lines · Bartlomiej Zolnierkiewicz
Stop abusing ide_lock lock (switch to a private locking).
Fixes same issue as fixed by Alan Cox in atiixp host driver with commit 6c5f8cc33eb2e10b6ab788bbe259fc142a068627.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Index: b/drivers/ide/legacy/qd65xx.c
===================================================================
drivers/ide/legacy/qd65xx.c +0 -0
@@ -89,13 +89,15 @@
 
 static int timings[4]={-1,-1,-1,-1}; /* stores current timing for 
each timer */
drivers/ide/legacy/qd65xx.c +4 -2
+static DEFINE_SPINLOCK(qd65xx_lock);
+
 static void qd_write_reg (u8 content, unsigned long reg)
 {
     unsigned long flags;
 
-    spin_lock_irqsave(&ide_lock, flags);
+    spin_lock_irqsave(&qd65xx_lock, flags);
     outb(content,reg);
-    spin_unlock_irqrestore(&ide_lock, flags);
+    spin_unlock_irqrestore(&qd65xx_lock, flags);
 }
 
 static u8 __init qd_read_reg (unsigned long reg)
@@ -103,9 +105,9 @@ static u8 __init qd_read_reg (unsigned l
     unsigned long flags;
     u8 read;
 

Quoted 10 lines

drivers/ide/legacy/qd65xx.c static u8 __init qd_read_reg (unsigned l · line 112
Quoted 7 lines
drivers/ide/legacy/qd65xx.c +2 -2
-    spin_lock_irqsave(&ide_lock, flags);
+    spin_lock_irqsave(&qd65xx_lock, flags);
     read = inb(reg);
-    spin_unlock_irqrestore(&ide_lock, flags);
+    spin_unlock_irqrestore(&qd65xx_lock, flags);
     return read;
 }
I don't see why all the locking above is needed at all -- isn't these
atomic functions? :-/

Agreed.

Jeff

Bartlomiej Zolnierkiewicz

author

2007-10-02 22:11 UTC #3 lore raw

Quoted 48 lines · Jeff Garzik

Quoted 36 lines · Sergei Shtylyov
Quoted 34 lines · Bartlomiej Zolnierkiewicz
Stop abusing ide_lock lock (switch to a private locking).
Fixes same issue as fixed by Alan Cox in atiixp host driver with commit 6c5f8cc33eb2e10b6ab788bbe259fc142a068627.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Index: b/drivers/ide/legacy/qd65xx.c
===================================================================
drivers/ide/legacy/qd65xx.c +0 -0
@@ -89,13 +89,15 @@
 
 static int timings[4]={-1,-1,-1,-1}; /* stores current timing for 
each timer */
drivers/ide/legacy/qd65xx.c +4 -2
+static DEFINE_SPINLOCK(qd65xx_lock);
+
 static void qd_write_reg (u8 content, unsigned long reg)
 {
     unsigned long flags;
 
-    spin_lock_irqsave(&ide_lock, flags);
+    spin_lock_irqsave(&qd65xx_lock, flags);
     outb(content,reg);
-    spin_unlock_irqrestore(&ide_lock, flags);
+    spin_unlock_irqrestore(&qd65xx_lock, flags);
 }
 
 static u8 __init qd_read_reg (unsigned long reg)
@@ -103,9 +105,9 @@ static u8 __init qd_read_reg (unsigned l
     unsigned long flags;
     u8 read;
 
Quoted 10 lines
drivers/ide/legacy/qd65xx.c static u8 __init qd_read_reg (unsigned l · line 112
Quoted 7 lines
drivers/ide/legacy/qd65xx.c +2 -2
-    spin_lock_irqsave(&ide_lock, flags);
+    spin_lock_irqsave(&qd65xx_lock, flags);
     read = inb(reg);
-    spin_unlock_irqrestore(&ide_lock, flags);
+    spin_unlock_irqrestore(&qd65xx_lock, flags);
     return read;
 }
I don't see why all the locking above is needed at all -- isn't these
atomic functions? :-/

Quoted 1 line

Agreed.

Indeed, incremental patch follows.

[PATCH] qd65xx: remove pointless qd_{read,write}_reg()

These functions are atomic so locking is pointless (noticed by Sergei).

Cc Sergei Shtylyov <[email protected]>

Cc Jeff Garzik <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/legacy/qd65xx.c |   43 +++++++++++--------------------------------
 1 file changed, 11 insertions(+), 32 deletions(-)

drivers/ide/legacy/qd65xx.c +11 ~~-32~~

Index: b/drivers/ide/legacy/qd65xx.c
===================================================================
@@ -89,28 +89,6 @@
 
 static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
 
-static DEFINE_SPINLOCK(qd65xx_lock);
-
-static void qd_write_reg (u8 content, unsigned long reg)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&qd65xx_lock, flags);
-	outb(content,reg);
-	spin_unlock_irqrestore(&qd65xx_lock, flags);
-}
-
-static u8 __init qd_read_reg (unsigned long reg)
-{
-	unsigned long flags;
-	u8 read;
-
-	spin_lock_irqsave(&qd65xx_lock, flags);
-	read = inb(reg);
-	spin_unlock_irqrestore(&qd65xx_lock, flags);
-	return read;
-}
-
 /*
  * qd_select:
  *

@@ -123,7 +101,7 @@ static void qd_select (ide_drive_t *driv
 			(QD_TIMREG(drive) & 0x02);
 
 	if (timings[index] != QD_TIMING(drive))
-		qd_write_reg(timings[index] = QD_TIMING(drive), QD_TIMREG(drive));
+		outb(timings[index] = QD_TIMING(drive), QD_TIMREG(drive));
 }
 
 /*

@@ -286,7 +264,7 @@ static void qd6580_set_pio_mode(ide_driv
 	}
 
 	if (!HWIF(drive)->channel && drive->media != ide_disk) {
-		qd_write_reg(0x5f, QD_CONTROL_PORT);
+		outb(0x5f, QD_CONTROL_PORT);
 		printk(KERN_WARNING "%s: ATAPI: disabled read-ahead FIFO "
 			"and post-write buffer on %s.\n",
 			drive->name, HWIF(drive)->name);

@@ -303,6 +281,7 @@ static void qd6580_set_pio_mode(ide_driv
 
 static int __init qd_testreg(int port)
 {
+	static DEFINE_SPINLOCK(qd65xx_lock);
 	unsigned long flags;
 	u8 savereg, readreg;

@@ -365,13 +344,13 @@ static void __exit qd_unsetup(ide_hwif_t
 
 	if (set_pio_mode == (void *)qd6500_set_pio_mode) {
 		// will do it for both
-		qd_write_reg(QD6500_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
+		outb(QD6500_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
 	} else if (set_pio_mode == (void *)qd6580_set_pio_mode) {
 		if (QD_CONTROL(hwif) & QD_CONTR_SEC_DISABLED) {
-			qd_write_reg(QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
-			qd_write_reg(QD6580_DEF_DATA2, QD_TIMREG(&hwif->drives[1]));
+			outb(QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
+			outb(QD6580_DEF_DATA2, QD_TIMREG(&hwif->drives[1]));
 		} else {
-			qd_write_reg(hwif->channel ? QD6580_DEF_DATA2 : QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
+			outb(hwif->channel ? QD6580_DEF_DATA2 : QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
 		}
 	} else {
 		printk(KERN_WARNING "Unknown qd65xx tuning fonction !\n");

@@ -394,7 +373,7 @@ static int __init qd_probe(int base)
 	u8 config;
 	u8 unit;
 
-	config = qd_read_reg(QD_CONFIG_PORT);
+	config = inb(QD_CONFIG_PORT);
 
 	if (! ((config & QD_CONFIG_BASEPORT) >> 1 == (base == 0xb0)) )
 		return 1;

@@ -438,7 +417,7 @@ static int __init qd_probe(int base)
 
 		/* qd6580 found */
 
-		control = qd_read_reg(QD_CONTROL_PORT);
+		control = inb(QD_CONTROL_PORT);
 
 		printk(KERN_NOTICE "qd6580 at %#x\n", base);
 		printk(KERN_DEBUG "qd6580: config=%#x, control=%#x, ID3=%u\n",

@@ -459,7 +438,7 @@ static int __init qd_probe(int base)
 
 			ide_device_add(idx);
 
-			qd_write_reg(QD_DEF_CONTR, QD_CONTROL_PORT);
+			outb(QD_DEF_CONTR, QD_CONTROL_PORT);
 
 			return 1;
 		} else {

@@ -486,7 +465,7 @@ static int __init qd_probe(int base)
 
 			ide_device_add(idx);
 
-			qd_write_reg(QD_DEF_CONTR, QD_CONTROL_PORT);
+			outb(QD_DEF_CONTR, QD_CONTROL_PORT);
 
 			return 0; /* no other qd65xx possible */
 		}

Jeff Garzik

2007-10-02 22:19 UTC #4 lore raw

Bartlomiej Zolnierkiewicz wrote:

ACK everything else (that I snipped)...

drivers/ide/legacy/qd65xx.c static void qd6580_set_pio_mode(ide_driv · line 286

Quoted 7 lines

drivers/ide/legacy/qd65xx.c +1 -0

@@ -303,6 +281,7 @@ static void qd6580_set_pio_mode(ide_driv
 
 static int __init qd_testreg(int port)
 {
+	static DEFINE_SPINLOCK(qd65xx_lock);
 	unsigned long flags;
 	u8 savereg, readreg;

At this point, the lock is largely pointless, isn't it?

Seems like you might as well use local_irq_save/restore.

Jeff

Bartlomiej Zolnierkiewicz

author

2007-10-03 22:37 UTC #5 lore raw

Quoted 14 lines · Jeff Garzik

Bartlomiej Zolnierkiewicz wrote:
ACK everything else (that I snipped)...
Review static void qd6580_set_pio_mode(ide_driv · line 286
Quoted 7 lines
(quoted diff) +1 -0
@@ -303,6 +281,7 @@ static void qd6580_set_pio_mode(ide_driv
 
 static int __init qd_testreg(int port)
 {
+	static DEFINE_SPINLOCK(qd65xx_lock);
 	unsigned long flags;
 	u8 savereg, readreg;
At this point, the lock is largely pointless, isn't it?

Quoted 1 line

Seems like you might as well use local_irq_save/restore.

Yeah.

[PATCH] qd65xx: remove pointless qd_{read,write}_reg() (take 2)

These functions are atomic so locking is pointless (noticed by Sergei).

v2:
We can now just use local_irq_save/restore() in qd_testreg() (noticed by Jeff).

Cc Sergei Shtylyov <[email protected]>

Cc Jeff Garzik <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/legacy/qd65xx.c |   46 +++++++++++---------------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

drivers/ide/legacy/qd65xx.c +12 ~~-34~~

Index: b/drivers/ide/legacy/qd65xx.c
===================================================================
@@ -89,28 +89,6 @@
 
 static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
 
-static DEFINE_SPINLOCK(qd65xx_lock);
-
-static void qd_write_reg (u8 content, unsigned long reg)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&qd65xx_lock, flags);
-	outb(content,reg);
-	spin_unlock_irqrestore(&qd65xx_lock, flags);
-}
-
-static u8 __init qd_read_reg (unsigned long reg)
-{
-	unsigned long flags;
-	u8 read;
-
-	spin_lock_irqsave(&qd65xx_lock, flags);
-	read = inb(reg);
-	spin_unlock_irqrestore(&qd65xx_lock, flags);
-	return read;
-}
-
 /*
  * qd_select:
  *

@@ -123,7 +101,7 @@ static void qd_select (ide_drive_t *driv
 			(QD_TIMREG(drive) & 0x02);
 
 	if (timings[index] != QD_TIMING(drive))
-		qd_write_reg(timings[index] = QD_TIMING(drive), QD_TIMREG(drive));
+		outb(timings[index] = QD_TIMING(drive), QD_TIMREG(drive));
 }
 
 /*

@@ -286,7 +264,7 @@ static void qd6580_set_pio_mode(ide_driv
 	}
 
 	if (!HWIF(drive)->channel && drive->media != ide_disk) {
-		qd_write_reg(0x5f, QD_CONTROL_PORT);
+		outb(0x5f, QD_CONTROL_PORT);
 		printk(KERN_WARNING "%s: ATAPI: disabled read-ahead FIFO "
 			"and post-write buffer on %s.\n",
 			drive->name, HWIF(drive)->name);

@@ -306,12 +284,12 @@ static int __init qd_testreg(int port)
 	unsigned long flags;
 	u8 savereg, readreg;
 
-	spin_lock_irqsave(&qd65xx_lock, flags);
+	local_irq_save(flags);
 	savereg = inb_p(port);
 	outb_p(QD_TESTVAL, port);	/* safe value */
 	readreg = inb_p(port);
 	outb(savereg, port);
-	spin_unlock_irqrestore(&qd65xx_lock, flags);
+	local_irq_restore(flags);
 
 	if (savereg == QD_TESTVAL) {
 		printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");

@@ -365,13 +343,13 @@ static void __exit qd_unsetup(ide_hwif_t
 
 	if (set_pio_mode == (void *)qd6500_set_pio_mode) {
 		// will do it for both
-		qd_write_reg(QD6500_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
+		outb(QD6500_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
 	} else if (set_pio_mode == (void *)qd6580_set_pio_mode) {
 		if (QD_CONTROL(hwif) & QD_CONTR_SEC_DISABLED) {
-			qd_write_reg(QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
-			qd_write_reg(QD6580_DEF_DATA2, QD_TIMREG(&hwif->drives[1]));
+			outb(QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
+			outb(QD6580_DEF_DATA2, QD_TIMREG(&hwif->drives[1]));
 		} else {
-			qd_write_reg(hwif->channel ? QD6580_DEF_DATA2 : QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
+			outb(hwif->channel ? QD6580_DEF_DATA2 : QD6580_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
 		}
 	} else {
 		printk(KERN_WARNING "Unknown qd65xx tuning fonction !\n");

@@ -394,7 +372,7 @@ static int __init qd_probe(int base)
 	u8 config;
 	u8 unit;
 
-	config = qd_read_reg(QD_CONFIG_PORT);
+	config = inb(QD_CONFIG_PORT);
 
 	if (! ((config & QD_CONFIG_BASEPORT) >> 1 == (base == 0xb0)) )
 		return 1;

@@ -438,7 +416,7 @@ static int __init qd_probe(int base)
 
 		/* qd6580 found */
 
-		control = qd_read_reg(QD_CONTROL_PORT);
+		control = inb(QD_CONTROL_PORT);
 
 		printk(KERN_NOTICE "qd6580 at %#x\n", base);
 		printk(KERN_DEBUG "qd6580: config=%#x, control=%#x, ID3=%u\n",

@@ -459,7 +437,7 @@ static int __init qd_probe(int base)
 
 			ide_device_add(idx);
 
-			qd_write_reg(QD_DEF_CONTR, QD_CONTROL_PORT);
+			outb(QD_DEF_CONTR, QD_CONTROL_PORT);
 
 			return 1;
 		} else {

@@ -486,7 +464,7 @@ static int __init qd_probe(int base)
 
 			ide_device_add(idx);
 
-			qd_write_reg(QD_DEF_CONTR, QD_CONTROL_PORT);
+			outb(QD_DEF_CONTR, QD_CONTROL_PORT);
 
 			return 0; /* no other qd65xx possible */
 		}

Jeff Garzik

2007-10-03 22:51 UTC #6 lore raw

Quoted 21 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines · Jeff Garzik
Bartlomiej Zolnierkiewicz wrote:
ACK everything else (that I snipped)...
Quoted 2 lines
(quoted diff) +0 -0
@@ -303,6 +281,7 @@ static void qd6580_set_pio_mode(ide_driv
 
Quoted 8 lines
Review static void qd6580_set_pio_mode(ide_driv · line 286
Quoted 5 lines
(quoted diff) +1 -0
 static int __init qd_testreg(int port)
 {
+	static DEFINE_SPINLOCK(qd65xx_lock);
 	unsigned long flags;
 	u8 savereg, readreg;
At this point, the lock is largely pointless, isn't it?
Seems like you might as well use local_irq_save/restore.
Yeah.
[PATCH] qd65xx: remove pointless qd_{read,write}_reg() (take 2)

Quoted 8 lines

These functions are atomic so locking is pointless (noticed by Sergei).
v2:
We can now just use local_irq_save/restore() in qd_testreg() (noticed by Jeff).
Cc Sergei Shtylyov <[email protected]>
Cc Jeff Garzik <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

ACK