[RFC/CFT] cmd640 irqlocking fixes

[RFC/CFT] cmd640 irqlocking fixes

[William Lee Irwin III]

I don't have one of these, and I'm not even sure what it is. But here's
a wild guess at a fix.


Cheers,
Bill
===== drivers/ide/cmd640.c 1.11 vs edited =====
--- 1.11/drivers/ide/cmd640.c	Wed May 22 04:21:11 2002
+++ edited/drivers/ide/cmd640.c	Wed Jul 24 18:51:54 2002
@@ -115,6 +115,12 @@
 #include "ata-timing.h"
 
 /*
+ * Is this remotely correct?
+ */
+static spinlock_t cmd640_lock = SPIN_LOCK_UNLOCKED;
+
+
+/*
  * This flag is set in ide.c by the parameter:  ide0=cmd640_vlb
  */
 int cmd640_vlb = 0;
@@ -220,11 +226,10 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
 	outb_p(val, (reg & 3) | 0xcfc);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 }
 
 static u8 get_cmd640_reg_pci1 (unsigned short reg)
@@ -232,11 +237,10 @@
 	u8 b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
 	b = inb_p((reg & 3) | 0xcfc);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 	return b;
 }
 
@@ -246,12 +250,11 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	outb_p(0x10, 0xcf8);
 	outb_p(val, cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 }
 
 static u8 get_cmd640_reg_pci2 (unsigned short reg)
@@ -259,12 +262,11 @@
 	u8 b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	outb_p(0x10, 0xcf8);
 	b = inb_p(cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 	return b;
 }
 
@@ -274,11 +276,10 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	outb_p(reg, cmd640_key);
 	outb_p(val, cmd640_key + 4);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 }
 
 static u8 get_cmd640_reg_vlb (unsigned short reg)
@@ -286,11 +287,10 @@
 	u8 b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	outb_p(reg, cmd640_key);
 	b = inb_p(cmd640_key + 4);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 	return b;
 }
 
@@ -367,8 +367,7 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 
 	outb_p(0x0a, 0x170 + IDE_SELECT_OFFSET);	/* select drive0 */
 	udelay(100);
@@ -376,11 +375,11 @@
 		outb_p(0x1a, 0x170 + IDE_SELECT_OFFSET); /* select drive1 */
 		udelay(100);
 		if ((inb_p(0x170 + IDE_SELECT_OFFSET) & 0x1f) != 0x1a) {
-			restore_flags(flags);
+			spin_unlock_irqrestore(&cmd640_lock, flags);
 			return 0; /* nothing responded */
 		}
 	}
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 	return 1; /* success */
 }
 
@@ -461,8 +460,7 @@
 	u8 b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	b = get_cmd640_reg(reg);
 	if (mode) {	/* want prefetch on? */
 # if CMD640_PREFETCH_MASKS
@@ -478,7 +476,7 @@
 		b |= prefetch_masks[index];	/* disable prefetch */
 	}
 	put_cmd640_reg(reg, b);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 }
 
 /*
@@ -579,8 +577,7 @@
 	/*
 	 * Now that everything is ready, program the new timings
 	 */
-	save_flags (flags);
-	cli();
+	spin_lock_irqsave(&cmd640_lock, flags);
 	/*
 	 * Program the address_setup clocks into ARTTIM reg,
 	 * and then the active/recovery counts into the DRWTIM reg
@@ -589,7 +586,7 @@
 	setup_count |= get_cmd640_reg(arttim_regs[index]) & 0x3f;
 	put_cmd640_reg(arttim_regs[index], setup_count);
 	put_cmd640_reg(drwtim_regs[index], pack_nibbles(active_count, recovery_count));
-	restore_flags(flags);
+	spin_unlock_irqrestore(&cmd640_lock, flags);
 }
 
 /*

Re: [RFC/CFT] cmd640 irqlocking fixes

[William Lee Irwin III]

On Wed, Jul 24, 2002 at 03:58:26PM -0700, William Lee Irwin III wrote:
> I don't have one of these, and I'm not even sure what it is. But here's
> a wild guess at a fix.

Please disregard. I've been informed of several errors in this patch.


Cheers,
Bill

Re: [RFC/CFT] cmd640 irqlocking fixes

[Alan Cox]

On Wed, 2002-07-24 at 23:58, William Lee Irwin III wrote:
> I don't have one of these, and I'm not even sure what it is. But here's
> a wild guess at a fix.

These must be locked against any other PCI access so it needs to share
the lock with arch/i386/kernel/pci*.c. The code is also wrong for other
reasons and there are some fixes in the 2.4.19-ac tree to those
functions that affect the locking and maybe should be merged too.

What is going on here is that we have to probe the CMD640 PCI either via
PCI conf1 or PCI conf2. We cannot use the kernel default functions
because they may trigger a bug in the CMD640 hardware.

get_cmd640_reg_pci[12] are basically reimplementations of the
pci_read_config bits for type 1/type 2 PCI configurations. The register
and lock are thus the same as the core. This lock also needs to match
the same lock on non x86 platforms so the pci config lock name should be
unified now before the brown and sticky impacts on the rotating
propellor blades

Re: [RFC/CFT] cmd640 irqlocking fixes

[Vojtech Pavlik]

On Thu, Jul 25, 2002 at 02:05:11AM +0100, Alan Cox wrote:

> > I don't have one of these, and I'm not even sure what it is. But here's
> > a wild guess at a fix.
> 
> These must be locked against any other PCI access so it needs to share
> the lock with arch/i386/kernel/pci*.c. The code is also wrong for other
> reasons and there are some fixes in the 2.4.19-ac tree to those
> functions that affect the locking and maybe should be merged too.
> 
> What is going on here is that we have to probe the CMD640 PCI either via
> PCI conf1 or PCI conf2. We cannot use the kernel default functions
> because they may trigger a bug in the CMD640 hardware.

> get_cmd640_reg_pci[12] are basically reimplementations of the
> pci_read_config bits for type 1/type 2 PCI configurations. The register
> and lock are thus the same as the core. This lock also needs to match
> the same lock on non x86 platforms so the pci config lock name should be
> unified now before the brown and sticky impacts on the rotating
> propellor blades

The kernel functions are OK. The problem is that the kernel can use
PCIBIOS calls to set the registers. And certain old buggy BIOSes which
violate the PCI spec can use wrong size data transfers to set the
registers, which the CMD640 doesn't like.

IMHO the best workaround here would be either to disable PCIBIOS calls
and revert to conf1 or conf2 in the PCI code if a CMD640 is present, or
just panic() in the CMD640 code and suggest to the user to use
"pci=nobios" on the kernel command line. I'd actually prefer the later.

-- 
Vojtech Pavlik
SuSE Labs

Re: [RFC/CFT] cmd640 irqlocking fixes

[Marcin Dalecki]

Vojtech Pavlik wrote:

> 
> The kernel functions are OK. The problem is that the kernel can use
> PCIBIOS calls to set the registers. And certain old buggy BIOSes which
> violate the PCI spec can use wrong size data transfers to set the
> registers, which the CMD640 doesn't like.
> 
> IMHO the best workaround here would be either to disable PCIBIOS calls
> and revert to conf1 or conf2 in the PCI code if a CMD640 is present, or
> just panic() in the CMD640 code and suggest to the user to use
> "pci=nobios" on the kernel command line. I'd actually prefer the later.
> 

 From a long long time ago during the first days of this driver I 
remember that those chips could be wired to both PCI and VLB(ISA) bus.
And this is the main reaons why the functions is question exist in first 
place -> "emulating" PCI configuration space access on VLB.

Re: [RFC/CFT] cmd640 irqlocking fixes

[Vojtech Pavlik]

On Thu, Jul 25, 2002 at 10:28:56AM +0200, Marcin Dalecki wrote:
> Vojtech Pavlik wrote:
> 
> > 
> > The kernel functions are OK. The problem is that the kernel can use
> > PCIBIOS calls to set the registers. And certain old buggy BIOSes which
> > violate the PCI spec can use wrong size data transfers to set the
> > registers, which the CMD640 doesn't like.
> > 
> > IMHO the best workaround here would be either to disable PCIBIOS calls
> > and revert to conf1 or conf2 in the PCI code if a CMD640 is present, or
> > just panic() in the CMD640 code and suggest to the user to use
> > "pci=nobios" on the kernel command line. I'd actually prefer the later.
> > 
> 
>  From a long long time ago during the first days of this driver I 
> remember that those chips could be wired to both PCI and VLB(ISA) bus.
> And this is the main reaons why the functions is question exist in first 
> place -> "emulating" PCI configuration space access on VLB.

No. For VLB the CMD640 has a somewhat different configuration method.
See the source. ;) We really should be using pci_write_config_* and
create vlb_write_config_* in CMD640 for the VLB accesses, panic() in
case we have a PCI system that uses BIOS and we found a CMD640, and
remove the duplicate PCI conf1 and PCI conf2 code from cmd640.c

-- 
Vojtech Pavlik
SuSE Labs

Re: [RFC/CFT] cmd640 irqlocking fixes

[Marcin Dalecki]

Vojtech Pavlik wrote:
> On Thu, Jul 25, 2002 at 10:28:56AM +0200, Marcin Dalecki wrote:
> 
>>Vojtech Pavlik wrote:
>>
>>
>>>The kernel functions are OK. The problem is that the kernel can use
>>>PCIBIOS calls to set the registers. And certain old buggy BIOSes which
>>>violate the PCI spec can use wrong size data transfers to set the
>>>registers, which the CMD640 doesn't like.
>>>
>>>IMHO the best workaround here would be either to disable PCIBIOS calls
>>>and revert to conf1 or conf2 in the PCI code if a CMD640 is present, or
>>>just panic() in the CMD640 code and suggest to the user to use
>>>"pci=nobios" on the kernel command line. I'd actually prefer the later.
>>>
>>
>> From a long long time ago during the first days of this driver I 
>>remember that those chips could be wired to both PCI and VLB(ISA) bus.
>>And this is the main reaons why the functions is question exist in first 
>>place -> "emulating" PCI configuration space access on VLB.
> 
> 
> No. For VLB the CMD640 has a somewhat different configuration method.
> See the source. ;) We really should be using pci_write_config_* and
> create vlb_write_config_* in CMD640 for the VLB accesses, panic() in
> case we have a PCI system that uses BIOS and we found a CMD640, and
> remove the duplicate PCI conf1 and PCI conf2 code from cmd640.c

OK. Right. We have to touch this code anyway. Do you know first hand how
to detect programmatically which configuration method is in charge? If 
not I can look it up on my own..

Re: [RFC/CFT] cmd640 irqlocking fixes

[Alan Cox]

On Thu, 2002-07-25 at 09:56, Marcin Dalecki wrote:
> OK. Right. We have to touch this code anyway. Do you know first hand how
> to detect programmatically which configuration method is in charge? If 
> not I can look it up on my own..

Just copy the code from 2.4.19-rc3-ac3. Andre didnt write it so you
don't have to pretend it doesn't exist. I'll do this and test it since I
did the original fixes in 2.4. Expect a patch later today

Re: [RFC/CFT] cmd640 irqlocking fixes

[Marcin Dalecki]

Alan Cox wrote:
> On Thu, 2002-07-25 at 09:56, Marcin Dalecki wrote:
> 
>>OK. Right. We have to touch this code anyway. Do you know first hand how
>>to detect programmatically which configuration method is in charge? If 
>>not I can look it up on my own..
> 
> 
> Just copy the code from 2.4.19-rc3-ac3. Andre didnt write it so you
> don't have to pretend it doesn't exist

Well...I *would* *not* mind patches from Andre.

Hell, I would take patches even from Saddam personally if they where
looking OK. And I look from time to time at the 2.4.xx tree. But I don't
look at the pre ac or whatever releases. Let them settle out
first :-) And last but not least most of the sorting out for particular 
host controller register tweaking is done by others not me...

 From Andre I only saw a single patch once a long long time ago
attached to full load of the usual personal insults.
It didn't even apply cleanly to the kernel tree it was supposed to apply 
too and it was "obviously" not correct as well.

Saddm, on the other side, apparently doesn't do Linux developement.
But Jamie Zawinski is still alive at least :-).

Re: [RFC/CFT] cmd640 irqlocking fixes

[Andre Hedrick]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]


On 25 Jul 2002, Alan Cox wrote:

> On Thu, 2002-07-25 at 09:56, Marcin Dalecki wrote:
> > OK. Right. We have to touch this code anyway. Do you know first hand how
> > to detect programmatically which configuration method is in charge? If 
> > not I can look it up on my own..
> 
> Just copy the code from 2.4.19-rc3-ac3. Andre didnt write it so you
> don't have to pretend it doesn't exist. I'll do this and test it since I
> did the original fixes in 2.4. Expect a patch later today

I am sorry Alan, but I fixed all of the locking code in 2.4 a long time
ago, and you adopted it somewhere around this patch.

Please check your patch revisions, and I can retrive out of archives
the date and time when I finally fixed it for all of x86 and then when I
started reworking all the arch specifics.  Noting that I had broken the
ia64 locking and DM of HP replied to me offering help to solve the need
for the local_irq_set() calls which are need currently.

I have now fix all the asm-*/system.h asm-*/hw_irq.h etc with proper
assembley calls.  

