Crash in IDE code

Crash in IDE code

[Michel Lanners]

Hi all,

My current devel kernel (Paul's rsync, 2.4.0-test1-ac21) crashes with my
Promise U/66 when trying to read the partition table:

hda: Maxtor 91024D4, ATA DISK drive
ide0 at 0xf77fe470-0xf77fe477,0x000 on irq 23
hda: UDMA 2 drive0 0x004ff3f4 0x004123f4
hda: 19746720 sectors (10110 MB) w/512KiB Cache, CHS=19590/16/63, UDMA(33)
Partition check:
 hda:NIP: C00B8C4C XER: C000BE6F LR: C00B8BB4 REGS: c01a7d90 TRAP: 0300
MSR: 00001032 EE: 0 PR: 0 FP: 0 ME: 1 IR/DR: 11
TASK = c01a6000[0] 'swapper' Last syscall: 120
last math 00000000 last altivec 00000000
GPR00: 00000000 C01A7E40 C01A6000 00000000 00001032 C01A7EC0 00800000 00000000
GPR08: C022E860 C023B02C C01AA410 00000000 95939E39 DEADBEEF 003F0000 DEADBEEF
GPR16: 00000000 DEADBEEF DEADBEEF DEADBEEF 00001032 001A7EB0 00000000 C0004B84
GPR24: C00058A4 0000FFFE 40000000 C023B02C C01A53E0 C00C34A4 C05FC4A0 C023B094
Call backtrace:
C0034480 C00057D4 C0005928 C0004B84 0000FFFE C0005E20 C0005E38
C01C27D8 000036C0
Kernel panic: kernel access of bad area pc c00b8c4c lr c00b8bb4 address 0 tsk swapper/0
In interrupt handler - not syncing

I've taken the time to disassemble the code part where it crashes. It's
in drivers/ide/ide.c, in this code inlined from ide_intr():

static inline int drive_is_ready (ide_drive_t *drive)
{
        if (drive->waiting_for_dma)
                return HWIF(drive)->dmaproc(ide_dma_test_irq, drive);
#if 0
        udelay(1);      /* need to guarantee 400ns since last command was issued */
#endif
//      if (GET_STAT() & BUSY_STAT)     /* Note: this may clear a pending IRQ!! */
        if (IN_BYTE(IDE_ALTSTATUS_REG) & BUSY_STAT)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^

                return 0;       /* drive busy:  definitely not interrupting */
        return 1;               /* drive ready: *might* be interrupting */
}

The crashing part is marked above; it's the IN_BYTE which tries to read
from addr. 0 :-(

Above code translates to this:

IN_BYTE( (((ide_hwif_t *)((drive)->hwif))->io_ports[IDE_CONTROL_OFFSET]) )

So, io_ports[IDE_CONTROL_OFFSET] is obviously zero; it's suposed to be
in GPR11 in the oops above. One point where it is explicitly set to zero
is in ide_pmac.c:

        if (ix >= MAX_HWIFS) {
                /* Probably a PCI interface... */
                for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
                        hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
                /* XXX is this right? */
                hw->io_ports[IDE_CONTROL_OFFSET] = 0;

So, question is: is it right to zero hw->io_ports[IDE_CONTROL_OFFSET]?
Should I compile without PMac IDE support, since my box has no 'native'
IDE? Or should the test above in drive_is_ready() be protected against a
NULL pointer, like most other occurences are?


Thanks for any help


Michel

-------------------------------------------------------------------------
Michel Lanners                 |  " Read Philosophy.  Study Art.
23, Rue Paul Henkes            |    Ask Questions.  Make Mistakes.
L-1710 Luxembourg              |
email   mlan@cpu.lu            |
http://www.cpu.lu/~mlan        |                     Learn Always. "

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Crash in IDE code

[Geert Uytterhoeven]

On Sat, 8 Jul 2000, Michel Lanners wrote:
> I've taken the time to disassemble the code part where it crashes. It's
> in drivers/ide/ide.c, in this code inlined from ide_intr():
>
> static inline int drive_is_ready (ide_drive_t *drive)
> {
>         if (drive->waiting_for_dma)
>                 return HWIF(drive)->dmaproc(ide_dma_test_irq, drive);
> #if 0
>         udelay(1);      /* need to guarantee 400ns since last command was issued */
> #endif
> //      if (GET_STAT() & BUSY_STAT)     /* Note: this may clear a pending IRQ!! */
>         if (IN_BYTE(IDE_ALTSTATUS_REG) & BUSY_STAT)
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>                 return 0;       /* drive busy:  definitely not interrupting */
>         return 1;               /* drive ready: *might* be interrupting */
> }
>
> The crashing part is marked above; it's the IN_BYTE which tries to read
> from addr. 0 :-(
>
> Above code translates to this:
>
> IN_BYTE( (((ide_hwif_t *)((drive)->hwif))->io_ports[IDE_CONTROL_OFFSET]) )

Yep, ALTSTATUS == CONTROL

> So, io_ports[IDE_CONTROL_OFFSET] is obviously zero; it's suposed to be
> in GPR11 in the oops above. One point where it is explicitly set to zero
> is in ide_pmac.c:
>
>         if (ix >= MAX_HWIFS) {
>                 /* Probably a PCI interface... */
>                 for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
>                         hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
>                 /* XXX is this right? */
>                 hw->io_ports[IDE_CONTROL_OFFSET] = 0;
>
> So, question is: is it right to zero hw->io_ports[IDE_CONTROL_OFFSET]?
> Should I compile without PMac IDE support, since my box has no 'native'
> IDE? Or should the test above in drive_is_ready() be protected against a
> NULL pointer, like most other occurences are?

It's indeed true that some hardware doesn't have the CONTROL/ALTSTATUS
register. I don't know about the PowerMac, but I know about the Amiga
IDE-doublers[*]. So there _must_ be a test for a non-zero register offset
prior to the usage of CONTROL/ALTSTATUS. Hence this is a bug.

Gr{oetje,eeting}s,

						Geert

[*] IDE-doublers are actually something that can be used on most IDE
    interfaces. IDE defines 2 banks of 8 registers to access the drive. From
    the second bank, only one register is actually used (CONTROL/ALTSTATUS).
    Since CONTROL/ALTSTATUS is not vital to the functionality of the IDE
    interface, the second bank of registers can be sacrificed and one IDE chain
    that supports 2 devices can be `split' into 2 chains supporting 2x2 devices
    using only a few diodes (schematic available on request). Then accessing
    the first bank of registers will still access the first bank of registers
    on the first chain, while accessing the second bank of registers will
    access the first bank of registers on the second chain.
    Driver: see the IDE-doubler option in drivers/ide/gayle.c.
--
Geert Uytterhoeven -- Linux/{m68k~Amiga,PPC~CHRP} -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Crash in IDE code

[Forwarded SuSE Mail]

Thanks Geert,

We both know that I should have known better.
Regardless if PPC has an issue, you will.
This I need to flag this better.

This adds a whole new wrinkle in the total rework of the TASK commands.

Cheers,

Andre Hedrick
The Linux ATA/IDE guy

On 8 Jul 2000, Geert Uytterhoeven wrote:

> On Sat, 8 Jul 2000, Michel Lanners wrote:
> > I've taken the time to disassemble the code part where it crashes. It's
> > in drivers/ide/ide.c, in this code inlined from ide_intr():
> >
> > static inline int drive_is_ready (ide_drive_t *drive)
> > {
> >         if (drive->waiting_for_dma)
> >                 return HWIF(drive)->dmaproc(ide_dma_test_irq, drive);
> > #if 0
> >         udelay(1);      /* need to guarantee 400ns since last command was issued */
> > #endif
> > //      if (GET_STAT() & BUSY_STAT)     /* Note: this may clear a pending IRQ!! */
> >         if (IN_BYTE(IDE_ALTSTATUS_REG) & BUSY_STAT)
> >             ^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >                 return 0;       /* drive busy:  definitely not interrupting */
> >         return 1;               /* drive ready: *might* be interrupting */
> > }
> >
> > The crashing part is marked above; it's the IN_BYTE which tries to read
> > from addr. 0 :-(
> >
> > Above code translates to this:
> >
> > IN_BYTE( (((ide_hwif_t *)((drive)->hwif))->io_ports[IDE_CONTROL_OFFSET]) )
>
> Yep, ALTSTATUS == CONTROL
>
> > So, io_ports[IDE_CONTROL_OFFSET] is obviously zero; it's suposed to be
> > in GPR11 in the oops above. One point where it is explicitly set to zero
> > is in ide_pmac.c:
> >
> >         if (ix >= MAX_HWIFS) {
> >                 /* Probably a PCI interface... */
> >                 for (i = IDE_DATA_OFFSET; i <= IDE_STATUS_OFFSET; ++i)
> >                         hw->io_ports[i] = data_port + i - IDE_DATA_OFFSET;
> >                 /* XXX is this right? */
> >                 hw->io_ports[IDE_CONTROL_OFFSET] = 0;
> >
> > So, question is: is it right to zero hw->io_ports[IDE_CONTROL_OFFSET]?
> > Should I compile without PMac IDE support, since my box has no 'native'
> > IDE? Or should the test above in drive_is_ready() be protected against a
> > NULL pointer, like most other occurences are?
>
> It's indeed true that some hardware doesn't have the CONTROL/ALTSTATUS
> register. I don't know about the PowerMac, but I know about the Amiga
> IDE-doublers[*]. So there _must_ be a test for a non-zero register offset
> prior to the usage of CONTROL/ALTSTATUS. Hence this is a bug.
>
> Gr{oetje,eeting}s,
>
> 						Geert
>
> [*] IDE-doublers are actually something that can be used on most IDE
>     interfaces. IDE defines 2 banks of 8 registers to access the drive. From
>     the second bank, only one register is actually used (CONTROL/ALTSTATUS).
>     Since CONTROL/ALTSTATUS is not vital to the functionality of the IDE
>     interface, the second bank of registers can be sacrificed and one IDE chain
>     that supports 2 devices can be `split' into 2 chains supporting 2x2 devices
>     using only a few diodes (schematic available on request). Then accessing
>     the first bank of registers will still access the first bank of registers
>     on the first chain, while accessing the second bank of registers will
>     access the first bank of registers on the second chain.
>     Driver: see the IDE-doubler option in drivers/ide/gayle.c.
> --
> Geert Uytterhoeven -- Linux/{m68k~Amiga,PPC~CHRP} -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> 							    -- Linus Torvalds
>

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: Crash in IDE code

[Forwarded SuSE Mail]

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


Apply this against any 2.4.0-test3 and it should go.

If this fails then holler.

Cheers,

Andre Hedrick
The Linux ATA/IDE guy


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

diff -urN linux-2.4.0-t3-6/drivers/ide/ide.c linux-2.4.0-t3-6.1/drivers/ide/ide.c
--- linux-2.4.0-t3-6/drivers/ide/ide.c	Sat Jul  8 02:01:18 2000
+++ linux-2.4.0-t3-6.1/drivers/ide/ide.c	Sat Jul  8 22:42:09 2000
@@ -481,13 +481,19 @@
  */
 static inline int drive_is_ready (ide_drive_t *drive)
 {
+	byte stat = 0;
 	if (drive->waiting_for_dma)
 		return HWIF(drive)->dmaproc(ide_dma_test_irq, drive);
 #if 0
 	udelay(1);	/* need to guarantee 400ns since last command was issued */
 #endif
-//	if (GET_STAT() & BUSY_STAT)	/* Note: this may clear a pending IRQ!! */
-	if (IN_BYTE(IDE_ALTSTATUS_REG) & BUSY_STAT)
+
+	if (IDE_CONTROL_REG)
+		stat = GET_ALTSTAT();
+	else
+		stat = GET_STAT();	/* Note: this may clear a pending IRQ!! */
+
+	if (stat & BUSY_STAT)
 		return 0;	/* drive busy:  definitely not interrupting */
 	return 1;		/* drive ready: *might* be interrupting */
 }
diff -urN linux-2.4.0-t3-6/include/linux/ide.h linux-2.4.0-t3-6.1/include/linux/ide.h
--- linux-2.4.0-t3-6/include/linux/ide.h	Sat Jul  8 02:01:29 2000
+++ linux-2.4.0-t3-6.1/include/linux/ide.h	Sat Jul  8 22:53:04 2000
@@ -162,6 +162,7 @@
 
 #define GET_ERR()		IN_BYTE(IDE_ERROR_REG)
 #define GET_STAT()		IN_BYTE(IDE_STATUS_REG)
+#define GET_ALTSTAT()		IN_BYTE(IDE_CONTROL_REG)
 #define OK_STAT(stat,good,bad)	(((stat)&((good)|(bad)))==(good))
 #define BAD_R_STAT		(BUSY_STAT   | ERR_STAT)
 #define BAD_W_STAT		(BAD_R_STAT  | WRERR_STAT)

Re: Crash in IDE code

[Forwarded SuSE Mail]

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


Slap this on to test3-pre7 and flame me later if it does not fix the
issues.

Cheers,



Andre Hedrick
The Linux ATA/IDE guy


[-- Attachment #2: Type: application/octet-stream, Size: 21265 bytes --]

Re: Crash in IDE code

[Henning Loeser]

Hi all,
I applied Andre's Patch to 2.4.0-test2-ac2 and there we go my ACARD
6260M ide card does work. Thanks a lot to everyone!
I hope this fixes things for other IDE-cards aswell.

Cheers
Henning

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/