Re: [PATCH 3/15] qd65xx: fix deadlock on error handling
Bartlomiej Zolnierkiewicz wrote:
On Wednesday 03 October 2007, Jeff Garzik wrote:
Bartlomiej Zolnierkiewicz wrote:
ACK everything else (that I snipped)...
@@ -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.
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]
ACK
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/15] qd65xx: fix deadlock on error handling
Bartlomiej Zolnierkiewicz wrote:
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
===
--- a/drivers/ide/legacy/qd65xx.c
+++ 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;
}
I don't see why all the locking above is needed at all -- isn't these
atomic functions? :-/
@@ -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);
Heh, 486 with VLB hardly needed spinlocks at all... although there *were*
486 SMP machines -- with external LAPICs.
MBR, Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/15] qd65xx: fix deadlock on error handling
Sergei Shtylyov wrote:
Bartlomiej Zolnierkiewicz wrote:
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
===
--- a/drivers/ide/legacy/qd65xx.c
+++ 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;
}
I don't see why all the locking above is needed at all -- isn't these
atomic functions? :-/
Agreed.
Jeff
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/15] qd65xx: fix deadlock on error handling
Bartlomiej Zolnierkiewicz wrote:
ACK everything else (that I snipped)...
@@ -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
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[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(-)
Index: b/drivers/ide/legacy/qd65xx.c
===
--- a/drivers/ide/legacy/qd65xx.c
+++ 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);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
