Skip to contentSkip to conversation

cmd640 irqlocking fixes

RFC/CFT William Lee Irwin III 10 participants 29 messages Jul 24, 2002 – Jul 29, 2002 SCSI

lore
Share & tools
lore

William Lee Irwin III

author
#0 lore raw

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

drivers/ide/cmd640.c +25 -28
@@ -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);
 }
 
 /*
author
#1 lore raw
Quoted 2 lines · 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.

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

Cheers,
Bill

#2 lore raw
In reply to William Lee Irwin III
Quoted 2 lines · 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.

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

#3 lore raw
Quoted 12 lines · Alan Cox
Quoted 2 lines

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.

Quoted 6 lines

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.

Signature · 2 lines
Vojtech Pavlik
SuSE Labs
#4 lore raw
Quoted 11 lines · Vojtech Pavlik

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.

#5 lore raw
Quoted 14 lines · Marcin Dalecki
Quoted 11 lines · Vojtech Pavlik

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.

Quoted 4 lines

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

Signature · 2 lines
Vojtech Pavlik
SuSE Labs
#6 lore raw
Quoted 20 lines · Vojtech Pavlik
Quoted 8 lines · Marcin Dalecki
Quoted 5 lines · Vojtech Pavlik

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.

Quoted 8 lines
Quoted 5 lines

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.

Quoted 5 lines

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

#7 lore raw
Quoted 3 lines · Marcin Dalecki

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

#8 lore raw
Quoted 8 lines · Alan Cox
Quoted 2 lines · Marcin Dalecki

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 :-).

#9 lore raw
In reply to Alan Cox
Quoted 8 lines · Alan Cox
Quoted 3 lines · Marcin Dalecki

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

drivers/ide/cmd640.c +38 -51
diff -urN linux-2.4.19-p8-ac1-pristine/drivers/ide/cmd640.c linux-2.4.19-p8-ac1/drivers/ide/cmd640.c
@@ -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())
#10 lore raw
Quoted 2 lines · Andre Hedrick

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.

Quoted 3 lines

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.

drivers/ide/cmd640.c line 741

Quoted 10 lines
drivers/ide/cmd640.c +2 -4
@@ -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)

#11 lore raw

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.

ide/cmd640.c renamed from linux-2.5.28/drivers/ide/cmd640.c +74 -22
@@ -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;
#12 lore raw
Quoted 3 lines · 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.

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

Thanks - I will include it in the next spin.

#13 lore raw
In reply to Alan Cox

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.

Quoted 146 lines · 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.

ide/cmd640.c renamed from linux-2.5.28/drivers/ide/cmd640.c +71 -20
@@ -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 {

ide/cmd640.c line 778

Quoted 10 lines
ide/cmd640.c renamed from linux-2.5.28/drivers/ide/cmd640.c +3 -2
 		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

#14 lore raw
Quoted 10 lines · 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.

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

#15 lore raw
In reply to Alan Cox
Quoted 3 lines

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.

#16 lore raw

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

Quoted 9 lines

Martin this patch should do the job. It uses the correct pci_config_lock

Quoted 2 lines

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.

Quoted 8 lines

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.

#17 lore raw

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

Quoted 9 lines · Benjamin Herrenschmidt

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.

#18 lore raw

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

Quoted 10 lines
Quoted 9 lines

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 ? :)

Quoted 3 lines

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.

#19 lore raw

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

Quoted 5 lines

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

#20 lore raw

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

In reply to [email protected]
Quoted 4 lines

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

#21 lore raw

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

In reply to Alan Cox
Quoted 2 lines · Alan Cox

But does anybody but x86 uses CMD640 ? :)
Possibly. I don't know.

ISTR these monsters appear on some Alphas too ?

Dave

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
#22 lore raw

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

Hi,

Quoted 1 line · Dave Jones

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

#23 lore raw

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

In reply to Dave Jones

From: Dave Jones <[email protected]>
Date: Thu, 25 Jul 2002 16:48:07 +0200

Quoted 2 lines · Alan Cox

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.

#24 lore raw

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

In reply to Benjamin Herrenschmidt
Quoted 26 lines · Benjamin Herrenschmidt
Quoted 10 lines

Martin this patch should do the job. It uses the correct pci_config_lock

Quoted 2 lines

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.

Quoted 8 lines

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)

Quoted 5 lines

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.

#25 lore raw
In reply to Alan Cox

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

Quoted 16 lines · Alan Cox
Quoted 2 lines · Andre Hedrick

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.

Quoted 3 lines

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.

Quoted 17 lines

Review line 741

Quoted 10 lines
(quoted diff) +2 -4
@@ -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)

drivers/ide/cmd640.c +42 -2
@@ -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;
drivers/ide/ali14xx.c +9 -10
diff -urN linux-2.4.19-p4/drivers/ide/ali14xx.c linux-2.4.19-p4.spin/drivers/ide/ali14xx.c
@@ -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;
 }
drivers/ide/alim15x3.c +8 -69

143 lines not on this page · raw message

drivers/ide/cmd640.c +19 -29

137 lines not on this page · raw message

drivers/ide/cmd64x.c +4 -3
diff -urN linux-2.4.19-p4/drivers/ide/cmd64x.c linux-2.4.19-p4.spin/drivers/ide/cmd64x.c
@@ -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 */
 }
 
 /*
drivers/ide/cs5530.c +3 -4
diff -urN linux-2.4.19-p4/drivers/ide/cs5530.c linux-2.4.19-p4.spin/drivers/ide/cs5530.c
@@ -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;
 }
drivers/ide/dtc2278.c +5 -6
diff -urN linux-2.4.19-p4/drivers/ide/dtc2278.c linux-2.4.19-p4.spin/drivers/ide/dtc2278.c
@@ -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;
drivers/ide/hpt34x.c +3 -3
diff -urN linux-2.4.19-p4/drivers/ide/hpt34x.c linux-2.4.19-p4.spin/drivers/ide/hpt34x.c
@@ -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) {
drivers/ide/hpt366.c +3 -3
diff -urN linux-2.4.19-p4/drivers/ide/hpt366.c linux-2.4.19-p4.spin/drivers/ide/hpt366.c
@@ -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);
 }
drivers/ide/ht6560b.c +9 -13
diff -urN linux-2.4.19-p4/drivers/ide/ht6560b.c linux-2.4.19-p4.spin/drivers/ide/ht6560b.c
@@ -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);
drivers/ide/ide-disk.c +1 -1
diff -urN linux-2.4.19-p4/drivers/ide/ide-disk.c linux-2.4.19-p4.spin/drivers/ide/ide-disk.c
@@ -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);
 	/*
drivers/ide/ide-features.c +9 -9
diff -urN linux-2.4.19-p4/drivers/ide/ide-features.c linux-2.4.19-p4.spin/drivers/ide/ide-features.c
@@ -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 */
 	}
 
 	/*
drivers/ide/ide-floppy.c +4 -4
diff -urN linux-2.4.19-p4/drivers/ide/ide-floppy.c linux-2.4.19-p4.spin/drivers/ide/ide-floppy.c
@@ -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;
 	}
drivers/ide/ide-pmac.c +3 -3
diff -urN linux-2.4.19-p4/drivers/ide/ide-pmac.c linux-2.4.19-p4.spin/drivers/ide/ide-pmac.c
@@ -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");
drivers/ide/ide-probe.c +15 -13
diff -urN linux-2.4.19-p4/drivers/ide/ide-probe.c linux-2.4.19-p4.spin/drivers/ide/ide-probe.c
@@ -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,
drivers/ide/ide-proc.c +1 -1
diff -urN linux-2.4.19-p4/drivers/ide/ide-proc.c linux-2.4.19-p4.spin/drivers/ide/ide-proc.c
@@ -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);
 }
drivers/ide/ide-tape.c +7 -6
diff -urN linux-2.4.19-p4/drivers/ide/ide-tape.c linux-2.4.19-p4.spin/drivers/ide/ide-tape.c
@@ -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;
drivers/ide/ide-taskfile.c +10 -11
diff -urN linux-2.4.19-p4/drivers/ide/ide-taskfile.c linux-2.4.19-p4.spin/drivers/ide/ide-taskfile.c
@@ -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 */
drivers/ide/ide.c +53 -47

