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);
}
/*
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
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
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
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...
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.
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
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..
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
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
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 :-).
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
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)
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;
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.
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)
>
>
>
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
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
[email protected]
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
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.
>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.
>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.
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.
>> 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.
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
> 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
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
> 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
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.
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.
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.
From: Dave Jones <[email protected]>
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.