Again I have to call this patch and fix and take credit and full ownership
of removing all the save/cli/sti/restore which littered the driver and
were spread like cow patties through a chopper gun.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

[-- Attachment #2: Type: text/plain, Size: 5910 bytes --]

diff -urN linux-2.4.19-p8-ac1-pristine/drivers/ide/cmd640.c linux-2.4.19-p8-ac1/drivers/ide/cmd640.c
--- linux-2.4.19-p8-ac1-pristine/drivers/ide/cmd640.c	Fri May 10 10:10:21 2002
+++ linux-2.4.19-p8-ac1/drivers/ide/cmd640.c	Fri May 10 05:15:51 2002
@@ -217,11 +217,10 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
-	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
+	spin_lock_irqsave(&io_request_lock, flags);
+	outb_p((reg & 0xfc) | cmd640_key, 0xcf8);
 	outb_p(val, (reg & 3) | 0xcfc);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 static byte get_cmd640_reg_pci1 (unsigned short reg)
@@ -229,11 +228,10 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
-	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
+	spin_lock_irqsave(&io_request_lock, flags);
+	outb_p((reg & 0xfc) | cmd640_key, 0xcf8);
 	b = inb_p((reg & 3) | 0xcfc);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return b;
 }
 
@@ -243,12 +241,11 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(0x10, 0xcf8);
 	outb_p(val, cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 static byte get_cmd640_reg_pci2 (unsigned short reg)
@@ -256,12 +253,11 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(0x10, 0xcf8);
 	b = inb_p(cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return b;
 }
 
@@ -271,11 +267,10 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(reg, cmd640_key);
 	outb_p(val, cmd640_key + 4);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 static byte get_cmd640_reg_vlb (unsigned short reg)
@@ -283,11 +278,10 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(reg, cmd640_key);
 	b = inb_p(cmd640_key + 4);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return b;
 }
 
@@ -315,7 +309,9 @@
 {
 	get_cmd640_reg = get_cmd640_reg_pci1;
 	put_cmd640_reg = put_cmd640_reg_pci1;
-	for (cmd640_key = 0x80000000; cmd640_key <= 0x8000f800; cmd640_key += 0x800) {
+	for (cmd640_key = 0x80000000;
+	     cmd640_key <= 0x8000f800;
+	     cmd640_key += 0x800) {
 		if (match_pci_cmd640_device())
 			return 1; /* success */
 	}
@@ -364,8 +360,7 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 
 	outb_p(0x0a, 0x170 + IDE_SELECT_OFFSET);	/* select drive0 */
 	udelay(100);
@@ -373,11 +368,11 @@
 		outb_p(0x1a, 0x170 + IDE_SELECT_OFFSET); /* select drive1 */
 		udelay(100);
 		if ((inb_p(0x170 + IDE_SELECT_OFFSET) & 0x1f) != 0x1a) {
-			restore_flags(flags);
+			spin_unlock_irqrestore(&io_request_lock, flags);
 			return 0; /* nothing responded */
 		}
 	}
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return 1; /* success */
 }
 
@@ -458,8 +453,7 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	b = get_cmd640_reg(reg);
 	if (mode) {	/* want prefetch on? */
 #if CMD640_PREFETCH_MASKS
@@ -475,7 +469,7 @@
 		b |= prefetch_masks[index];	/* disable prefetch */
 	}
 	put_cmd640_reg(reg, b);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 /*
@@ -576,8 +570,7 @@
 	/*
 	 * Now that everything is ready, program the new timings
 	 */
-	save_flags (flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	/*
 	 * Program the address_setup clocks into ARTTIM reg,
 	 * and then the active/recovery counts into the DRWTIM reg
@@ -586,7 +579,7 @@
 	setup_count |= get_cmd640_reg(arttim_regs[index]) & 0x3f;
 	put_cmd640_reg(arttim_regs[index], setup_count);
 	put_cmd640_reg(drwtim_regs[index], pack_nibbles(active_count, recovery_count));
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 /*
@@ -697,19 +690,17 @@
 	u32 tmp;
 	unsigned long flags;
 
-	save_flags(flags);
-	__cli();
-	outb(0x01, 0xCFB);
+	spin_lock_irqsave(&io_request_lock, flags);
+	OUT_BYTE(0x01, 0xCFB);
 	tmp = inl(0xCF8);
 	outl(0x80000000, 0xCF8);
-	if(inl(0xCF8) == 0x80000000)
-	{
+	if (inl(0xCF8) == 0x80000000) {
 		outl(tmp, 0xCF8);
-		restore_flags(flags);
+		spin_unlock_irqrestore(&io_request_lock, flags);
 		return 1;
 	}
 	outl(tmp, 0xCF8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return 0;
 }
 
@@ -717,17 +708,15 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	__cli();
-	outb(0x00, 0xCFB);
-	outb(0x00, 0xCF8);
-	outb(0x00, 0xCFA);
-	if(inb(0xCF8) == 0x00 && inb(0xCF8) == 0x00)
-	{
-		restore_flags(flags);
+	spin_lock_irqsave(&io_request_lock, flags);
+	OUT_BYTE(0x00, 0xCFB);
+	OUT_BYTE(0x00, 0xCF8);
+	OUT_BYTE(0x00, 0xCFA);
+	if (IN_BYTE(0xCF8) == 0x00 && IN_BYTE(0xCF8) == 0x00) {
+		spin_unlock_irqrestore(&io_request_lock, flags);
 		return 1;
 	}
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return 0;
 }
 
@@ -748,10 +737,8 @@
 		bus_type = "VLB";
 	} else {
 		cmd640_vlb = 0;
-		/*
-		 * Don't leak I/O cycles on the PCI bus by blindly attempting
-		 * a configuration cycle type that is not supported by the hardware.
-		 */
+		/* Find out what kind of PCI probing is supported otherwise
+		   Justin Gibbs will sulk.. */
 		if (pci_conf1() && probe_for_cmd640_pci1())
 			bus_type = "PCI (type1)";
 		else if (pci_conf2() && probe_for_cmd640_pci2())

Re: [RFC/CFT] cmd640 irqlocking fixes

[Alan Cox]

On Thu, 2002-07-25 at 11:51, Andre Hedrick wrote:
> I am sorry Alan, but I fixed all of the locking code in 2.4 a long time
> ago, and you adopted it somewhere around this patch.

Andre - the PCI probe fixes which this is about is something I wrote.
The other locking stuff which is unrelated to this discussion is your
code but thats not Im talking about - OK.

> Again I have to call this patch and fix and take credit and full ownership
> of removing all the save/cli/sti/restore which littered the driver and
> were spread like cow patties through a chopper gun.

That patch is wrong by the way because I made a mistake in 2.4. Your PCI
config locking should be using pci_config lock not io_request. I'll fix
the 2.4 tree in a bit now I've tidied up the 2.5 version.

> @@ -748,10 +737,8 @@
>  		bus_type = "VLB";
>  	} else {
>  		cmd640_vlb = 0;
> -		/*
> -		 * Don't leak I/O cycles on the PCI bus by blindly attempting
> -		 * a configuration cycle type that is not supported by the hardware.
> -		 */
> +		/* Find out what kind of PCI probing is supported otherwise
> +		   Justin Gibbs will sulk.. */


Just ask Justin Gibbs. He didn't like my comment 8) Im sure he remembers
who wrote it 8)

Re: [RFC/CFT] cmd640 irqlocking fixes

[Andre Hedrick]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]


Alan,

You are partially correct; however, I can show earlier evolutions an
fragments about the locking changes but that should not be needed.

original AC cmd640.c.patch

quickest I could find that pre-dates your changes to cmd640.c
ide.2.4.19-p4.spin.03302002.patch

This will fix about 80% of the mess in 2.5 too but the rest is a goat
screw because it violates the NDA erratiums about the hardware.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

On 25 Jul 2002, Alan Cox wrote:

> On Thu, 2002-07-25 at 11:51, Andre Hedrick wrote:
> > I am sorry Alan, but I fixed all of the locking code in 2.4 a long time
> > ago, and you adopted it somewhere around this patch.
> 
> Andre - the PCI probe fixes which this is about is something I wrote.
> The other locking stuff which is unrelated to this discussion is your
> code but thats not Im talking about - OK.
> 
> > Again I have to call this patch and fix and take credit and full ownership
> > of removing all the save/cli/sti/restore which littered the driver and
> > were spread like cow patties through a chopper gun.
> 
> That patch is wrong by the way because I made a mistake in 2.4. Your PCI
> config locking should be using pci_config lock not io_request. I'll fix
> the 2.4 tree in a bit now I've tidied up the 2.5 version.
> 
> > @@ -748,10 +737,8 @@
> >  		bus_type = "VLB";
> >  	} else {
> >  		cmd640_vlb = 0;
> > -		/*
> > -		 * Don't leak I/O cycles on the PCI bus by blindly attempting
> > -		 * a configuration cycle type that is not supported by the hardware.
> > -		 */
> > +		/* Find out what kind of PCI probing is supported otherwise
> > +		   Justin Gibbs will sulk.. */
> 
> 
> Just ask Justin Gibbs. He didn't like my comment 8) Im sure he remembers
> who wrote it 8)
> 
> 
> 

[-- Attachment #2: Type: text/plain, Size: 1363 bytes --]

--- linux-2.4.19-p5/drivers/ide/cmd640.c	Fri Feb 16 16:02:36 2001
+++ linux-2.4.19-p5-ac2/drivers/ide/cmd640.c	Thu Apr  4 23:15:52 2002
@@ -692,6 +692,44 @@
 
 #endif /* CONFIG_BLK_DEV_CMD640_ENHANCED */
 
+static int pci_conf1(void)
+{
+	u32 tmp;
+	unsigned long flags;
+
+	save_flags(flags);
+	__cli();
+	outb(0x01, 0xCFB);
+	tmp = inl(0xCF8);
+	outl(0x80000000, 0xCF8);
+	if(inl(0xCF8) == 0x80000000)
+	{
+		outl(tmp, 0xCF8);
+		__restore_flags(flags);
+		return 1;
+	}
+	outl(tmp, 0xCF8);
+	return 0;
+}
+
+static int pci_conf2(void)
+{
+	unsigned long flags;
+
+	save_flags(flags);
+	__cli();
+	outb(0x00, 0xCFB);
+	outb(0x00, 0xCF8);
+	outb(0x00, 0xCFA);
+	if(inb(0xCF8) == 0x00 && inb(0xCF8) == 0x00)
+	{
+		restore_flags(flags);
+		return 1;
+	}
+	restore_flags(flags);
+	return 0;
+}
+
 /*
  * Probe for a cmd640 chipset, and initialize it if found.  Called from ide.c
  */
@@ -709,9 +747,11 @@
 		bus_type = "VLB";
 	} else {
 		cmd640_vlb = 0;
-		if (probe_for_cmd640_pci1())
+		/* Find out what kind of PCI probing is supported otherwise
+		   Justin Gibbs will sulk.. */
+		if (pci_conf1() && probe_for_cmd640_pci1())
 			bus_type = "PCI (type1)";
-		else if (probe_for_cmd640_pci2())
+		else if (pci_conf2() && probe_for_cmd640_pci2())
 			bus_type = "PCI (type2)";
 		else
 			return 0;

[-- Attachment #3: Type: text/plain, Size: 48748 bytes --]

diff -urN linux-2.4.19-p4/drivers/ide/ali14xx.c linux-2.4.19-p4.spin/drivers/ide/ali14xx.c
--- linux-2.4.19-p4/drivers/ide/ali14xx.c	Sun Jul 15 16:22:23 2001
+++ linux-2.4.19-p4.spin/drivers/ide/ali14xx.c	Wed Mar 27 22:08:53 2002
@@ -137,15 +137,14 @@
 
 	/* stuff timing parameters into controller registers */
 	driveNum = (HWIF(drive)->index << 1) + drive->select.b.unit;
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(regOn, basePort);
 	outReg(param1, regTab[driveNum].reg1);
 	outReg(param2, regTab[driveNum].reg2);
 	outReg(param3, regTab[driveNum].reg3);
 	outReg(param4, regTab[driveNum].reg4);
 	outb_p(regOff, basePort);
-	restore_flags(flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 /*
@@ -157,8 +156,8 @@
 	byte t;
 	unsigned long flags;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_disable();	/* local CPU only */
 	for (i = 0; i < ALI_NUM_PORTS; ++i) {
 		basePort = ports[i];
 		regOff = inb(basePort);
@@ -169,7 +168,7 @@
 				dataPort = basePort + 8;
 				t = inReg(0) & 0xf0;
 				outb_p(regOff, basePort);
-				__restore_flags(flags);	/* local CPU only */
+				local_irq_restore(flags);	/* local CPU only */
 				if (t != 0x50)
 					return 0;
 				return 1;  /* success */
@@ -177,7 +176,7 @@
 		}
 		outb_p(regOff, basePort);
 	}
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	return 0;
 }
 
@@ -189,15 +188,15 @@
 	byte t;
 	unsigned long flags;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
 	outb_p(regOn, basePort);
 	for (p = initData; p->reg != 0; ++p)
 		outReg(p->data, p->reg);
 	outb_p(0x01, regPort);
 	t = inb(regPort) & 0x01;
 	outb_p(regOff, basePort);
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	return t;
 }
 
