Re: [PATCH] drivers/ide/qd65xx: no cli/sti (2.4.19-pre3 & 2.5.28)

Samuel Thibault (samuel.thibault@fnac.net)
Thu, 25 Jul 2002 17:15:42 +0200 (CEST)


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);

-
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/