Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards)

Hi,

this patch fixes the hard hang (no SYSRQ) on inserting any PCMCIA ATA/IDE card (e.g. CompactFlash, Clik40 etc) to a PCI-Cardbus bridge add-in card.

Thanks David for his valuable explanation about what happens:
ide-probe registers it's irq handler too late! After it triggers the interrupt during the probe the (shared) irq loops forever, effectively wedging the machine completely.

Regards, Gunther

drivers/ide/ide-cs.c Tue Jun 26 21:22:19 2001 renamed from drivers/ide/ide-cs.c Fri Feb 9 20:40:02 2001 +3 -0

@@ -324,6 +324,9 @@
     if (link->io.NumPorts2)
        release_region(link->io.BasePort2, link->io.NumPorts2);
 
+    outb(0x02, ctl_base); // Set nIEN = disable device interrupts
+                         // else it hangs on PCI-Cardbus add-in cards, wedging irq
+
     /* retry registration in case device is still spinning up */
     for (i = 0; i < 10; i++) {
        hd = ide_register(io_base, ctl_base, link->irq.AssignedIRQ);

I can not help if you have a device that not compliant to the rules. ATA-2 is OBSOLETED thus we forced (the NCITS Standards Body) the CFA

Quoted 2 lines

I can not help if you have a device that not compliant to the rules. ATA-2 is OBSOLETED thus we forced (the NCITS Standards Body) the CFA

ATA-2 may be obsolete but existing ATA-2 hardware doesnt spontaenously combust when the spec changes (much Im sure to some vendors dissappointmnent)

obsoleting ATA-2 did their attention at CFA become alarmed. I agree that there needs to be a fix, but not at the price of locking the rest of the driver. Since we now the identity of the device prior to assigned the interrupt we can handle the execption, but you do not go around blanket wacking the control register of all devices.

Quoted 5 lines

obsoleting ATA-2 did their attention at CFA become alarmed. I agree that there needs to be a fix, but not at the price of locking the rest of the driver. Since we now the identity of the device prior to assigned the interrupt we can handle the execption, but you do not go around blanket wacking the control register of all devices.

I dont see why it locks up the driver ?

PARANIOA.

Remember that ATAPI is generally screwed beyond reality, so adjusting the probe code in general (global) is a bad thing.

Quoted 6 lines

Quoted 6 lines

obsoleting ATA-2 did their attention at CFA become alarmed. I agree that there needs to be a fix, but not at the price of locking the rest of the driver. Since we now the identity of the device prior to assigned the interrupt we can handle the execption, but you do not go around blanket wacking the control register of all devices.

Quoted 2 lines

I dont see why it locks up the driver ?

Quoted 2 lines · Andre Hedrick

PARANIOA.

This is not a valid reason.

This clearly fixes a bug in linux. Note: the irq disable is local to ide-cs. Are you paranoid enough to believe enabling the irq by writing globally to the control register that existed since ATA will have ill effects?

You claim the relevant PCMCIA ATA behaviour is not ATA(>3?) compliant, however you didn`t yet give any facts to support this !

You claim this locks the driver, again no facts.

Quoted 3 lines

Remember that ATAPI is generally screwed beyond reality, so adjusting the probe code in general (global) is a bad thing.

...

Quoted 7 lines · Alan Cox

Quoted 6 lines

Quoted 6 lines

obsoleting ATA-2 did their attention at CFA become alarmed. I agree that there needs to be a fix, but not at the price of locking the rest of the driver. Since we now the identity of the device prior to assigned the interrupt we can handle the execption, but you do not go around blanket wacking the control register of all devices.

The proposed patch is very simple (as per Linus' liking). When considering to install an earlier (and global) irq handler I believe you can see this will impose a much greater risk !

Quoted 2 lines

Quoted 2 lines

I dont see why it locks up the driver ?

It fixes a BUG in CFA, but what will it do to the other stuff? Parse it exclusive to CFA and there is not an issue.

Not all ./arch have a control register doing this randomly without know the rest of the driver will kill more than it fixes.

Quoted 3 lines · Andre Hedrick

It fixes a BUG in CFA, but what will it do to the other stuff? Parse it exclusive to CFA and there is not an issue.

...

Quoted 3 lines

Not all ./arch have a control register doing this randomly without know the rest of the driver will kill more than it fixes.

Thanks for pointing out this implementation bug. Although I fixed another problem in ide-cs, where ctl_base could eventually be 0.

I would rather not add a special hwif->is_pcmcia flag, as the control register (if it exists) is well defined (bit2=softreset bit1=nIEN, others reserved; however there is
a hardcoded value of 0x08 somewhere in the ide code?).

-
Gunther

drivers/ide/ide-cs.c Thu Jun 28 18:04:27 2001 renamed from drivers/ide/ide-cs.c Fri Feb 9 20:40:02 2001 +14 -2

@@ -42,6 +42,7 @@
 #include <linux/ioport.h>
 #include <linux/hdreg.h>
 #include <linux/major.h>
+#include <linux/ide.h>
 
 #include <asm/io.h>
 #include <asm/system.h>

@@ -223,6 +224,15 @@
 #define CFG_CHECK(fn, args...) \
 if (CardServices(fn, args) != 0) goto next_entry
 
+int idecs_register (int arg1, int arg2, int irq)
+{
+        hw_regs_t hw;
+        ide_init_hwif_ports(&hw, (ide_ioreg_t) arg1, (ide_ioreg_t) arg2, NULL);
+        hw.irq = irq;
+        hw.chipset = ide_pci; // this enables IRQ sharing w/ PCI irqs
+        return ide_register_hw(&hw, NULL);
+}
+
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;

@@ -326,10 +336,12 @@
 
     /* retry registration in case device is still spinning up */
     for (i = 0; i < 10; i++) {
-       hd = ide_register(io_base, ctl_base, link->irq.AssignedIRQ);
+       if(ctl_base) outb(0x02, ctl_base); // Set nIEN = disable device interrupts
+       hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
        if (hd >= 0) break;
        if (link->io.NumPorts1 == 0x20) {
-           hd = ide_register(io_base+0x10, ctl_base+0x10,
+           if(ctl_base) outb(0x02, ctl_base+0x10);
+           hd = idecs_register(io_base+0x10, ctl_base+0x10,
                              link->irq.AssignedIRQ);
            if (hd >= 0) {
                io_base += 0x10; ctl_base += 0x10;

drivers/ide/ide-probe.c Thu Jun 28 18:43:43 2001 renamed from drivers/ide/ide-probe.c Sun Mar 18 18:25:02 2001 +3 -0

@@ -685,6 +685,9 @@
 #else /* !CONFIG_IDEPCI_SHARE_IRQ */
                int sa = (hwif->chipset == ide_pci) ? SA_INTERRUPT|SA_SHIRQ : SA_INTERRUPT;
 #endif /* CONFIG_IDEPCI_SHARE_IRQ */
+
+               if(hwif->io_ports[IDE_CONTROL_OFFSET])
+                       OUT_BYTE(0x00, hwif->io_ports[IDE_CONTROL_OFFSET]); // clear nIEN == enable irqs
                if (ide_request_irq(hwif->irq, &ide_intr, sa, hwif->name, hwgroup)) {
                        if (!match)
                                kfree(hwgroup);

drivers/ide/ide.c Thu Jun 28 18:04:42 2001 renamed from drivers/ide/ide.c Wed May 2 01:05:00 2001 +1 -0

@@ -2181,6 +2181,7 @@
        memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
        hwif->irq = hw->irq;
        hwif->noprobe = 0;
+       hwif->chipset = hw->chipset;
 
        if (!initializing) {
                ide_probe_module();

include/linux/ide.h Thu Jun 28 18:18:05 2001 renamed from include/linux/ide.h Sat May 26 03:02:42 2001 +14 -11

@@ -226,6 +226,19 @@
 #endif
 
 /*
+ * hwif_chipset_t is used to keep track of the specific hardware
+ * chipset used by each IDE interface, if known.
+ */
+typedef enum {  ide_unknown,    ide_generic,    ide_pci,
+                ide_cmd640,     ide_dtc2278,    ide_ali14xx,
+                ide_qd6580,     ide_umc8672,    ide_ht6560b,
+                ide_pdc4030,    ide_rz1000,     ide_trm290,
+                ide_cmd646,     ide_cy82c693,   ide_4drives,
+                ide_pmac
+} hwif_chipset_t;
+
+
+/*
  * Structure to hold all information about the location of this port
  */
 typedef struct hw_regs_s {

@@ -234,6 +247,7 @@
        int             dma;                    /* our dma entry */
        ide_ack_intr_t  *ack_intr;              /* acknowledge interrupt */
        void            *priv;                  /* interface specific data */
+       hwif_chipset_t  chipset;
 } hw_regs_t;
 
 /*

@@ -396,17 +410,6 @@
 typedef void (ide_maskproc_t) (ide_drive_t *, int);
 typedef void (ide_rw_proc_t) (ide_drive_t *, ide_dma_action_t);
 
-/*
- * hwif_chipset_t is used to keep track of the specific hardware
- * chipset used by each IDE interface, if known.
- */
-typedef enum { ide_unknown,    ide_generic,    ide_pci,
-               ide_cmd640,     ide_dtc2278,    ide_ali14xx,
-               ide_qd6580,     ide_umc8672,    ide_ht6560b,
-               ide_pdc4030,    ide_rz1000,     ide_trm290,
-               ide_cmd646,     ide_cy82c693,   ide_4drives,
-               ide_pmac
-} hwif_chipset_t;
 
 #ifdef CONFIG_BLK_DEV_IDEPCI
 typedef struct ide_pci_devid_s {

That is a legacy bit from ATA-2 but it is one of those things you can not get rid of :-( even thou things are obsoleted, they are not retired. This means that you have to go back into the past to see how it was used, silly! I hope you agree to that point.

This is the drive->ctrl register pointer.

outp(drive->ctl|0x02, IDE_CONTROL_REG);

typedef union {
unsigned all : 8; /* all of the bits together */
struct {
unsigned bit0 : 1;
unsigned nIEN : 1; /* device INTRQ to host */
unsigned SRST : 1; /* host soft reset bit */
unsigned bit3 : 1; /* ATA-2 thingy */
unsigned reserved456 : 3;
unsigned HOB : 1; /* 48-bit address ordering */
} b;
} control_t;

This is a new struct that is to be added for 48-bit addressing and it will reflect drive->ctl soon. I have not decided how to use it best or at all, but it has meaning and once I add-in the real def of bit3 then I will not need to look it up again.

Quoted 5 lines · Andre Hedrick

That is a legacy bit from ATA-2 but it is one of those things you can not get rid of :-( even thou things are obsoleted, they are not retired. This means that you have to go back into the past to see how it was used, silly! I hope you agree to that point.

No,
in ANSI X3.279-1996, "AT Attachment Interface with Extensions (ATA-2)", Approved September 11, 1996 , control register bit 3-7 are reserved.

However ANSI X3.221-1994, "AT Attachment Interface for Disk Drives", Approved May 12, 1994, bit3 is "1" and bits 4-7 are "x". No further explanation.

How far back must we go, to get the sense ?

Quoted 21 lines

This is the drive->ctrl register pointer.

outp(drive->ctl|0x02, IDE_CONTROL_REG);

typedef union {
unsigned all : 8; /* all of the bits together */
struct {
unsigned bit0 : 1;
unsigned nIEN : 1; /* device INTRQ to host */
unsigned SRST : 1; /* host soft reset bit */
unsigned bit3 : 1; /* ATA-2 thingy */
unsigned reserved456 : 3;
unsigned HOB : 1; /* 48-bit address ordering */
} b;
} control_t;

This is a new struct that is to be added for 48-bit addressing and it will reflect drive->ctl soon. I have not decided how to use it best or at all, but it has meaning and once I add-in the real def of bit3 then I will not need to look it up again.

I can not help if you have a device that not compliant to the rules. ATA-2 is OBSOLETED thus we forced (the NCITS Standards Body) the CFA people to move to ATA-4 or ATA-5.

That device is enabling (with its ablity to assert) its device->host interrupt regardless of the HOST...that is a bad device.

Send me the manufacturer and I will tear them apart for making a non-compliant device.

Then figure out a way to de-assert the like regardless if it exists without hang the rest of the driver.

Quoted 40 lines

Hi,

this patch fixes the hard hang (no SYSRQ) on inserting any PCMCIA ATA/IDE card (e.g. CompactFlash, Clik40 etc) to a PCI-Cardbus bridge add-in card.

Thanks David for his valuable explanation about what happens:
ide-probe registers it's irq handler too late! After it triggers the interrupt during the probe the (shared) irq loops forever, effectively wedging the machine completely.

Regards, Gunther

drivers/ide/ide-cs.c Tue Jun 26 21:22:19 2001 renamed from drivers/ide/ide-cs.c Fri Feb 9 20:40:02 2001 +3 -0

@@ -324,6 +324,9 @@
     if (link->io.NumPorts2)
        release_region(link->io.BasePort2, link->io.NumPorts2);
 
+    outb(0x02, ctl_base); // Set nIEN = disable device interrupts
+                         // else it hangs on PCI-Cardbus add-in cards, wedging irq
+
     /* retry registration in case device is still spinning up */
     for (i = 0; i < 10; i++) {
        hd = ide_register(io_base, ctl_base, link->irq.AssignedIRQ);

drivers/ide/ide-probe.c Tue Jun 26 21:25:07 2001 renamed from drivers/ide/ide-probe.c Sun Mar 18 18:25:02 2001 +2 -0

@@ -685,6 +685,8 @@
 #else /* !CONFIG_IDEPCI_SHARE_IRQ */
                int sa = (hwif->chipset == ide_pci) ? SA_INTERRUPT|SA_SHIRQ : SA_INTERRUPT;
 #endif /* CONFIG_IDEPCI_SHARE_IRQ */
+
+               outb(0x00, hwif->io_ports[IDE_CONTROL_OFFSET]); // clear nIEN == enable irqs
                if (ide_request_irq(hwif->irq, &ide_intr, sa, hwif->name, hwgroup)) {
                        if (!match)
                                kfree(hwgroup);

I can not help if you have a device that not compliant to the rules. ATA-2 is OBSOLETED thus we forced (the NCITS Standards Body) the CFA people to move to ATA-4 or ATA-5.

That device is enabling with its ablity to assert its device->host interrupt regardless of the HOST...that is a bad device.

Send me the manufacturer and I will tear them apart for making a non-compliant device. Then figure out a way to de-assert the like regardless if it exists without hang the rest of the driver.

Quoted 11 lines · Andre Hedrick

I can not help if you have a device that not compliant to the rules. ATA-2 is OBSOLETED thus we forced (the NCITS Standards Body) the CFA people to move to ATA-4 or ATA-5.

That device is enabling with its ablity to assert its device->host interrupt regardless of the HOST...that is a bad device.

Send me the manufacturer and I will tear them apart for making a non-compliant device. Then figure out a way to de-assert the like regardless if it exists without hang the rest of the driver.

I don't understand the ATA spec issue, but *every* PCMCIA ATA device I know of (including all SmartMedia, CompactFlash, etc) suffers from this problem. It is not an isolated manufacturer. As far as I know, the IDE driver has always had the problem that it may trigger interrupts before it installs a handler. Are you saying that is only true of pre-ATA-4 devices, or only devices that deviate from the spec?

-- Dave