diff -urN linux-2.4.19-p4/drivers/ide/alim15x3.c linux-2.4.19-p4.spin/drivers/ide/alim15x3.c
--- linux-2.4.19-p4/drivers/ide/alim15x3.c	Fri Mar 29 14:47:09 2002
+++ linux-2.4.19-p4.spin/drivers/ide/alim15x3.c	Fri Mar 29 14:48:36 2002
@@ -272,9 +272,9 @@
 		if (r_clc >= 16)
 			r_clc = 0;
 	}
-	__save_flags(flags);
-	__cli();
-	
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_disable();	/* local CPU only */
+
 	/* 
 	 * PIO mode => ATA FIFO on, ATAPI FIFO off
 	 */
@@ -295,7 +295,7 @@
 	
 	pci_write_config_byte(dev, port, s_clc);
 	pci_write_config_byte(dev, port+drive->select.b.unit+2, (a_clc << 4) | r_clc);
-	__restore_flags(flags);
+	local_irq_restore(flags);	/* local CPU only */
 
 	/*
 	 * setup   active  rec
@@ -441,7 +441,7 @@
 	ide_hwif_t *hwif		= HWIF(drive);
 	ide_dma_action_t dma_func	= ide_dma_on;
 	byte can_ultra_dma		= ali15x3_can_ultra(drive);
-	
+
 	if ((m5229_revision<=0x20) && (drive->media!=ide_disk))
 		return hwif->dmaproc(ide_dma_off_quietly, drive);
 
@@ -507,17 +507,10 @@
 }
 #endif /* CONFIG_BLK_DEV_IDEDMA */
 
-#define ALI_INIT_CODE_TEST
-
 unsigned int __init pci_init_ali15x3 (struct pci_dev *dev, const char *name)
 {
 	unsigned long fixdma_base = pci_resource_start(dev, 4);
 
-#ifdef ALI_INIT_CODE_TEST
-	unsigned long flags;
-	byte tmpbyte;
-#endif /* ALI_INIT_CODE_TEST */
-
 	pci_read_config_byte(dev, PCI_REVISION_ID, &m5229_revision);
 
 	isa_dev = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
@@ -544,56 +537,6 @@
 	}
 #endif  /* defined(DISPLAY_ALI_TIMINGS) && defined(CONFIG_PROC_FS) */
 
-#ifdef ALI_INIT_CODE_TEST
-	__save_flags(flags);
-	__cli();
-
-	if (m5229_revision >= 0xC2) {
-		/*
-		 * 1543C-B?, 1535, 1535D, 1553
-		 * Note 1: not all "motherboard" support this detection
-		 * Note 2: if no udma 66 device, the detection may "error".
-		 *         but in this case, we will not set the device to
-		 *         ultra 66, the detection result is not important
-		 */
-
-		/*
-		 * enable "Cable Detection", m5229, 0x4b, bit3
-		 */
-		pci_read_config_byte(dev, 0x4b, &tmpbyte);
-		pci_write_config_byte(dev, 0x4b, tmpbyte | 0x08);
-
-		/*
-		 * set south-bridge's enable bit, m1533, 0x79
-		 */
-		pci_read_config_byte(isa_dev, 0x79, &tmpbyte);
-		if (m5229_revision == 0xC2) {
-			/*
-			 * 1543C-B0 (m1533, 0x79, bit 2)
-			 */
-			pci_write_config_byte(isa_dev, 0x79, tmpbyte | 0x04);
-		} else if (m5229_revision >= 0xC3) {
-			/*
-			 * 1553/1535 (m1533, 0x79, bit 1)
-			 */
-			pci_write_config_byte(isa_dev, 0x79, tmpbyte | 0x02);
-		}
-	} else {
-		/*
-		 * revision 0x20 (1543-E, 1543-F)
-		 * revision 0xC0, 0xC1 (1543C-C, 1543C-D, 1543C-E)
-		 * clear CD-ROM DMA write bit, m5229, 0x4b, bit 7
-		 */
-		pci_read_config_byte(dev, 0x4b, &tmpbyte);
-		/*
-		 * clear bit 7
-		 */
-		pci_write_config_byte(dev, 0x4b, tmpbyte & 0x7F);
-	}
-
-	__restore_flags(flags);
-#endif /* ALI_INIT_CODE_TEST */
-
 	return 0;
 }
 
@@ -611,11 +554,10 @@
 	unsigned long flags;
 	byte tmpbyte;
 
-	__save_flags(flags);
-	__cli();
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_disable();	/* local CPU only */
 
 	if (m5229_revision >= 0xC2) {
-#ifndef ALI_INIT_CODE_TEST
 		/*
 		 * 1543C-B?, 1535, 1535D, 1553
 		 * Note 1: not all "motherboard" support this detection
@@ -645,7 +587,6 @@
 			 */
 			pci_write_config_byte(isa_dev, 0x79, tmpbyte | 0x02);
 		}
-#endif /* ALI_INIT_CODE_TEST */
 		/*
 		 * Ultra66 cable detection (from Host View)
 		 * m5229, 0x4a, bit0: primary, bit1: secondary 80 pin
@@ -666,7 +607,6 @@
 		 */
 		ata66 = (hwif->channel)?cable_80_pin[1]:cable_80_pin[0];
 	} else {
-#ifndef ALI_INIT_CODE_TEST
 		/*
 		 * revision 0x20 (1543-E, 1543-F)
 		 * revision 0xC0, 0xC1 (1543C-C, 1543C-D, 1543C-E)
@@ -677,7 +617,6 @@
 		 * clear bit 7
 		 */
 		pci_write_config_byte(dev, 0x4b, tmpbyte & 0x7F);
-#endif /* ALI_INIT_CODE_TEST */
 		/*
 		 * check m1533, 0x5e, bit 1~4 == 1001 => & 00011110 = 00010010
 		 */
@@ -696,7 +635,7 @@
 
 	pci_write_config_byte(dev, 0x53, tmpbyte);
 
-	__restore_flags(flags);
+	local_irq_restore(flags);	/* local CPU only */
 
 	return(ata66);
 }
diff -urN linux-2.4.19-p4/drivers/ide/cmd640.c linux-2.4.19-p4.spin/drivers/ide/cmd640.c
--- linux-2.4.19-p4/drivers/ide/cmd640.c	Fri Feb 16 16:02:36 2001
+++ linux-2.4.19-p4.spin/drivers/ide/cmd640.c	Wed Mar 27 22:06:14 2002
@@ -217,11 +217,10 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
 	outb_p(val, (reg & 3) | 0xcfc);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 static byte get_cmd640_reg_pci1 (unsigned short reg)
@@ -229,11 +228,10 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
 	b = inb_p((reg & 3) | 0xcfc);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return b;
 }
 
@@ -243,12 +241,11 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(0x10, 0xcf8);
 	outb_p(val, cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 static byte get_cmd640_reg_pci2 (unsigned short reg)
@@ -256,12 +253,11 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(0x10, 0xcf8);
 	b = inb_p(cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return b;
 }
 
@@ -271,11 +267,10 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(reg, cmd640_key);
 	outb_p(val, cmd640_key + 4);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 static byte get_cmd640_reg_vlb (unsigned short reg)
@@ -283,11 +278,10 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(reg, cmd640_key);
 	b = inb_p(cmd640_key + 4);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return b;
 }
 
@@ -364,20 +358,18 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
-
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb_p(0x0a, 0x170 + IDE_SELECT_OFFSET);	/* select drive0 */
 	udelay(100);
 	if ((inb_p(0x170 + IDE_SELECT_OFFSET) & 0x1f) != 0x0a) {
 		outb_p(0x1a, 0x170 + IDE_SELECT_OFFSET); /* select drive1 */
 		udelay(100);
 		if ((inb_p(0x170 + IDE_SELECT_OFFSET) & 0x1f) != 0x1a) {
-			restore_flags(flags);
+			spin_unlock_irqrestore(&io_request_lock, flags);
 			return 0; /* nothing responded */
 		}
 	}
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return 1; /* success */
 }
 
@@ -458,8 +450,7 @@
 	byte b;
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	b = get_cmd640_reg(reg);
 	if (mode) {	/* want prefetch on? */
 #if CMD640_PREFETCH_MASKS
@@ -475,7 +466,7 @@
 		b |= prefetch_masks[index];	/* disable prefetch */
 	}
 	put_cmd640_reg(reg, b);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 /*
@@ -576,8 +567,7 @@
 	/*
 	 * Now that everything is ready, program the new timings
 	 */
-	save_flags (flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	/*
 	 * Program the address_setup clocks into ARTTIM reg,
 	 * and then the active/recovery counts into the DRWTIM reg
@@ -586,7 +576,7 @@
 	setup_count |= get_cmd640_reg(arttim_regs[index]) & 0x3f;
 	put_cmd640_reg(arttim_regs[index], setup_count);
 	put_cmd640_reg(drwtim_regs[index], pack_nibbles(active_count, recovery_count));
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 /*
diff -urN linux-2.4.19-p4/drivers/ide/cmd64x.c linux-2.4.19-p4.spin/drivers/ide/cmd64x.c
--- linux-2.4.19-p4/drivers/ide/cmd64x.c	Sat Mar 30 23:46:00 2002
+++ linux-2.4.19-p4.spin/drivers/ide/cmd64x.c	Sat Mar 30 23:44:07 2002
@@ -298,8 +298,9 @@
 	/*
 	 * Now that everything is ready, program the new timings
 	 */
-	__save_flags (flags);
-	__cli();
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
+
 	/*
 	 * Program the address_setup clocks into ARTTIM reg,
 	 * and then the active/recovery counts into the DRWTIM reg
@@ -311,7 +312,7 @@
 		(byte) ((active_count << 4) | recovery_count));
 	cmdprintk ("Write %x to %x\n", ((byte) setup_count) | (temp_b & 0x3f), arttim_regs[channel][slave]);
 	cmdprintk ("Write %x to %x\n", (byte) ((active_count << 4) | recovery_count), drwtim_regs[channel][slave]);
-	__restore_flags(flags);
+	local_irq_restore(flags);	/* local CPU only */
 }
 
 /*
diff -urN linux-2.4.19-p4/drivers/ide/cs5530.c linux-2.4.19-p4.spin/drivers/ide/cs5530.c
--- linux-2.4.19-p4/drivers/ide/cs5530.c	Tue Jan  2 16:58:45 2001
+++ linux-2.4.19-p4.spin/drivers/ide/cs5530.c	Wed Mar 27 22:10:12 2002
@@ -286,9 +286,8 @@
 		return 0;
 	}
 
-	save_flags(flags);
-	cli();	/* all CPUs (there should only be one CPU with this chipset) */
-
+	/* all CPUs (there should only be one CPU with this chipset) */
+	spin_lock_irqsave(&io_request_lock, flags);
 	/*
 	 * Enable BusMaster and MemoryWriteAndInvalidate for the cs5530:
 	 * -->  OR 0x14 into 16-bit PCI COMMAND reg of function 0 of the cs5530
@@ -333,7 +332,7 @@
 	pci_write_config_byte(master_0, 0x42, 0x00);
 	pci_write_config_byte(master_0, 0x43, 0xc1);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 
 	return 0;
 }
diff -urN linux-2.4.19-p4/drivers/ide/dtc2278.c linux-2.4.19-p4.spin/drivers/ide/dtc2278.c
--- linux-2.4.19-p4/drivers/ide/dtc2278.c	Thu Apr 13 22:54:26 2000
+++ linux-2.4.19-p4.spin/drivers/ide/dtc2278.c	Wed Mar 27 21:47:59 2002
@@ -77,14 +77,13 @@
 	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
 
 	if (pio >= 3) {
-		save_flags(flags);	/* all CPUs */
-		cli();			/* all CPUs */
+		spin_lock_irqsave(&io_request_lock, flags);
 		/*
 		 * This enables PIO mode4 (3?) on the first interface
 		 */
 		sub22(1,0xc3);
 		sub22(0,0xa0);
-		restore_flags(flags);	/* all CPUs */
+		spin_unlock_irqrestore(&io_request_lock, flags);
 	} else {
 		/* we don't know how to set it back again.. */
 	}
