Hello,
Here are patches for 2.4.19-pre3 & 2.5.28 which free them from using
cli/sti in qd65xx stuff. (also using ide's OUT_BYTE / IN_BYTE btw)
IMHO, it may use its own spinlock, instead of using io_request_lock as
suggested in pre3-ac, since what we have to protect is this card from
parallel selectprocing 2 channels at a time which may upset the board (I
don't know, and don't have a vlb smp system to test)
2 qd6500 boards may be ok to parallelize it, I don't know (I don't have
any)...
for 2.4.19rc3:
--- linux-2.4.19rc3/drivers/ide/qd65xx.c Thu Jul 25 01:03:28 2002
+++ linux-2.4.19rc3/drivers/ide/qd65xx.c Thu Jul 25 01:26:33 2002
@@ -88,14 +88,15 @@
static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
+static spinlock_t qd_lock = SPIN_LOCK_UNLOCKED; /* lock for i/o operations */
+
static void qd_write_reg (byte content, byte reg)
{
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- outb(content,reg);
- restore_flags(flags); /* all CPUs */
+ spin_lock_irqsave(&qd_lock, flags);
+ OUT_BYTE(content,reg);
+ spin_unlock_irqrestore(&qd_lock, flags);
}
byte __init qd_read_reg (byte reg)
@@ -103,10 +104,9 @@
unsigned long flags;
byte read;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- read = inb(reg);
- restore_flags(flags); /* all CPUs */
+ spin_lock_irqsave(&qd_lock, flags);
+ read = IN_BYTE(reg);
+ spin_unlock_irqrestore(&qd_lock, flags);
return read;
}
@@ -311,13 +311,12 @@
byte readreg;
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- savereg = inb_p(port);
- outb_p(QD_TESTVAL,port); /* safe value */
- readreg = inb_p(port);
- outb(savereg,port);
- restore_flags(flags); /* all CPUs */
+ spin_lock_irqsave(&qd_lock, flags);
+ savereg = IN_BYTE(port);
+ OUT_BYTE(QD_TESTVAL,port); /* safe value */
+ readreg = IN_BYTE(port);
+ OUT_BYTE(savereg,port);
+ spin_unlock_irqrestore(&qd_lock, flags);
if (savereg == QD_TESTVAL) {
printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");
@@ -336,7 +335,7 @@
* return 1 if another qd may be probed
*/
-int __init probe (int base)
+static int __init qd_probe(int base)
{
byte config;
byte index;
@@ -449,5 +448,5 @@
void __init init_qd65xx (void)
{
- if (probe(0x30)) probe(0xb0);
+ if (qd_probe(0x30)) qd_probe(0xb0);
}
(also corrected silly non-static probe function !)
for 2.5.28:
--- linux-2.5.28/drivers/ide/qd65xx.c Thu Jul 25 01:10:26 2002
+++ linux-2.5.28/drivers/ide/qd65xx.c Thu Jul 25 01:09:09 2002
@@ -85,14 +85,15 @@
static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
+static spinlock_t qd_lock = SPIN_LOCK_UNLOCKED; /* lock for i/o operations */
+
static void qd_write_reg(byte content, byte reg)
{
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- outb(content,reg);
- restore_flags(flags); /* all CPUs */
+ spin_lock_irqsave(&qd_lock, flags);
+ OUT_BYTE(content,reg);
+ spin_unlock_irqrestore(&qd_lock, flags);
}
byte __init qd_read_reg(byte reg)
@@ -100,10 +101,9 @@
unsigned long flags;
byte read;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- read = inb(reg);
- restore_flags(flags); /* all CPUs */
+ spin_lock_irqsave(&qd_lock, flags);
+ read = IN_BYTE(reg);
+ spin_unlock_irqrestore(&qd_lock, flags);
return read;
}
@@ -309,13 +309,12 @@
byte readreg;
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- savereg = inb_p(port);
- outb_p(QD_TESTVAL, port); /* safe value */
- readreg = inb_p(port);
- outb(savereg, port);
- restore_flags(flags); /* all CPUs */
+ spin_lock_irqsave(&qd_lock, flags);
+ savereg = IN_BYTE(port);
+ OUT_BYTE(QD_TESTVAL,port); /* safe value */
+ readreg = IN_BYTE(port);
+ OUT_BYTE(savereg,port);
+ spin_unlock_irqrestore(&qd_lock, flags);
if (savereg == QD_TESTVAL) {
printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");
Regards,
Samuel Thibault
On Thu, Jul 25, 2002 at 01:45:00AM +0200, Samuel Thibault wrote:
> Hello,
>
> Here are patches for 2.4.19-pre3 & 2.5.28 which free them from using
> cli/sti in qd65xx stuff.
Cool.
> (also using ide's OUT_BYTE / IN_BYTE btw)
In my opinion this doesn't make sense. The qd65xx is a VESA Local Bus
only hardware and is very very unlikely to be used on anything else than
an x86, where these defines are needed. Also, the ports written to are
not a part of the IDE controller region, so the IN_BYTE/OUT_BYTE macros
might not work there if it was ever used on a non-x86 machine. Also, it
makes the code less readable.
> IMHO, it may use its own spinlock, instead of using io_request_lock as
> suggested in pre3-ac, since what we have to protect is this card from
> parallel selectprocing 2 channels at a time which may upset the board (I
> don't know, and don't have a vlb smp system to test)
>
> 2 qd6500 boards may be ok to parallelize it, I don't know (I don't have
> any)...
>
> for 2.4.19rc3:
>
> --- linux-2.4.19rc3/drivers/ide/qd65xx.c Thu Jul 25 01:03:28 2002
> +++ linux-2.4.19rc3/drivers/ide/qd65xx.c Thu Jul 25 01:26:33 2002
> @@ -88,14 +88,15 @@
>
> static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
>
> +static spinlock_t qd_lock = SPIN_LOCK_UNLOCKED; /* lock for i/o operations */
> +
> static void qd_write_reg (byte content, byte reg)
> {
> unsigned long flags;
>
> - save_flags(flags); /* all CPUs */
> - cli(); /* all CPUs */
> - outb(content,reg);
> - restore_flags(flags); /* all CPUs */
> + spin_lock_irqsave(&qd_lock, flags);
> + OUT_BYTE(content,reg);
> + spin_unlock_irqrestore(&qd_lock, flags);
> }
>
> byte __init qd_read_reg (byte reg)
> @@ -103,10 +104,9 @@
> unsigned long flags;
> byte read;
>
> - save_flags(flags); /* all CPUs */
> - cli(); /* all CPUs */
> - read = inb(reg);
> - restore_flags(flags); /* all CPUs */
> + spin_lock_irqsave(&qd_lock, flags);
> + read = IN_BYTE(reg);
> + spin_unlock_irqrestore(&qd_lock, flags);
> return read;
> }
>
> @@ -311,13 +311,12 @@
> byte readreg;
> unsigned long flags;
>
> - save_flags(flags); /* all CPUs */
> - cli(); /* all CPUs */
> - savereg = inb_p(port);
> - outb_p(QD_TESTVAL,port); /* safe value */
> - readreg = inb_p(port);
> - outb(savereg,port);
> - restore_flags(flags); /* all CPUs */
> + spin_lock_irqsave(&qd_lock, flags);
> + savereg = IN_BYTE(port);
> + OUT_BYTE(QD_TESTVAL,port); /* safe value */
> + readreg = IN_BYTE(port);
> + OUT_BYTE(savereg,port);
> + spin_unlock_irqrestore(&qd_lock, flags);
>
> if (savereg == QD_TESTVAL) {
> printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");
> @@ -336,7 +335,7 @@
> * return 1 if another qd may be probed
> */
>
> -int __init probe (int base)
> +static int __init qd_probe(int base)
> {
> byte config;
> byte index;
> @@ -449,5 +448,5 @@
>
> void __init init_qd65xx (void)
> {
> - if (probe(0x30)) probe(0xb0);
> + if (qd_probe(0x30)) qd_probe(0xb0);
> }
>
> (also corrected silly non-static probe function !)
>
> for 2.5.28:
>
> --- linux-2.5.28/drivers/ide/qd65xx.c Thu Jul 25 01:10:26 2002
> +++ linux-2.5.28/drivers/ide/qd65xx.c Thu Jul 25 01:09:09 2002
> @@ -85,14 +85,15 @@
>
> static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
>
> +static spinlock_t qd_lock = SPIN_LOCK_UNLOCKED; /* lock for i/o operations */
> +
> static void qd_write_reg(byte content, byte reg)
> {
> unsigned long flags;
>
> - save_flags(flags); /* all CPUs */
> - cli(); /* all CPUs */
> - outb(content,reg);
> - restore_flags(flags); /* all CPUs */
> + spin_lock_irqsave(&qd_lock, flags);
> + OUT_BYTE(content,reg);
> + spin_unlock_irqrestore(&qd_lock, flags);
> }
>
> byte __init qd_read_reg(byte reg)
> @@ -100,10 +101,9 @@
> unsigned long flags;
> byte read;
>
> - save_flags(flags); /* all CPUs */
> - cli(); /* all CPUs */
> - read = inb(reg);
> - restore_flags(flags); /* all CPUs */
> + spin_lock_irqsave(&qd_lock, flags);
> + read = IN_BYTE(reg);
> + spin_unlock_irqrestore(&qd_lock, flags);
> return read;
> }
>
> @@ -309,13 +309,12 @@
> byte readreg;
> unsigned long flags;
>
> - save_flags(flags); /* all CPUs */
> - cli(); /* all CPUs */
> - savereg = inb_p(port);
> - outb_p(QD_TESTVAL, port); /* safe value */
> - readreg = inb_p(port);
> - outb(savereg, port);
> - restore_flags(flags); /* all CPUs */
> + spin_lock_irqsave(&qd_lock, flags);
> + savereg = IN_BYTE(port);
> + OUT_BYTE(QD_TESTVAL,port); /* safe value */
> + readreg = IN_BYTE(port);
> + OUT_BYTE(savereg,port);
> + spin_unlock_irqrestore(&qd_lock, flags);
>
> if (savereg == QD_TESTVAL) {
> printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");
>
>
> Regards,
>
> Samuel Thibault
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Vojtech Pavlik
SuSE Labs
Vojtech Pavlik wrote:
> On Thu, Jul 25, 2002 at 01:45:00AM +0200, Samuel Thibault wrote:
>
>>Hello,
>>
>>Here are patches for 2.4.19-pre3 & 2.5.28 which free them from using
>>cli/sti in qd65xx stuff.
>
>
> Cool.
>
>
>>(also using ide's OUT_BYTE / IN_BYTE btw)
>
>
> In my opinion this doesn't make sense. The qd65xx is a VESA Local Bus
> only hardware and is very very unlikely to be used on anything else than
> an x86, where these defines are needed. Also, the ports written to are
> not a part of the IDE controller region, so the IN_BYTE/OUT_BYTE macros
> might not work there if it was ever used on a non-x86 machine. Also, it
> makes the code less readable.
Amen. BTW> I think proper fix is to simple *remove* the cli() cti()
commands. They don't make much sense in first place.
On Thu, Jul 25, 2002 at 10:25:26AM +0200, Marcin Dalecki wrote:
> Vojtech Pavlik wrote:
> > On Thu, Jul 25, 2002 at 01:45:00AM +0200, Samuel Thibault wrote:
> >
> >>Hello,
> >>
> >>Here are patches for 2.4.19-pre3 & 2.5.28 which free them from using
> >>cli/sti in qd65xx stuff.
> >
> >
> > Cool.
> >
> >
> >>(also using ide's OUT_BYTE / IN_BYTE btw)
> >
> >
> > In my opinion this doesn't make sense. The qd65xx is a VESA Local Bus
> > only hardware and is very very unlikely to be used on anything else than
> > an x86, where these defines are needed. Also, the ports written to are
> > not a part of the IDE controller region, so the IN_BYTE/OUT_BYTE macros
> > might not work there if it was ever used on a non-x86 machine. Also, it
> > makes the code less readable.
>
> Amen. BTW> I think proper fix is to simple *remove* the cli() cti()
> commands. They don't make much sense in first place.
Yes. Btw, it would be nice if the IDE core guaranteed that no two tuning
functions would be ever run at the same time. No performance hit, life a
lot easier.
--
Vojtech Pavlik
SuSE Labs
On Wed, 24 Jul 2002, Andre Hedrick wrote:
> You just dropped IO sync with the main loop, it is a dead dog.
Why sync with the main loop ?
On Thu, 25 Jul 2002, Samuel Thibault wrote:
>
> On Wed, 24 Jul 2002, Andre Hedrick wrote:
>
> > You just dropped IO sync with the main loop, it is a dead dog.
>
> Why sync with the main loop ?
I am sorry, I sent this offline where it was intended to be!
Cheers,
Andre Hedrick
LAD Storage Consulting Group
On Thu, 25 Jul 2002, Vojtech Pavlik wrote:
> so the IN_BYTE/OUT_BYTE macros might not work there.
and
> it would be nice if the IDE core guaranteed that no two tuning
> functions would be ever run at the same time. No performance hit, life
> a lot easier.
Oops, I didn't realize that...
Here are corrected patches, according to your notes:
I eventually completely removed qd_read_reg, since it is only needed when
probing when one has to read config & control registers: nothing to do
with tuning or selecting, it shouldn't hurt the board.
--- linux-2.4.19rc3/drivers/ide/qd65xx.c
+++ linux-2.4.19rc3/drivers/ide/qd65xx.c
@@ -90,32 +90,23 @@
static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
+static spinlock_t qd_iolock = SPIN_LOCK_UNLOCKED; /* lock for i/o operations */
+static spinlock_t qd_timlock = SPIN_LOCK_UNLOCKED; /* lock for time computing */
+
static void qd_write_reg (byte content, byte reg)
{
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
+ spin_lock_irqsave(&qd_iolock,flags);
outb(content,reg);
- restore_flags(flags); /* all CPUs */
-}
-
-byte __init qd_read_reg (byte reg)
-{
- unsigned long flags;
- byte read;
-
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- read = inb(reg);
- restore_flags(flags); /* all CPUs */
- return read;
+ spin_unlock_irqrestore(&qd_iolock,flags);
}
/*
* qd_select:
*
* This routine is invoked from ide.c to prepare for access to a given drive.
+ * A timing reg can only be shared within one ata channel.
*/
static void qd_select (ide_drive_t *drive)
@@ -214,15 +205,19 @@
static void qd_set_timing (ide_drive_t *drive, byte timing)
{
+ unsigned long flags;
ide_hwif_t *hwif = HWIF(drive);
drive->drive_data &= 0xff00;
drive->drive_data |= timing;
+
+ spin_lock_irqsave(&qd_timlock,flags);
if (qd_timing_ok(hwif->drives)) {
qd_select(drive); /* selects once */
hwif->selectproc = NULL;
} else
hwif->selectproc = &qd_select;
+ spin_unlock_irqrestore(&qd_timlock,flags);
printk(KERN_DEBUG "%s: %#x\n",drive->name,timing);
}
@@ -313,13 +308,12 @@
byte readreg;
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
+ spin_lock_irqsave(&qd_iolock,flags);
savereg = inb_p(port);
outb_p(QD_TESTVAL,port); /* safe value */
readreg = inb_p(port);
outb(savereg,port);
- restore_flags(flags); /* all CPUs */
+ spin_unlock_irqrestore(&qd_iolock,flags);
if (savereg == QD_TESTVAL) {
printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");
@@ -338,12 +332,12 @@
* return 1 if another qd may be probed
*/
-int __init probe (int base)
+static int __init qd_probe (int base)
{
byte config;
byte index;
- config = qd_read_reg(QD_CONFIG_PORT);
+ config = inb(QD_CONFIG_PORT);
if (! ((config & QD_CONFIG_BASEPORT) >> 1 == (base == 0xb0)) ) return 1;
@@ -387,7 +381,7 @@
/* 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",
@@ -403,8 +397,8 @@
hwif->chipset = ide_qd65xx;
hwif->select_data = base;
hwif->config_data = config | (control <<8);
- hwif->drives[0].drive_data =
- hwif->drives[1].drive_data = QD6580_DEF_DATA;
+ hwif->drives[0].drive_data = QD6580_DEF_DATA;
+ hwif->drives[1].drive_data = QD6580_DEF_DATA2;
hwif->drives[0].io_32bit =
hwif->drives[1].io_32bit = 1;
hwif->tuneproc = &qd6580_tune_drive;
@@ -452,5 +446,5 @@
void __init init_qd65xx (void)
{
- if (probe(0x30)) probe(0xb0);
+ if (qd_probe(0x30)) qd_probe(0xb0);
}
--- linux-2.5.28/drivers/ide/qd65xx.c
+++ linux-2.5.28/drivers/ide/qd65xx.c
@@ -85,32 +85,23 @@
static int timings[4]={-1,-1,-1,-1}; /* stores current timing for each timer */
+static spinlock_t qd_iolock = SPIN_LOCK_UNLOCKED; /* lock for i/o operations */
+static spinlock_t qd_timlock = SPIN_LOCK_UNLOCKED; /* lock for time computing */
+
static void qd_write_reg(byte content, byte reg)
{
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
+ spin_lock_irqsave(&qd_iolock,flags);
outb(content,reg);
- restore_flags(flags); /* all CPUs */
-}
-
-byte __init qd_read_reg(byte reg)
-{
- unsigned long flags;
- byte read;
-
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
- read = inb(reg);
- restore_flags(flags); /* all CPUs */
- return read;
+ spin_unlock_irqrestore(&qd_iolock,flags);
}
/*
* qd_select:
*
* This routine is invoked from ide.c to prepare for access to a given drive.
+ * A timing reg can only be shared within one ata channel.
*/
static void qd_select(struct ata_device *drive)
@@ -207,15 +198,19 @@
static void qd_set_timing(struct ata_device *drive, byte timing)
{
+ unsigned long flags;
struct ata_channel *hwif = drive->channel;
drive->drive_data &= 0xff00;
drive->drive_data |= timing;
+
+ spin_lock_irqsave(&qd_timlock,flags);
if (qd_timing_ok(hwif->drives)) {
qd_select(drive); /* selects once */
hwif->selectproc = NULL;
} else
hwif->selectproc = &qd_select;
+ spin_unlock_irqrestore(&qd_timlock,flags);
printk(KERN_DEBUG "%s: %#x\n", drive->name, timing);
}
@@ -309,13 +304,12 @@
byte readreg;
unsigned long flags;
- save_flags(flags); /* all CPUs */
- cli(); /* all CPUs */
+ spin_lock_irqsave(&qd_iolock,flags);
savereg = inb_p(port);
outb_p(QD_TESTVAL, port); /* safe value */
readreg = inb_p(port);
outb(savereg, port);
- restore_flags(flags); /* all CPUs */
+ spin_unlock_irqrestore(&qd_iolock,flags);
if (savereg == QD_TESTVAL) {
printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");
@@ -333,7 +327,7 @@
* called to setup an ata channel : adjusts attributes & links for tuning
*/
-void __init qd_setup(int unit, int base, int config, unsigned int data0, unsigned int data1, void (*tuneproc) (struct ata_device *, byte pio))
+static void __init qd_setup(int unit, int base, int config, unsigned int data0, unsigned int data1, void (*tuneproc) (struct ata_device *, byte pio))
{
struct ata_channel *hwif = &ide_hwifs[unit];
@@ -352,7 +346,7 @@
*
* called to unsetup an ata channel : back to default values, unlinks tuning
*/
-void __init qd_unsetup(int unit) {
+static void __init qd_unsetup(int unit) {
struct ata_channel *hwif = &ide_hwifs[unit];
byte config = hwif->config_data;
int base = hwif->select_data;
@@ -388,12 +382,12 @@
* return 1 if another qd may be probed
*/
-int __init qd_probe(int base)
+static int __init qd_probe(int base)
{
byte config;
int unit;
- config = qd_read_reg(QD_CONFIG_PORT);
+ config = inb(QD_CONFIG_PORT);
if (! ((config & QD_CONFIG_BASEPORT) >> 1 == (base == 0xb0)) ) return 1;
@@ -424,7 +418,7 @@
/* 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", config, control, QD_ID3);
On Thu, 25 Jul 2002, Samuel Thibault wrote:
> static void qd_write_reg (byte content, byte reg)
> {
> unsigned long flags;
>
> - save_flags(flags); /* all CPUs */
> - cli(); /* all CPUs */
> + spin_lock_irqsave(&qd_iolock,flags);
> outb(content,reg);
Do we need a lock/cli for that one outb?
> + spin_unlock_irqrestore(&qd_iolock,flags);
Zwane
--
function.linuxpower.ca
On Thu, 25 Jul 2002, Zwane Mwaikambo wrote:
> On Thu, 25 Jul 2002, Samuel Thibault wrote:
>
> > static void qd_write_reg (byte content, byte reg)
> > {
> > unsigned long flags;
> >
> > - save_flags(flags); /* all CPUs */
> > - cli(); /* all CPUs */
> > + spin_lock_irqsave(&qd_iolock,flags);
> > outb(content,reg);
>
> Do we need a lock/cli for that one outb?
>
> > + spin_unlock_irqrestore(&qd_iolock,flags);
Well, I put it since many ide chipset drivers put it, but we may indeed
get rid of it, provided the board isn't upset when parallel selectprocing
/ tuning on ide0 and ide1. I won't be able to test before September.
Oh, btw, I only use one spinlock for timing computing, while there could
be one per QD channel, which would speed up tunig :o)
--
Samuel Thibault <[email protected]>
Hi ! I'm a .signature virus ! Copy me into your ~/.signature, please !