270 lines not on this page · raw message

drivers/ide/it8172.c +2 -3
diff -urN linux-2.4.19-p4/drivers/ide/it8172.c linux-2.4.19-p4.spin/drivers/ide/it8172.c
@@ -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)
drivers/ide/ns87415.c +4 -4
diff -urN linux-2.4.19-p4/drivers/ide/ns87415.c linux-2.4.19-p4.spin/drivers/ide/ns87415.c
@@ -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)
drivers/ide/opti621.c +2 -4
diff -urN linux-2.4.19-p4/drivers/ide/opti621.c linux-2.4.19-p4.spin/drivers/ide/opti621.c
@@ -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);
 }
 
 /*
drivers/ide/pdc4030.c +1 -1
diff -urN linux-2.4.19-p4/drivers/ide/pdc4030.c linux-2.4.19-p4.spin/drivers/ide/pdc4030.c
@@ -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:
drivers/ide/piix.c +2 -3
diff -urN linux-2.4.19-p4/drivers/ide/piix.c linux-2.4.19-p4.spin/drivers/ide/piix.c
@@ -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)
drivers/ide/qd65xx.c +6 -9
diff -urN linux-2.4.19-p4/drivers/ide/qd65xx.c linux-2.4.19-p4.spin/drivers/ide/qd65xx.c
@@ -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");
drivers/ide/slc90e66.c +2 -3
diff -urN linux-2.4.19-p4/drivers/ide/slc90e66.c linux-2.4.19-p4.spin/drivers/ide/slc90e66.c
@@ -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
drivers/ide/trm290.c +12 -12
diff -urN linux-2.4.19-p4/drivers/ide/trm290.c linux-2.4.19-p4.spin/drivers/ide/trm290.c
@@ -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
drivers/ide/umc8672.c +10 -14
diff -urN linux-2.4.19-p4/drivers/ide/umc8672.c linux-2.4.19-p4.spin/drivers/ide/umc8672.c
@@ -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;
drivers/scsi/ide-scsi.c +1 -1
diff -urN linux-2.4.19-p4/drivers/scsi/ide-scsi.c linux-2.4.19-p4.spin/drivers/scsi/ide-scsi.c
@@ -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);
#26 lore raw
In reply to Vojtech Pavlik
Quoted 3 lines · Vojtech Pavlik

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.

#27 lore raw
In reply to Vojtech Pavlik
Quoted 4 lines · Vojtech Pavlik

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

#28 lore raw
In reply to William Lee Irwin III
Quoted 10 lines · 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 =====

drivers/ide/cmd640.c line 119

Quoted 5 lines
drivers/ide/cmd640.c +2 -0
@@ -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...