@@ -100,8 +99,8 @@
 {
 	unsigned long flags;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
 	/*
 	 * This enables the second interface
 	 */
@@ -117,7 +116,7 @@
 	sub22(1,0xc3);
 	sub22(0,0xa0);
 #endif
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 
 	ide_hwifs[0].serialized = 1;
 	ide_hwifs[1].serialized = 1;
diff -urN linux-2.4.19-p4/drivers/ide/hpt34x.c linux-2.4.19-p4.spin/drivers/ide/hpt34x.c
--- linux-2.4.19-p4/drivers/ide/hpt34x.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/hpt34x.c	Wed Mar 27 19:52:27 2002
@@ -385,8 +385,8 @@
 	unsigned short cmd;
 	unsigned long flags;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
 
 	pci_write_config_byte(dev, HPT34X_PCI_INIT_REG, 0x00);
 	pci_read_config_word(dev, PCI_COMMAND, &cmd);
@@ -417,7 +417,7 @@
 	pci_write_config_dword(dev, PCI_BASE_ADDRESS_3, dev->resource[3].start);
 	pci_write_config_word(dev, PCI_COMMAND, cmd);
 
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 
 #if defined(DISPLAY_HPT34X_TIMINGS) && defined(CONFIG_PROC_FS)
 	if (!hpt34x_proc) {
diff -urN linux-2.4.19-p4/drivers/ide/hpt366.c linux-2.4.19-p4.spin/drivers/ide/hpt366.c
--- linux-2.4.19-p4/drivers/ide/hpt366.c	Sat Mar 30 21:02:14 2002
+++ linux-2.4.19-p4.spin/drivers/ide/hpt366.c	Sat Mar 30 21:39:04 2002
@@ -1524,8 +1524,8 @@
 	byte secondary	= hwif->channel ? 0x4f : 0x47;
 	unsigned long flags;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
 
 	dma_new = dma_old;
 	pci_read_config_byte(hwif->pci_dev, primary, &masterdma);
@@ -1535,7 +1535,7 @@
 	if (slavedma & 0x30)	dma_new |= 0x40;
 	if (dma_new != dma_old) outb(dma_new, dmabase+2);
 
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 
 	ide_setup_dma(hwif, dmabase, 8);
 }
diff -urN linux-2.4.19-p4/drivers/ide/ht6560b.c linux-2.4.19-p4.spin/drivers/ide/ht6560b.c
--- linux-2.4.19-p4/drivers/ide/ht6560b.c	Thu Apr 13 22:54:26 2000
+++ linux-2.4.19-p4.spin/drivers/ide/ht6560b.c	Wed Mar 27 21:52:21 2002
@@ -134,9 +134,9 @@
 	static byte current_timing = 0;
 	byte select, timing;
 	
-	__save_flags (flags);	/* local CPU only */
-	__cli();		/* local CPU only */
-	
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */	
+
 	select = HT_CONFIG(drive);
 	timing = HT_TIMING(drive);
 	
@@ -159,7 +159,7 @@
 		printk("ht6560b: %s: select=%#x timing=%#x\n", drive->name, select, timing);
 #endif
 	}
-	__restore_flags (flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 }
 
 /*
@@ -257,9 +257,8 @@
 	unsigned long flags;
 	int t = HT_PREFETCH_MODE << 8;
 	
-	save_flags (flags);	/* all CPUs */
-	cli();		        /* all CPUs */
-	
+	spin_lock_irqsave(&io_request_lock, flags);
+
 	/*
 	 *  Prefetch mode and unmask irq seems to conflict
 	 */
@@ -272,7 +271,7 @@
 		drive->no_unmask = 0;
 	}
 	
-	restore_flags (flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	
 #ifdef DEBUG
 	printk("ht6560b: drive %s prefetch mode %sabled\n", drive->name, (state ? "en" : "dis"));
@@ -293,13 +292,10 @@
 	
 	timing = ht_pio2timings(drive, pio);
 	
-	save_flags (flags);	/* all CPUs */
-	cli();		        /* all CPUs */
-	
+	spin_lock_irqsave(&io_request_lock, flags);	
 	drive->drive_data &= 0xff00;
 	drive->drive_data |= timing;
-	
-	restore_flags (flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	
 #ifdef DEBUG
 	printk("ht6560b: drive %s tuned to pio mode %#x timing=%#x\n", drive->name, pio, timing);
diff -urN linux-2.4.19-p4/drivers/ide/ide-disk.c linux-2.4.19-p4.spin/drivers/ide/ide-disk.c
--- linux-2.4.19-p4/drivers/ide/ide-disk.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-disk.c	Wed Mar 27 19:57:43 2002
@@ -646,7 +646,7 @@
 			return startstop;
 		}
 		if (!drive->unmask)
-			__cli();	/* local CPU only */
+			local_irq_disable();	/* local CPU only */
 		if (drive->mult_count) {
 			ide_hwgroup_t *hwgroup = HWGROUP(drive);
 	/*
diff -urN linux-2.4.19-p4/drivers/ide/ide-features.c linux-2.4.19-p4.spin/drivers/ide/ide-features.c
--- linux-2.4.19-p4/drivers/ide/ide-features.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-features.c	Wed Mar 27 21:37:24 2002
@@ -169,18 +169,18 @@
 		printk("%s: CHECK for good STATUS\n", drive->name);
 		return 0;
 	}
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only; some systems need this */
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
 	SELECT_MASK(HWIF(drive), drive, 0);
 	id = kmalloc(SECTOR_WORDS*4, GFP_ATOMIC);
 	if (!id) {
-		__restore_flags(flags);	/* local CPU only */
+		local_irq_restore(flags);	/* local CPU only */
 		return 0;
 	}
 	ata_input_data(drive, id, SECTOR_WORDS);
-	(void) GET_STAT();	/* clear drive IRQ */
-	ide__sti();		/* local CPU only */
-	__restore_flags(flags);	/* local CPU only */
+	(void) GET_STAT();		/* clear drive IRQ */
+	local_irq_enable();		/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	ide_fix_driveid(id);
 	if (id) {
 		drive->id->dma_ultra = id->dma_ultra;
@@ -301,14 +301,14 @@
 	 */
 	if ((stat = GET_STAT()) & BUSY_STAT) {
 		unsigned long flags, timeout;
-		__save_flags(flags);	/* local CPU only */
-		ide__sti();		/* local CPU only -- for jiffies */
+		local_irq_save(flags);	/* local CPU only */
+		local_irq_enable();	/* local CPU only -- for jiffies */
 		timeout = jiffies + WAIT_CMD;
 		while ((stat = GET_STAT()) & BUSY_STAT) {
 			if (time_after(jiffies, timeout))
 				break;
 		}
-		__restore_flags(flags); /* local CPU only */
+		local_irq_restore(flags);	/* local CPU only */
 	}
 
 	/*
diff -urN linux-2.4.19-p4/drivers/ide/ide-floppy.c linux-2.4.19-p4.spin/drivers/ide/ide-floppy.c
--- linux-2.4.19-p4/drivers/ide/ide-floppy.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-floppy.c	Wed Mar 27 21:37:49 2002
@@ -939,7 +939,7 @@
 #endif /* IDEFLOPPY_DEBUG_LOG */
 		clear_bit (PC_DMA_IN_PROGRESS, &pc->flags);
 
-		ide__sti();	/* local CPU only */
+		local_irq_enable();	/* local CPU only */
 
 		if (status.b.check || test_bit (PC_DMA_ERROR, &pc->flags)) {	/* Error detected */
 #if IDEFLOPPY_DEBUG_LOG
@@ -1669,10 +1669,10 @@
 		idefloppy_status_reg_t status;
 		unsigned long flags;
 
-		__save_flags(flags);
-		__cli();
+		local_irq_save(flags);		/* local CPU only */
+		local_irq_disable();		/* local CPU only */
 		status.all=GET_STAT();
-		__restore_flags(flags);
+		local_irq_restore(flags);	/* local CPU only */
 
 		progress_indication= !status.b.dsc ? 0:0x10000;
 	}
diff -urN linux-2.4.19-p4/drivers/ide/ide-pmac.c linux-2.4.19-p4.spin/drivers/ide/ide-pmac.c
--- linux-2.4.19-p4/drivers/ide/ide-pmac.c	Fri Mar 22 00:16:36 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-pmac.c	Wed Mar 27 21:32:48 2002
@@ -385,10 +385,10 @@
 	OUT_BYTE(SETFEATURES_XFER, IDE_FEATURE_REG);
 	OUT_BYTE(WIN_SETFEATURES, IDE_COMMAND_REG);
 	udelay(1);
-	__save_flags(flags);	/* local CPU only */
-	ide__sti();		/* local CPU only -- for jiffies */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_enable();	/* local CPU only -- for jiffies */
 	result = wait_for_ready(drive, 2000);	/* Timeout bumped for some powerbooks */
-	__restore_flags(flags); /* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	OUT_BYTE(drive->ctl, IDE_CONTROL_REG);
 	if (result)
 		printk(KERN_ERR "pmac_ide_do_setfeature disk not ready after SET_FEATURE !\n");
diff -urN linux-2.4.19-p4/drivers/ide/ide-probe.c linux-2.4.19-p4.spin/drivers/ide/ide-probe.c
--- linux-2.4.19-p4/drivers/ide/ide-probe.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-probe.c	Wed Mar 27 21:40:53 2002
@@ -74,7 +74,7 @@
                         *ptr++ = inl(IDE_DATA_REG);
         }
 #endif
-	ide__sti();	/* local CPU only */
+	local_irq_enable();	/* local CPU only */
 	ide_fix_driveid(id);
 
 	if (id->word156 == 0x4d42) {
@@ -254,12 +254,13 @@
 	ide_delay_50ms();		/* wait for IRQ and DRQ_STAT */
 	if (OK_STAT(GET_STAT(),DRQ_STAT,BAD_R_STAT)) {
 		unsigned long flags;
-		__save_flags(flags);	/* local CPU only */
-		__cli();		/* local CPU only; some systems need this */
+		local_irq_save(flags);		/* local CPU only */
+		local_irq_disable();		/* local CPU only */
+			/* local CPU only; some systems need this */
 		do_identify(drive, cmd); /* drive returned ID */
 		rc = 0;			/* drive responded with ID */
 		(void) GET_STAT();	/* clear drive IRQ */
-		__restore_flags(flags);	/* local CPU only */
+		local_irq_restore(flags);	/* local CPU only */
 	} else
 		rc = 2;			/* drive refused ID */
 	return rc;
@@ -540,8 +541,10 @@
 		return;	
 	}
 
-	__save_flags(flags);	/* local CPU only */
-	__sti();		/* local CPU only; needed for jiffies and irq probing */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_enable();	/* local CPU only */
+		/* needed for jiffies and irq probing */
+
 	/*
 	 * Second drive should only exist if first drive was found,
 	 * but a lot of cdrom drives are configured as single slaves.
@@ -570,7 +573,7 @@
 		} while ((stat & BUSY_STAT) && time_after(timeout, jiffies));
 
 	}
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
 		ide_drive_t *drive = &hwif->drives[unit];
 		if (drive->present) {
@@ -648,10 +651,8 @@
 	/* Allocate the buffer and potentially sleep first */
 	
 	new_hwgroup = kmalloc(sizeof(ide_hwgroup_t),GFP_KERNEL);
-	
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
 
+	spin_lock_irqsave(&io_request_lock, flags);
 	hwif->hwgroup = NULL;
 #if MAX_HWIFS > 1
 	/*
@@ -687,7 +688,7 @@
 	} else {
 		hwgroup = new_hwgroup;
 		if (!hwgroup) {
-			restore_flags(flags);	/* all CPUs */
+			spin_unlock_irqrestore(&io_request_lock, flags);
 			return 1;
 		}
 		memset(hwgroup, 0, sizeof(ide_hwgroup_t));
@@ -717,7 +718,7 @@
 		if (ide_request_irq(hwif->irq, &ide_intr, sa, hwif->name, hwgroup)) {
 			if (!match)
 				kfree(hwgroup);
-			restore_flags(flags);	/* all CPUs */
+			spin_unlock_irqrestore(&io_request_lock, flags);
 			return 1;
 		}
 	}
@@ -745,7 +746,8 @@
 		printk("%s : Adding missed hwif to hwgroup!!\n", hwif->name);
 #endif
 	}
-	restore_flags(flags);	/* all CPUs; safe now that hwif->hwgroup is set up */
+	spin_unlock_irqrestore(&io_request_lock, flags);
+		/* all CPUs; safe now that hwif->hwgroup is set up */
 
 #if !defined(__mc68000__) && !defined(CONFIG_APUS) && !defined(__sparc__)
 	printk("%s at 0x%03x-0x%03x,0x%03x on irq %d", hwif->name,
diff -urN linux-2.4.19-p4/drivers/ide/ide-proc.c linux-2.4.19-p4.spin/drivers/ide/ide-proc.c
--- linux-2.4.19-p4/drivers/ide/ide-proc.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-proc.c	Wed Mar 27 21:58:26 2002
@@ -597,7 +597,7 @@
 	if (!driver)
 		len = sprintf(page, "(none)\n");
         else
-		len = sprintf(page,"%llu\n", (__u64) ((ide_driver_t *)drive->driver)->capacity(drive));
+		len = sprintf(page,"%lu\n", (u64) ((ide_driver_t *)drive->driver)->capacity(drive));
 	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
 }
 
diff -urN linux-2.4.19-p4/drivers/ide/ide-tape.c linux-2.4.19-p4.spin/drivers/ide/ide-tape.c
--- linux-2.4.19-p4/drivers/ide/ide-tape.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-tape.c	Wed Mar 27 22:01:09 2002
@@ -2090,7 +2090,7 @@
 #endif /* IDETAPE_DEBUG_LOG */
 		clear_bit (PC_DMA_IN_PROGRESS, &pc->flags);
 
-		ide__sti();	/* local CPU only */
+		local_irq_enable();	/* local CPU only */
 
 #if SIMULATE_ERRORS
 		if ((pc->c[0] == IDETAPE_WRITE_CMD || pc->c[0] == IDETAPE_READ_CMD) && (++error_sim_count % 100) == 0) {
@@ -6085,14 +6085,15 @@
 	int minor = tape->minor;
 	unsigned long flags;
 
-	save_flags (flags);	/* all CPUs (overkill?) */
-	cli();			/* all CPUs (overkill?) */
-	if (test_bit (IDETAPE_BUSY, &tape->flags) || tape->first_stage != NULL || tape->merge_stage_size || drive->usage) {
-		restore_flags(flags);	/* all CPUs (overkill?) */
+	spin_lock_irqsave(&io_request_lock, flags);
+	if (test_bit (IDETAPE_BUSY, &tape->flags) ||
+	    tape->first_stage != NULL ||
+	    tape->merge_stage_size || drive->usage) {
+		spin_unlock_irqrestore(&io_request_lock, flags);
 		return 1;
 	}
 	idetape_chrdevs[minor].drive = NULL;
-	restore_flags (flags);	/* all CPUs (overkill?) */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	DRIVER(drive)->busy = 0;
 	(void) ide_unregister_subdriver (drive);
 	drive->driver_data = NULL;
diff -urN linux-2.4.19-p4/drivers/ide/ide-taskfile.c linux-2.4.19-p4.spin/drivers/ide/ide-taskfile.c
--- linux-2.4.19-p4/drivers/ide/ide-taskfile.c	Thu Mar 21 07:05:05 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide-taskfile.c	Wed Mar 27 21:39:54 2002
@@ -127,11 +127,11 @@
 #if SUPPORT_VLB_SYNC
 		if (io_32bit & 2) {
 			unsigned long flags;
-			__save_flags(flags);	/* local CPU only */
-			__cli();		/* local CPU only */
+			local_irq_save(flags);		/* local CPU only */
+			local_irq_disable();		/* local CPU only */
 			task_vlb_sync(IDE_NSECTOR_REG);
 			insl(IDE_DATA_REG, buffer, wcount);
-			__restore_flags(flags);	/* local CPU only */
+			local_irq_restore(flags);	/* local CPU only */
 		} else
 #endif /* SUPPORT_VLB_SYNC */
 			insl(IDE_DATA_REG, buffer, wcount);
@@ -167,11 +167,11 @@
 #if SUPPORT_VLB_SYNC
 		if (io_32bit & 2) {
 			unsigned long flags;
-			__save_flags(flags);	/* local CPU only */
-			__cli();		/* local CPU only */
+			local_irq_save(flags);		/* local CPU only */
+			local_irq_disable();		/* local CPU only */
 			task_vlb_sync(IDE_NSECTOR_REG);
 			outsl(IDE_DATA_REG, buffer, wcount);
-			__restore_flags(flags);	/* local CPU only */
+			local_irq_restore(flags);	/* local CPU only */
 		} else
 #endif /* SUPPORT_VLB_SYNC */
 			outsl(IDE_DATA_REG, buffer, wcount);
@@ -491,8 +491,8 @@
 	unsigned long flags;
 	byte err = 0;
 
-	__save_flags (flags);	/* local CPU only */
-	ide__sti();		/* local CPU only */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_enable();	/* local CPU only */
 	printk("%s: %s: status=0x%02x", drive->name, msg, stat);
 #if FANCY_STATUS_DUMPS
 	printk(" { ");
@@ -557,7 +557,7 @@
 #endif  /* FANCY_STATUS_DUMPS */
 		printk("\n");
 	}
-	__restore_flags (flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	return err;
 }
 
@@ -742,8 +742,7 @@
 	ide_task_t *args	= HWGROUP(drive)->rq->special;
 	byte stat		= GET_STAT();
 
-	ide__sti();	/* local CPU only */
-
+	local_irq_enable();	/* local CPU only */
 	if (!OK_STAT(stat, READY_STAT, BAD_STAT))
 		return ide_error(drive, "task_no_data_intr", stat);
 		/* calls ide_end_drive_cmd */
diff -urN linux-2.4.19-p4/drivers/ide/ide.c linux-2.4.19-p4.spin/drivers/ide/ide.c
--- linux-2.4.19-p4/drivers/ide/ide.c	Thu Mar 21 07:05:06 2002
+++ linux-2.4.19-p4.spin/drivers/ide/ide.c	Wed Mar 27 21:23:06 2002
@@ -217,13 +217,13 @@
 	unsigned long t, flags;
 	int i;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
 	t = jiffies * 11932;
     	outb_p(0, 0x43);
 	i = inb_p(0x40);
 	i |= inb(0x40) << 8;
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	return (t - i);
 }
 #endif /* DISK_RECOVERY_TIME */
@@ -605,8 +605,8 @@
 	ide_hwif_t *hwif = HWIF(drive);
 	ide_hwgroup_t *hwgroup = HWGROUP(drive);
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);		/* local CPU only */
+	local_irq_disable();		/* local CPU only */
 
 	/* For an ATAPI device, first try an ATAPI SRST. */
 	if (drive->media != ide_disk && !do_not_try_atapi) {
@@ -616,7 +616,7 @@
 		OUT_BYTE (WIN_SRST, IDE_COMMAND_REG);
 		hwgroup->poll_timeout = jiffies + WAIT_WORSTCASE;
 		ide_set_handler (drive, &atapi_reset_pollfunc, HZ/20, NULL);
-		__restore_flags (flags);	/* local CPU only */
+		local_irq_restore(flags);	/* local CPU only */
 		return ide_started;
 	}
 
@@ -629,7 +629,7 @@
 
 #if OK_TO_RESET_CONTROLLER
 	if (!IDE_CONTROL_REG) {
-		__restore_flags(flags);
+		local_irq_restore(flags);	/* local CPU only */
 		return ide_stopped;
 	}
 	/*
@@ -661,7 +661,7 @@
 
 #endif	/* OK_TO_RESET_CONTROLLER */
 
-	__restore_flags (flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	return ide_started;
 }
 
@@ -768,8 +768,8 @@
 	unsigned long flags;
 	byte err = 0;
 
-	__save_flags (flags);	/* local CPU only */
-	ide__sti();		/* local CPU only */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_enable();	/* local CPU only */
 	printk("%s: %s: status=0x%02x", drive->name, msg, stat);
 #if FANCY_STATUS_DUMPS
 	printk(" { ");
@@ -835,7 +835,7 @@
 #endif	/* FANCY_STATUS_DUMPS */
 		printk("\n");
 	}
-	__restore_flags (flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 	return err;
 }
 
@@ -946,7 +946,7 @@
 	byte stat = GET_STAT();
 	int retries = 10;
 
-	ide__sti();	/* local CPU only */
+	local_irq_enable();	/* local CPU only */
 	if ((stat & DRQ_STAT) && args && args[3]) {
 		byte io_32bit = drive->io_32bit;
 		drive->io_32bit = 0;
@@ -1011,17 +1011,17 @@
 
 	udelay(1);	/* spec allows drive 400ns to assert "BUSY" */
 	if ((stat = GET_STAT()) & BUSY_STAT) {
-		__save_flags(flags);	/* local CPU only */
-		ide__sti();		/* local CPU only */
+		local_irq_save(flags);	/* local CPU only */
+		local_irq_enable();	/* local CPU only */
 		timeout += jiffies;
 		while ((stat = GET_STAT()) & BUSY_STAT) {
 			if (time_after(jiffies, timeout)) {
-				__restore_flags(flags);	/* local CPU only */
+				local_irq_restore(flags);	/* local CPU only */
 				*startstop = ide_error(drive, "status timeout", stat);
 				return 1;
 			}
 		}
-		__restore_flags(flags);	/* local CPU only */
+		local_irq_restore(flags);	/* local CPU only */
 	}
 	/*
 	 * Allow status to settle, then read it again.
@@ -1344,7 +1344,8 @@
 
 	ide_get_lock(&ide_lock, ide_intr, hwgroup);	/* for atari only: POSSIBLY BROKEN HERE(?) */
 
-	__cli();	/* necessary paranoia: ensure IRQs are masked on local CPU */
+	local_irq_disable();	/* local CPU only */
+		/* necessary paranoia: ensure IRQs are masked on local CPU */
 
 	while (!hwgroup->busy) {
 		hwgroup->busy = 1;
@@ -1405,7 +1406,8 @@
 		if (masked_irq && hwif->irq != masked_irq)
 			disable_irq_nosync(hwif->irq);
 		spin_unlock(&io_request_lock);
-		ide__sti();	/* allow other IRQs while we start this request */
+		local_irq_enable();	/* local CPU only */
+			/* allow other IRQs while we start this request */
 		startstop = start_request(drive, rq);
 		spin_lock_irq(&io_request_lock);
 		if (masked_irq && hwif->irq != masked_irq)
@@ -1540,7 +1542,8 @@
 #else
 			disable_irq(hwif->irq);	/* disable_irq_nosync ?? */
 #endif /* DISABLE_IRQ_NOSYNC */
-			__cli();	/* local CPU only, as if we were handling an interrupt */
+			local_irq_disable();	/* local CPU only */
+				/* as if we were handling an interrupt */
 			if (hwgroup->poll_timeout != 0) {
 				startstop = handler(drive);
 			} else if (drive_is_ready(drive)) {
@@ -1642,7 +1645,8 @@
 		return;
 	}
 
-	if ((handler = hwgroup->handler) == NULL || hwgroup->poll_timeout != 0) {
+	if ((handler = hwgroup->handler) == NULL ||
+	     hwgroup->poll_timeout != 0) {
 		/*
 		 * Not expecting an interrupt from this drive.
 		 * That means this could be:
@@ -1667,7 +1671,8 @@
 #ifdef CONFIG_BLK_DEV_IDEPCI
 		} else {
 			/*
-			 * Whack the status register, just in case we have a leftover pending IRQ.
+			 * Whack the status register,
+			 * just in case we have a leftover pending IRQ.
 			 */
 			(void) IN_BYTE(hwif->io_ports[IDE_STATUS_OFFSET]);
 #endif /* CONFIG_BLK_DEV_IDEPCI */
@@ -1678,16 +1683,18 @@
 	drive = hwgroup->drive;
 	if (!drive) {
 		/*
-		 * This should NEVER happen, and there isn't much we could do about it here.
+		 * This should NEVER happen,
+		 * and there isn't much we could do about it here.
 		 */
 		spin_unlock_irqrestore(&io_request_lock, flags);
 		return;
 	}
 	if (!drive_is_ready(drive)) {
 		/*
-		 * This happens regularly when we share a PCI IRQ with another device.
-		 * Unfortunately, it can also happen with some buggy drives that trigger
-		 * the IRQ before their status register is up to date.  Hopefully we have
+		 * This happens regularly when we share a PCI IRQ with
+		 * another device.  Unfortunately, it can also happen
+		 * with some buggy drives that trigger the IRQ before
+		 * their status register is up to date.  Hopefully we have
 		 * enough advance overhead that the latter isn't a problem.
 		 */
 		spin_unlock_irqrestore(&io_request_lock, flags);
@@ -1702,8 +1709,8 @@
 	spin_unlock(&io_request_lock);
 
 	if (drive->unmask)
-		ide__sti();	/* local CPU only */
-	startstop = handler(drive);		/* service this interrupt, may set handler for next interrupt */
+		local_irq_enable();	/* local CPU only */
+	startstop = handler(drive);	/* service this interrupt, may set handler for next interrupt */
 	spin_lock_irq(&io_request_lock);
 
 	/*
@@ -2046,8 +2053,7 @@
 
 	if (index >= MAX_HWIFS)
 		return;
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
+	spin_lock_irqsave(&io_request_lock, flags);
 	hwif = &ide_hwifs[index];
 	if (!hwif->present)
 		goto abort;
@@ -2065,7 +2071,7 @@
 	/*
 	 * All clear?  Then blow away the buffer cache
 	 */
-	sti();
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	for (unit = 0; unit < MAX_DRIVES; ++unit) {
 		drive = &hwif->drives[unit];
 		if (!drive->present)
@@ -2081,7 +2087,7 @@
 		destroy_proc_ide_drives(hwif);
 #endif
 	}
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	hwgroup = hwif->hwgroup;
 
 	/*
@@ -2201,7 +2207,7 @@
 	hwif->straight8		= old_hwif.straight8;
 	hwif->hwif_data		= old_hwif.hwif_data;
 abort:
-	restore_flags(flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 /*
@@ -2413,14 +2419,14 @@
 	while (hwgroup->busy) {
 		unsigned long lflags;
 		spin_unlock_irq(&io_request_lock);
-		__save_flags(lflags);	/* local CPU only */
-		__sti();		/* local CPU only; needed for jiffies */
+		local_irq_save(lflags);	/* local CPU only */
+		local_irq_enable();	/* local CPU only; needed for jiffies */
 		if (time_after(jiffies, timeout)) {
-			__restore_flags(lflags);	/* local CPU only */
+			local_irq_restore(lflags);	/* local CPU only */
 			printk("%s: channel busy\n", drive->name);
 			return -EBUSY;
 		}
-		__restore_flags(lflags);	/* local CPU only */
+		local_irq_restore(lflags);	/* local CPU only */
 		spin_lock_irq(&io_request_lock);
 	}
 	return 0;
@@ -3593,16 +3599,17 @@
 int ide_register_subdriver (ide_drive_t *drive, ide_driver_t *driver, int version)
 {
 	unsigned long flags;
-	
-	save_flags(flags);		/* all CPUs */
-	cli();				/* all CPUs */
-	if (version != IDE_SUBDRIVER_VERSION || !drive->present || drive->driver != NULL || drive->busy || drive->usage) {
-		restore_flags(flags);	/* all CPUs */
+
+	spin_lock_irqsave(&io_request_lock, flags);
+	if (version != IDE_SUBDRIVER_VERSION ||
+			!drive->present || drive->driver != NULL ||
+			drive->busy || drive->usage) {
+		spin_unlock_irqrestore(&io_request_lock, flags);
 		return 1;
 	}
 	drive->driver = driver;
 	setup_driver_defaults(drive);
-	restore_flags(flags);		/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	if (drive->autotune != 2) {
 		if (driver->supports_dma && HWIF(drive)->dmaproc != NULL) {
 			/*
@@ -3629,11 +3636,10 @@
 int ide_unregister_subdriver (ide_drive_t *drive)
 {
 	unsigned long flags;
-	
-	save_flags(flags);		/* all CPUs */
-	cli();				/* all CPUs */
+
+	spin_lock_irqsave(&io_request_lock, flags);
 	if (drive->usage || drive->busy || drive->driver == NULL || DRIVER(drive)->busy) {
-		restore_flags(flags);	/* all CPUs */
+		spin_unlock_irqrestore(&io_request_lock, flags);
 		return 1;
 	}
 #if defined(CONFIG_BLK_DEV_ISAPNP) && defined(CONFIG_ISAPNP) && defined(MODULE)
@@ -3645,7 +3651,7 @@
 #endif
 	auto_remove_settings(drive);
 	drive->driver = NULL;
-	restore_flags(flags);		/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return 0;
 }
 
diff -urN linux-2.4.19-p4/drivers/ide/it8172.c linux-2.4.19-p4.spin/drivers/ide/it8172.c
--- linux-2.4.19-p4/drivers/ide/it8172.c	Wed Mar 20 18:26:36 2002
+++ linux-2.4.19-p4.spin/drivers/ide/it8172.c	Wed Mar 27 22:02:01 2002
@@ -95,10 +95,9 @@
 	    drive_enables |= 0x0006;
     }
 
-    save_flags(flags);
-    cli();
+    spin_lock_irqsave(&io_request_lock, flags);
     pci_write_config_word(HWIF(drive)->pci_dev, 0x40, drive_enables);
-    restore_flags(flags);
+    spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 #if defined(CONFIG_BLK_DEV_IDEDMA) && defined(CONFIG_IT8172_TUNING)
diff -urN linux-2.4.19-p4/drivers/ide/ns87415.c linux-2.4.19-p4.spin/drivers/ide/ns87415.c
--- linux-2.4.19-p4/drivers/ide/ns87415.c	Tue Jun 20 07:52:36 2000
+++ linux-2.4.19-p4.spin/drivers/ide/ns87415.c	Wed Mar 27 21:24:03 2002
@@ -38,8 +38,9 @@
 	struct pci_dev *dev = hwif->pci_dev;
 	unsigned long flags;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_disable();	/* local CPU only */
+
 	new = *old;
 
 	/* Adjust IRQ enable bit */
@@ -72,8 +73,7 @@
 		 */
 		udelay(10);
 	}
-
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 }
 
 static void ns87415_selectproc (ide_drive_t *drive)
diff -urN linux-2.4.19-p4/drivers/ide/opti621.c linux-2.4.19-p4.spin/drivers/ide/opti621.c
--- linux-2.4.19-p4/drivers/ide/opti621.c	Tue May  1 16:05:00 2001
+++ linux-2.4.19-p4.spin/drivers/ide/opti621.c	Wed Mar 27 22:07:34 2002
@@ -276,9 +276,7 @@
 		hwif->name, ax, second.data_time, second.recovery_time, drdy);
 #endif
 
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
-
+	spin_lock_irqsave(&io_request_lock, flags);
      	reg_base = hwif->io_ports[IDE_DATA_OFFSET];
 	outb(0xc0, reg_base+CNTRL_REG);	/* allow Register-B */
 	outb(0xff, reg_base+5);		/* hmm, setupvic.exe does this ;-) */
@@ -302,7 +300,7 @@
  	write_reg(misc, MISC_REG);	/* set address setup, DRDY timings,   */
  					/*  and read prefetch for both drives */
 
-	restore_flags(flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 /*
diff -urN linux-2.4.19-p4/drivers/ide/pdc4030.c linux-2.4.19-p4.spin/drivers/ide/pdc4030.c
--- linux-2.4.19-p4/drivers/ide/pdc4030.c	Thu Mar 21 07:05:06 2002
+++ linux-2.4.19-p4.spin/drivers/ide/pdc4030.c	Wed Mar 27 21:25:00 2002
@@ -655,7 +655,7 @@
 			return startstop;
 	    	}
 		if (!drive->unmask)
-			__cli();	/* local CPU only */
+			local_irq_disable();	/* local CPU only */
 		HWGROUP(drive)->wrq = *rq; /* scratchpad */
 		return promise_write(drive);
 	default:
diff -urN linux-2.4.19-p4/drivers/ide/piix.c linux-2.4.19-p4.spin/drivers/ide/piix.c
--- linux-2.4.19-p4/drivers/ide/piix.c	Thu Mar 21 07:05:06 2002
+++ linux-2.4.19-p4.spin/drivers/ide/piix.c	Wed Mar 27 22:02:56 2002
@@ -268,12 +268,11 @@
 		master_data = master_data | (timings[pio][0] << 12) |
 			      (timings[pio][1] << 8);
 	}
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	pci_write_config_word(HWIF(drive)->pci_dev, master_port, master_data);
 	if (is_slave)
 		pci_write_config_byte(HWIF(drive)->pci_dev, slave_port, slave_data);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 #if defined(CONFIG_BLK_DEV_IDEDMA) && defined(CONFIG_PIIX_TUNING)
diff -urN linux-2.4.19-p4/drivers/ide/qd65xx.c linux-2.4.19-p4.spin/drivers/ide/qd65xx.c
--- linux-2.4.19-p4/drivers/ide/qd65xx.c	Wed Mar 20 18:26:36 2002
+++ linux-2.4.19-p4.spin/drivers/ide/qd65xx.c	Wed Mar 27 22:05:02 2002
@@ -94,10 +94,9 @@
 {
 	unsigned long flags;
 
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
+	spin_lock_irqsave(&io_request_lock, flags);
 	outb(content,reg);
-	restore_flags(flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 byte __init qd_read_reg (byte reg)
@@ -105,10 +104,9 @@
 	unsigned long flags;
 	byte read;
 
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
+	spin_lock_irqsave(&io_request_lock, flags);
 	read = inb(reg);
-	restore_flags(flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	return read;
 }
 
@@ -313,13 +311,12 @@
 	byte readreg;
 	unsigned long flags;
 
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
+	spin_lock_irqsave(&io_request_lock, 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(&io_request_lock, flags);
 
 	if (savereg == QD_TESTVAL) {
 		printk(KERN_ERR "Outch ! the probe for qd65xx isn't reliable !\n");
diff -urN linux-2.4.19-p4/drivers/ide/slc90e66.c linux-2.4.19-p4.spin/drivers/ide/slc90e66.c
--- linux-2.4.19-p4/drivers/ide/slc90e66.c	Wed Mar 20 18:26:36 2002
+++ linux-2.4.19-p4.spin/drivers/ide/slc90e66.c	Wed Mar 27 22:05:46 2002
@@ -222,12 +222,11 @@
 		master_data = master_data | (timings[pio][0] << 12) |
 			      (timings[pio][1] << 8);
 	}
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&io_request_lock, flags);
 	pci_write_config_word(HWIF(drive)->pci_dev, master_port, master_data);
 	if (is_slave)
 		pci_write_config_byte(HWIF(drive)->pci_dev, slave_port, slave_data);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 #ifdef CONFIG_BLK_DEV_IDEDMA
diff -urN linux-2.4.19-p4/drivers/ide/trm290.c linux-2.4.19-p4.spin/drivers/ide/trm290.c
--- linux-2.4.19-p4/drivers/ide/trm290.c	Thu Mar 21 07:05:06 2002
+++ linux-2.4.19-p4.spin/drivers/ide/trm290.c	Wed Mar 27 22:14:07 2002
@@ -148,12 +148,13 @@
 	/* select PIO or DMA */
 	reg = use_dma ? (0x21 | 0x82) : (0x21 & ~0x82);
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_disable();	/* local CPU only */
 
 	if (reg != hwif->select_data) {
 		hwif->select_data = reg;
-		outb(0x51|(hwif->channel<<3), hwif->config_data+1);	/* set PIO/DMA */
+		outb(0x51|(hwif->channel<<3), hwif->config_data+1);
+			/* set PIO/DMA */
 		outw(reg & 0xff, hwif->config_data);
 	}
 
@@ -163,8 +164,7 @@
 		reg &= ~(1 << hwif->channel);
 		outw(reg, hwif->config_data+3);
 	}
-
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 }
 
 static void trm290_selectproc (ide_drive_t *drive)
@@ -252,21 +252,21 @@
 		printk("TRM290: using default config base at 0x%04lx\n", hwif->config_data);
 	}
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_disable();	/* local CPU only */
 	/* put config reg into first byte of hwif->select_data */
 	outb(0x51|(hwif->channel<<3), hwif->config_data+1);
-	hwif->select_data = 0x21;			/* select PIO as default */
+	hwif->select_data = 0x21;		/* select PIO as default */
 	outb(hwif->select_data, hwif->config_data);
-	reg = inb(hwif->config_data+3);			/* get IRQ info */
-	reg = (reg & 0x10) | 0x03;			/* mask IRQs for both ports */
+	reg = inb(hwif->config_data+3);		/* get IRQ info */
+	reg = (reg & 0x10) | 0x03;		/* mask IRQs for both ports */
 	outb(reg, hwif->config_data+3);
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);	/* local CPU only */
 
 	if ((reg & 0x10))
 		hwif->irq = hwif->channel ? 15 : 14;	/* legacy mode */
 	else if (!hwif->irq && hwif->mate && hwif->mate->irq)
-		hwif->irq = hwif->mate->irq;		/* sharing IRQ with mate */
+		hwif->irq = hwif->mate->irq;	/* sharing IRQ with mate */
 	ide_setup_dma(hwif, (hwif->config_data + 4) ^ (hwif->channel ? 0x0080 : 0x0000), 3);
 
 #ifdef CONFIG_BLK_DEV_IDEDMA
diff -urN linux-2.4.19-p4/drivers/ide/umc8672.c linux-2.4.19-p4.spin/drivers/ide/umc8672.c
--- linux-2.4.19-p4/drivers/ide/umc8672.c	Thu Apr 13 22:54:26 2000
+++ linux-2.4.19-p4.spin/drivers/ide/umc8672.c	Wed Mar 27 21:29:36 2002
@@ -92,13 +92,11 @@
 	out_umc (0xd7,(speedtab[0][speeds[2]] | (speedtab[0][speeds[3]]<<4)));
 	out_umc (0xd6,(speedtab[0][speeds[0]] | (speedtab[0][speeds[1]]<<4)));
 	tmp = 0;
-	for (i = 3; i >= 0; i--)
-	{
+	for (i = 3; i >= 0; i--) {
 		tmp = (tmp << 2) | speedtab[1][speeds[i]];
 	}
 	out_umc (0xdc,tmp);
-	for (i = 0;i < 4; i++)
-	{
+	for (i = 0;i < 4; i++) {
 		out_umc (0xd0+i,speedtab[2][speeds[i]]);
 		out_umc (0xd8+i,speedtab[2][speeds[i]]);
 	}
@@ -115,39 +113,37 @@
 
 	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
 	printk("%s: setting umc8672 to PIO mode%d (speed %d)\n", drive->name, pio, pio_to_umc[pio]);
-	save_flags(flags);	/* all CPUs */
-	cli();			/* all CPUs */
+	spin_lock_irqsave(&io_request_lock, flags);
 	if (hwgroup && hwgroup->handler != NULL) {
 		printk("umc8672: other interface is busy: exiting tune_umc()\n");
 	} else {
 		current_speeds[drive->name[2] - 'a'] = pio_to_umc[pio];
 		umc_set_speeds (current_speeds);
 	}
-	restore_flags(flags);	/* all CPUs */
+	spin_unlock_irqrestore(&io_request_lock, flags);
 }
 
 void __init init_umc8672 (void)	/* called from ide.c */
 {
 	unsigned long flags;
 
-	__save_flags(flags);	/* local CPU only */
-	__cli();		/* local CPU only */
+	local_irq_save(flags);	/* local CPU only */
+	local_irq_disable();	/* local CPU only */
 	if (check_region(0x108, 2)) {
-		__restore_flags(flags);
+		local_irq_restore(flags);	/* local CPU only */
 		printk("\numc8672: PORTS 0x108-0x109 ALREADY IN USE\n");
 		return;
 	}
 	outb_p (0x5A,0x108); /* enable umc */
-	if (in_umc (0xd5) != 0xa0)
-	{
-		__restore_flags(flags);	/* local CPU only */
+	if (in_umc (0xd5) != 0xa0) {
+		local_irq_restore(flags);	/* local CPU only */
 		printk ("umc8672: not found\n");
 		return;  
 	}
 	outb_p (0xa5,0x108); /* disable umc */
 
 	umc_set_speeds (current_speeds);
-	__restore_flags(flags);	/* local CPU only */
+	local_irq_restore(flags);		/* local CPU only */
 
 	request_region(0x108, 2, "umc8672");
 	ide_hwifs[0].chipset = ide_umc8672;
diff -urN linux-2.4.19-p4/drivers/scsi/ide-scsi.c linux-2.4.19-p4.spin/drivers/scsi/ide-scsi.c
--- linux-2.4.19-p4/drivers/scsi/ide-scsi.c	Fri Mar 29 15:31:38 2002
+++ linux-2.4.19-p4.spin/drivers/scsi/ide-scsi.c	Fri Mar 29 15:32:12 2002
@@ -334,7 +334,7 @@
 	if ((status & DRQ_STAT) == 0) {					/* No more interrupts */
 		if (test_bit(IDESCSI_LOG_CMD, &scsi->log))
 			printk (KERN_INFO "Packet command completed, %d bytes transferred\n", pc->actually_transferred);
-		ide__sti();
+		local_irq_enable();
 		if (status & ERR_STAT)
 			rq->errors++;
 		idescsi_end_request(drive, 1);

Re: [RFC/CFT] cmd640 irqlocking fixes

[Alan Cox]

Martin this patch should do the job. It uses the correct pci_config_lock
and it also adds the 2.4 probe safety checks for deciding which pci
modes to use.

--- ../linux-2.5.28/drivers/ide/cmd640.c	Thu Jul 25 10:49:19 2002
+++ drivers/ide/cmd640.c	Thu Jul 25 11:41:27 2002
@@ -216,27 +216,27 @@
 
 /* PCI method 1 access */
 
-static void put_cmd640_reg_pci1 (unsigned short reg, byte val)
+extern spinlock_t pci_config_lock;
+
+static void put_cmd640_reg_pci1 (unsigned short reg, u8 val)
 {
 	unsigned long flags;
-
-	save_flags(flags);
-	cli();
-	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
+	
+	spin_lock_irqsave(&pci_config_lock, flags);
+	outb_p((reg & 0xfc) | cmd640_key, 0xcf8);
 	outb_p(val, (reg & 3) | 0xcfc);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&pci_config_lock, flags);
 }
 
 static u8 get_cmd640_reg_pci1 (unsigned short reg)
 {
 	u8 b;
 	unsigned long flags;
-
-	save_flags(flags);
-	cli();
-	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
-	b = inb_p((reg & 3) | 0xcfc);
-	restore_flags(flags);
+	
+	spin_lock_irqsave(&pci_config_lock, flags);
+	outb_p((reg & 0xfc) | cmd640_key, 0xcf8);
+	b=inb_p((reg & 3) | 0xcfc);
+	spin_unlock_irqrestore(&pci_config_lock, flags);
 	return b;
 }
 
@@ -245,26 +245,24 @@
 static void put_cmd640_reg_pci2 (unsigned short reg, u8 val)
 {
 	unsigned long flags;
-
-	save_flags(flags);
-	cli();
+	
+	spin_lock_irqsave(&pci_config_lock, flags);
 	outb_p(0x10, 0xcf8);
 	outb_p(val, cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&pci_config_lock, flags);
 }
 
 static u8 get_cmd640_reg_pci2 (unsigned short reg)
 {
 	u8 b;
 	unsigned long flags;
-
-	save_flags(flags);
-	cli();
+	
+	spin_lock_irqsave(&pci_config_lock, flags);
 	outb_p(0x10, 0xcf8);
 	b = inb_p(cmd640_key + reg);
 	outb_p(0, 0xcf8);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&pci_config_lock, flags);
 	return b;
 }
 
@@ -701,9 +699,62 @@
 
 #endif
 
+/**
+ *	pci_conf1	-	check for PCI type 1 configuration
+ *	
+ *	Issues a safe probe sequence for PCI configuration type 1 and
+ *	returns non-zero if conf1 is supported. Takes the pci_config lock
+ */
+ 
+static int pci_conf1(void)
+{
+	u32 tmp;
+	unsigned long flags;
+	
+	spin_lock_irqsave(&pci_config_lock, flags);
+
+	OUT_BYTE(0x01, 0xCFB);
+	tmp = inl(0xCF8);
+	outl(0x80000000, 0xCF8);
+	if (inl(0xCF8) == 0x80000000) {
+		spin_unlock_irqrestore(&pci_config_lock, flags);
+		outl(tmp, 0xCF8);
+		return 1;
+	}
+	outl(tmp, 0xCF8);
+	spin_unlock_irqrestore(&pci_config_lock, flags);
+	return 0;
+}
+
+/**
+ *	pci_conf2	-	check for PCI type 2 configuration
+ *	
+ *	Issues a safe probe sequence for PCI configuration type 2 and
+ *	returns non-zero if conf2 is supported. Takes the pci_config lock.
+ */
+ 
+
+static int pci_conf2(void)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&pci_config_lock, flags);
+	
+	OUT_BYTE(0x00, 0xCFB);
+	OUT_BYTE(0x00, 0xCF8);
+	OUT_BYTE(0x00, 0xCFA);
+	if (IN_BYTE(0xCF8) == 0x00 && IN_BYTE(0xCF8) == 0x00) {
+		spin_unlock_irqrestore(&pci_config_lock, flags);
+		return 1;
+	}
+	spin_unlock_irqrestore(&pci_config_lock, flags);
+	return 0;
+}
+
+
 /*
  * Probe for a cmd640 chipset, and initialize it if found.  Called from ide.c
  */
+ 
 int __init ide_probe_for_cmd640x(void)
 {
 #ifdef CONFIG_BLK_DEV_CMD640_ENHANCED
@@ -718,9 +769,10 @@
 		bus_type = "VLB";
 	} else {
 		cmd640_vlb = 0;
-		if (probe_for_cmd640_pci1())
+		/* Find out what kind of PCI probing is supported */
+		if (pci_conf1() && probe_for_cmd640_pci1())
 			bus_type = "PCI (type1)";
-		else if (probe_for_cmd640_pci2())
+		else if (pci_conf2() && probe_for_cmd640_pci2())
 			bus_type = "PCI (type2)";
 		else
 			return 0;

Re: [RFC/CFT] cmd640 irqlocking fixes

[Marcin Dalecki]

Alan Cox wrote:
> Martin this patch should do the job. It uses the correct pci_config_lock
> and it also adds the 2.4 probe safety checks for deciding which pci
> modes to use.

I'm sure it does the job indeed ;-).

Thanks - I will include it in the next spin.

Re: [RFC/CFT] cmd640 irqlocking fixes

[Andre Hedrick]

EEK!

Alan, how are you going to sync to access to the shared HBA registers if
the lock is decoupled from the main loop?  Since there are general
register mucking abouts during data transfers, iirc the behavors of the
CMD640B I use for testing.  You have me a little close to the edge of my
seat on concern.


On 25 Jul 2002, Alan Cox wrote:

> Martin this patch should do the job. It uses the correct pci_config_lock
> and it also adds the 2.4 probe safety checks for deciding which pci
> modes to use.
> 
> --- ../linux-2.5.28/drivers/ide/cmd640.c	Thu Jul 25 10:49:19 2002
> +++ drivers/ide/cmd640.c	Thu Jul 25 11:41:27 2002
> @@ -216,27 +216,27 @@
>  
>  /* PCI method 1 access */
>  
> -static void put_cmd640_reg_pci1 (unsigned short reg, byte val)
> +extern spinlock_t pci_config_lock;
> +
> +static void put_cmd640_reg_pci1 (unsigned short reg, u8 val)
>  {
>  	unsigned long flags;
> -
> -	save_flags(flags);
> -	cli();
> -	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
> +	
> +	spin_lock_irqsave(&pci_config_lock, flags);
> +	outb_p((reg & 0xfc) | cmd640_key, 0xcf8);
>  	outb_p(val, (reg & 3) | 0xcfc);
> -	restore_flags(flags);
> +	spin_unlock_irqrestore(&pci_config_lock, flags);
>  }
>  
>  static u8 get_cmd640_reg_pci1 (unsigned short reg)
>  {
>  	u8 b;
>  	unsigned long flags;
> -
> -	save_flags(flags);
> -	cli();
> -	outl_p((reg & 0xfc) | cmd640_key, 0xcf8);
> -	b = inb_p((reg & 3) | 0xcfc);
> -	restore_flags(flags);
> +	
> +	spin_lock_irqsave(&pci_config_lock, flags);
> +	outb_p((reg & 0xfc) | cmd640_key, 0xcf8);
> +	b=inb_p((reg & 3) | 0xcfc);
> +	spin_unlock_irqrestore(&pci_config_lock, flags);
>  	return b;
>  }
>  
> @@ -245,26 +245,24 @@
>  static void put_cmd640_reg_pci2 (unsigned short reg, u8 val)
>  {
>  	unsigned long flags;
> -
> -	save_flags(flags);
> -	cli();
> +	
> +	spin_lock_irqsave(&pci_config_lock, flags);
>  	outb_p(0x10, 0xcf8);
>  	outb_p(val, cmd640_key + reg);
>  	outb_p(0, 0xcf8);
> -	restore_flags(flags);
> +	spin_unlock_irqrestore(&pci_config_lock, flags);
>  }
>  
>  static u8 get_cmd640_reg_pci2 (unsigned short reg)
>  {
>  	u8 b;
>  	unsigned long flags;
> -
> -	save_flags(flags);
> -	cli();
> +	
> +	spin_lock_irqsave(&pci_config_lock, flags);
>  	outb_p(0x10, 0xcf8);
>  	b = inb_p(cmd640_key + reg);
>  	outb_p(0, 0xcf8);
> -	restore_flags(flags);
> +	spin_unlock_irqrestore(&pci_config_lock, flags);
>  	return b;
>  }
>  
> @@ -701,9 +699,62 @@
>  
>  #endif
>  
> +/**
> + *	pci_conf1	-	check for PCI type 1 configuration
> + *	
> + *	Issues a safe probe sequence for PCI configuration type 1 and
> + *	returns non-zero if conf1 is supported. Takes the pci_config lock
> + */
> + 
> +static int pci_conf1(void)
> +{
> +	u32 tmp;
> +	unsigned long flags;
> +	
> +	spin_lock_irqsave(&pci_config_lock, flags);
> +
> +	OUT_BYTE(0x01, 0xCFB);
> +	tmp = inl(0xCF8);
> +	outl(0x80000000, 0xCF8);
> +	if (inl(0xCF8) == 0x80000000) {
> +		spin_unlock_irqrestore(&pci_config_lock, flags);
> +		outl(tmp, 0xCF8);
> +		return 1;
> +	}
> +	outl(tmp, 0xCF8);
> +	spin_unlock_irqrestore(&pci_config_lock, flags);
> +	return 0;
> +}
> +
> +/**
> + *	pci_conf2	-	check for PCI type 2 configuration
> + *	
> + *	Issues a safe probe sequence for PCI configuration type 2 and
> + *	returns non-zero if conf2 is supported. Takes the pci_config lock.
> + */
> + 
> +
> +static int pci_conf2(void)
> +{
> +	unsigned long flags;
> +	spin_lock_irqsave(&pci_config_lock, flags);
> +	
> +	OUT_BYTE(0x00, 0xCFB);
> +	OUT_BYTE(0x00, 0xCF8);
> +	OUT_BYTE(0x00, 0xCFA);
> +	if (IN_BYTE(0xCF8) == 0x00 && IN_BYTE(0xCF8) == 0x00) {
> +		spin_unlock_irqrestore(&pci_config_lock, flags);
> +		return 1;
> +	}
> +	spin_unlock_irqrestore(&pci_config_lock, flags);
> +	return 0;
> +}
> +
> +
>  /*
>   * Probe for a cmd640 chipset, and initialize it if found.  Called from ide.c
>   */
> + 
>  int __init ide_probe_for_cmd640x(void)
>  {
>  #ifdef CONFIG_BLK_DEV_CMD640_ENHANCED
> @@ -718,9 +769,10 @@
>  		bus_type = "VLB";
>  	} else {
>  		cmd640_vlb = 0;
> -		if (probe_for_cmd640_pci1())
> +		/* Find out what kind of PCI probing is supported */
> +		if (pci_conf1() && probe_for_cmd640_pci1())
>  			bus_type = "PCI (type1)";
> -		else if (probe_for_cmd640_pci2())
> +		else if (pci_conf2() && probe_for_cmd640_pci2())
>  			bus_type = "PCI (type2)";
>  		else
>  			return 0;
> 

Andre Hedrick
LAD Storage Consulting Group

Re: [RFC/CFT] cmd640 irqlocking fixes

[Alan Cox]

On Thu, 2002-07-25 at 13:30, Andre Hedrick wrote:
> 
> EEK!
> 
> Alan, how are you going to sync to access to the shared HBA registers if
> the lock is decoupled from the main loop?  Since there are general
> register mucking abouts during data transfers, iirc the behavors of the
> CMD640B I use for testing.  You have me a little close to the edge of my
> seat on concern.

>From inspection I don't believe this is a problem. Also for example they
have not been synchronized for some time on VLB with no apparent
problems. If there are cases where PCI config needs synchronizing here
(which I dont believe is the case) then the caller should be holding the
io_request lock anyway, and seems to

Re: [RFC/CFT] cmd640 irqlocking fixes

[Benjamin Herrenschmidt]

>Martin this patch should do the job. It uses the correct pci_config_lock
>and it also adds the 2.4 probe safety checks for deciding which pci
>modes to use.

Hrm... pci_config_lock is specific to arch/i386 it seems (and is even
a static in 2.4.19rc3). That is no good as this isn't the only
driver to do config access from interrupts, so at least PPC is
broken in this regard.

Wouldn't it make sense to generalize it and implement it on all archs ?

(That is move extern declaration of it to linux/pci.h, definition to
drivers/pci/pci.c, and so on...)

I'd rather have a per-host lock, but on the other hand, the host bus
mecanism is still quite arch-specific, thus making difficult to use
a per-host lock in drivers, at least in 2.4


Ben.

PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[Benjamin Herrenschmidt]

>Martin this patch should do the job. It uses the correct pci_config_lock
>>and it also adds the 2.4 probe safety checks for deciding which pci
>>modes to use.
>
>Hrm... pci_config_lock is specific to arch/i386 it seems (and is even
>a static in 2.4.19rc3). That is no good as this isn't the only
>driver to do config access from interrupts, so at least PPC is
>broken in this regard.
>
>Wouldn't it make sense to generalize it and implement it on all archs ?
>
>(That is move extern declaration of it to linux/pci.h, definition to
>drivers/pci/pci.c, and so on...)
>
>I'd rather have a per-host lock, but on the other hand, the host bus
>mecanism is still quite arch-specific, thus making difficult to use
>a per-host lock in drivers, at least in 2.4

Ok, fixing my own crap...

So there seem to be a problem with your patch: pci_config_lock appears
to be an x86-only thing that lives deep inside arch/i386/xxx/pci-pc.c
(xxx beeing kernel or pci)

On the other hand, there is already such a lock provided by
drivers/pci/access.c (pci_lock). You should probably fix your patch
to use that one. (and eventually get rid of the pci_config_lock
in x86, provided I didn't miss something else). But does anybody
but x86 uses CMD640 ? :)

Now, after more thinking and discussions, it seems we may actually
want 2 different things:

 - Protecting the pci config ops themselves as they are usually
indirect access, and thus potentially race. This is also what the
current pci_lock does and is somewhat duplicated by the pci_config_lock
used on x86. That could eventually be moved to a per-host lock

 - Protecting a device, that is preventing (usually from a driver)
any config access during some time, either for doing what you are
doing in the cmd640 patch, but also for _batching_ a set of config
space accesses that have to be atomic together on this device.

The later would probably be better done with a per-device lock. What
I don't like that much here is that normal access will result in 2
spinlocks beeing taken, though config space accesses are usually
rare and slow compared to other IOs, so it may not be that a problem.

That would require the pci config ops to be split into normal
pci_config_xxx that does take the per-device lock, and _pci_config_xxx
that doesn't, so a driver can manually take that lock, batch accesses
and release it.

What do you think ?

Ben.

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[Alan Cox]

On Thu, 2002-07-25 at 15:18, Benjamin Herrenschmidt wrote:
> So there seem to be a problem with your patch: pci_config_lock appears
> to be an x86-only thing that lives deep inside arch/i386/xxx/pci-pc.c
> (xxx beeing kernel or pci)
> 
> On the other hand, there is already such a lock provided by
> drivers/pci/access.c (pci_lock). You should probably fix your patch
> to use that one. (and eventually get rid of the pci_config_lock
> in x86, provided I didn't miss something else). But does anybody
> but x86 uses CMD640 ? :)

Possibly. I don't know. The lock in question we want is the config lock.
We are issuing configuration cycles so must lock against a parallel
issue of I/O to the configuration ports.

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[benh]

>> So there seem to be a problem with your patch: pci_config_lock appears
>> to be an x86-only thing that lives deep inside arch/i386/xxx/pci-pc.c
>> (xxx beeing kernel or pci)
>> 
>> On the other hand, there is already such a lock provided by
>> drivers/pci/access.c (pci_lock). You should probably fix your patch
>> to use that one. (and eventually get rid of the pci_config_lock
>> in x86, provided I didn't miss something else). But does anybody
>> but x86 uses CMD640 ? :)
>
>Possibly. I don't know. The lock in question we want is the config lock.
>We are issuing configuration cycles so must lock against a parallel
>issue of I/O to the configuration ports.

Well, all config operations are going through the
pci_read/write_config_xxxx in drivers/pci/pci.c or access.c (2.5)
which will take the pci_lock already. Couldn't x86 use that instead
of stacking another lock ?

Ben.

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[Alan Cox]

> Well, all config operations are going through the
> pci_read/write_config_xxxx in drivers/pci/pci.c or access.c (2.5)
> which will take the pci_lock already. Couldn't x86 use that instead
> of stacking another lock ?

I'll take a look. We probably can

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[Alan Cox]

> Well, all config operations are going through the
> pci_read/write_config_xxxx in drivers/pci/pci.c or access.c (2.5)
> which will take the pci_lock already. Couldn't x86 use that instead
> of stacking another lock ?

Yes - done in my tree

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[Dave Jones]

On Thu, Jul 25, 2002 at 04:45:15PM +0100, Alan Cox wrote:
 > But does anybody but x86 uses CMD640 ? :)
 > Possibly. I don't know.

ISTR these monsters appear on some Alphas too ?

        Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[Thunder from the hill]

Hi,

On Thu, 25 Jul 2002, Dave Jones wrote:
> ISTR these monsters appear on some Alphas too ?

Somehow it seems to be a yes. I don't have it, but yes, something like 
that must have been there.

			Thunder

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[David S. Miller]

   From: Dave Jones <davej@suse.de>
   Date: Thu, 25 Jul 2002 16:48:07 +0200

   On Thu, Jul 25, 2002 at 04:45:15PM +0100, Alan Cox wrote:
    > But does anybody but x86 uses CMD640 ? :)
    > Possibly. I don't know.
   
   ISTR these monsters appear on some Alphas too ?
   
Several Alpha's have the CMD646, which uses a different driver.  Maybe
some really really old Alpha's had the CMD640.

Re: PCI config locking (WAS Re: [RFC/CFT] cmd640 irqlocking fixes)2

[Marcin Dalecki]

Benjamin Herrenschmidt wrote:
>>Martin this patch should do the job. It uses the correct pci_config_lock
>>
>>>and it also adds the 2.4 probe safety checks for deciding which pci
>>>modes to use.
>>
>>Hrm... pci_config_lock is specific to arch/i386 it seems (and is even
>>a static in 2.4.19rc3). That is no good as this isn't the only
>>driver to do config access from interrupts, so at least PPC is
>>broken in this regard.
>>
>>Wouldn't it make sense to generalize it and implement it on all archs ?
>>
>>(That is move extern declaration of it to linux/pci.h, definition to
>>drivers/pci/pci.c, and so on...)
>>
>>I'd rather have a per-host lock, but on the other hand, the host bus
>>mecanism is still quite arch-specific, thus making difficult to use
>>a per-host lock in drivers, at least in 2.4
> 
> 
> Ok, fixing my own crap...
> 
> So there seem to be a problem with your patch: pci_config_lock appears
> to be an x86-only thing that lives deep inside arch/i386/xxx/pci-pc.c
> (xxx beeing kernel or pci)
> 
> On the other hand, there is already such a lock provided by
> drivers/pci/access.c (pci_lock). You should probably fix your patch
> to use that one. (and eventually get rid of the pci_config_lock
> in x86, provided I didn't miss something else). But does anybody
> but x86 uses CMD640 ? :)

I agree on the pci_lock item.
And yes CMD640 chips where quite common on Sparcs about 4-6 years ago.
I think some Alpha based systems used them too... but I'm not sure.
Regarding the locking issue. I think the best place to
put it would be just before call down to the corresonding low level
functions in the generic IDE layer. We may "overlock" a bit here -
But who cares?  This is by no way a time critical operation.

Re: [RFC/CFT] cmd640 irqlocking fixes

[Albert D. Cahalan]

Vojtech Pavlik writes:

> We really should be using pci_write_config_* and
> create vlb_write_config_* in CMD640 for the VLB accesses, panic() in
> case we have a PCI system that uses BIOS and we found a CMD640

You should not panic, since there may be another disk controller.
Just disable the CMD640 by grabbing the resources and forgetting them.

Re: [RFC/CFT] cmd640 irqlocking fixes

[Alan Cox]

On Thu, 2002-07-25 at 08:54, Vojtech Pavlik wrote:
> IMHO the best workaround here would be either to disable PCIBIOS calls
> and revert to conf1 or conf2 in the PCI code if a CMD640 is present, or
> just panic() in the CMD640 code and suggest to the user to use
> "pci=nobios" on the kernel command line. I'd actually prefer the later.

For x86 we can probably do that. It would needlessly break 2.0/2.2/2.4
functionality for the sake of some trivial lock handling. Much better to
fix the problem. I'll take a look at it, when I merge all the other
needed pci config fixes from 2.4 into 2.5, and I guess start putting out
2.5-ac since Linus didnt take all my patches.

[The 2.5 CMD640 code breaks some adaptec stuff which is fixed in the 2.4
 code]

Alan

Re: [RFC/CFT] cmd640 irqlocking fixes

[Marcin Dalecki]

William Lee Irwin III wrote:
> I don't have one of these, and I'm not even sure what it is. But here's
> a wild guess at a fix.
> 
> 
> Cheers,
> Bill
> ===== drivers/ide/cmd640.c 1.11 vs edited =====
> --- 1.11/drivers/ide/cmd640.c	Wed May 22 04:21:11 2002
> +++ edited/drivers/ide/cmd640.c	Wed Jul 24 18:51:54 2002
> @@ -115,6 +115,12 @@
>  #include "ata-timing.h"
>  
>  /*
> + * Is this remotely correct?
> + */

The proper fix (rating: 50% propability of beeing perfect and 99.99% 
propability of working) is just to *remove* those georgeous IRQ
"tooglers" *alltogether*. Scrap them... becouse I don't see how any
of the encolosed commands could cause an IRQ...

Re: [RFC/CFT] cmd640 irqlocking fixes

[Petr Vandrovec]

On 25 Jul 02 at 14:08, Alan Cox wrote:

> +   OUT_BYTE(0x00, 0xCFB);
> +   OUT_BYTE(0x00, 0xCF8);
> +   OUT_BYTE(0x00, 0xCFA);
> +   if (IN_BYTE(0xCF8) == 0x00 && IN_BYTE(0xCF8) == 0x00) {
                                            ^^^^^
It should be 0xCFA according to arch/i386/pci/direct.c...
                                        Petr Vandrovec
                                        vandrove@vc.cvut.cz
                                        

Re: [RFC/CFT] cmd640 irqlocking fixes

[Alan Cox]

On Thu, 2002-07-25 at 13:50, Petr Vandrovec wrote:
> On 25 Jul 02 at 14:08, Alan Cox wrote:
> 
> > +   OUT_BYTE(0x00, 0xCFB);
> > +   OUT_BYTE(0x00, 0xCF8);
> > +   OUT_BYTE(0x00, 0xCFA);
> > +   if (IN_BYTE(0xCF8) == 0x00 && IN_BYTE(0xCF8) == 0x00) {
>                                             ^^^^^
> It should be 0xCFA according to arch/i386/pci/direct.c...

Correct. Martin I agree with this change.

Re: [RFC/CFT] cmd640 irqlocking fixes

[Marcin Dalecki]

Alan Cox wrote:
> On Thu, 2002-07-25 at 13:50, Petr Vandrovec wrote:
> 
>>On 25 Jul 02 at 14:08, Alan Cox wrote:
>>
>>
>>>+   OUT_BYTE(0x00, 0xCFB);
>>>+   OUT_BYTE(0x00, 0xCF8);
>>>+   OUT_BYTE(0x00, 0xCFA);
>>>+   if (IN_BYTE(0xCF8) == 0x00 && IN_BYTE(0xCF8) == 0x00) {
>>
>>                                            ^^^^^
>>It should be 0xCFA according to arch/i386/pci/direct.c...
> 
> 
> Correct. Martin I agree with this change.

Thanks. I noted it already.