(IDE) restore access to low order LBA following error

I've been playing with code which needs access to the drive's reported error LBA following a drive detected sector error. Currently, as far as I can tell, the only code which uses this information are the *_dump_status routines (ide_, idedisk_, and (only in 2.4) taskfile_) which put the error info in the syslog. Anyone else who wants to read the LBA that the error occurred on will be accessing the drive registers after these functions have run.

The problem is, these routines assume no one else looks at the registers and leaves them pointed to the high 24 bits of the 48 bit address. I think the kernel should leave the registers in a known state.

The following patches restore the drive register access to a known state (the low order bits of the LBA) following status dumps. The patches are based on 2.4.26 and 2.6.7 respectively and apply cleanly to 2.4.27-rc5 and 2.6.8-rc3.

I previously sent a version of this patch to the IDE list, please ignore that one as it was rushed, incomplete, and had a mistake.

I considered several alternatives to the patch I'm submitting:
1) whether ide_read_24 should take an argument (low/high) that would access the desired bits and clean up after itself
2) whether all readers of the LBA register should point to the desired bits before reading the LBA

...but I think this patched implementation may be more acceptable. If not, please advise and I will code either alternative.

I have one question on the ide-taskfile.c patch for 2.4 below: should drive->ctl be OR'ed with 0x80 when setting the high order LBA access bit? As you can see, it wasn't previously but this seems at risk of losing the nIEN bit. It may be a non-issue though because I don't think taskfile_dump_status is used.

Thank you!
Brett

2.4============================================

@@ -857,6 +857,9 @@
                                low = idedisk_read_24(drive);
                                hwif->OUTB(drive->ctl|0x80, 

IDE_CONTROL_REG);
                                high = idedisk_read_24(drive);
+                               /* Restore access to low order LBA */
+                               hwif->OUTB(drive->ctl & ~0x80,
+                                          IDE_CONTROL_REG);
                                sectors = ((__u64)high << 24) | low;
                                printk(", LBAsect=%llu, high=%d, low=%d",
                                       (unsigned long long) sectors,

@@ -407,8 +407,12 @@
                                        u64 sectors = 0;
                                        u32 high = 0;
                                        u32 low = read_24(drive);
-                                       hwif->OUTB(drive->ctl|0x80, IDE_CONTROL_REG);
+                                       hwif->OUTB(drive->ctl | 0x80,
+                                                  IDE_CONTROL_REG);
                                        high = read_24(drive);
+                                       /* Restore access to low order LBA */
+                                       hwif->OUTB(drive->ctl & ~0x80,
+                                                  IDE_CONTROL_REG);
 
                                        sectors = ((u64)high << 24) | low;
                                        printk(", LBAsect=%llu, 

high=%d, low=%d",

@@ -308,8 +308,11 @@
                                u64 sectors = 0;
                                u32 high = 0;
                                u32 low = task_read_24(drive);
-                               hwif->OUTB(0x80, IDE_CONTROL_REG);
+                               hwif->OUTB(drive->ctl | 0x80, IDE_CONTROL_REG);
                                high = task_read_24(drive);
+                               /* Restore access to low order LBA */
+                               hwif->OUTB(drive->ctl & ~0x80,
+                                          IDE_CONTROL_REG);
                                sectors = ((u64)high << 24) | low;
                                printk(", LBAsect=%lld", sectors);
                        } else {

2.6============================================

@@ -409,6 +409,9 @@
                                        u32 low = ide_read_24(drive);
                                        hwif->OUTB(drive->ctl|0x80, 

IDE_CONTROL_REG);
                                        high = ide_read_24(drive);

+ /* Restore access to low order LBA */
+ hwif->OUTB(drive->ctl & ~0x80,
+ IDE_CONTROL_REG);

                                        sectors = ((u64)high << 24) | low;
                                        printk(", LBAsect=%llu, 

high=%d, low=%d",

@@ -675,6 +675,9 @@
                                low = ide_read_24(drive);
                                hwif->OUTB(drive->ctl|0x80, 

IDE_CONTROL_REG);
                                high = ide_read_24(drive);
+                               /* Restore access to low order LBA */
+                               hwif->OUTB(drive->ctl & ~0x80,
+                                          IDE_CONTROL_REG);
                                sectors = ((__u64)high << 24) | low;
                                printk(", LBAsect=%llu, high=%d, low=%d",
                                       (unsigned long long) sectors,

I would make sure you are looking at the right register set in your own code. Any code now thats going to exist for most 2.4 users will need to do that itself, and after my 2.4 experience with the IDE core I'd advise anyone working on it to

a) Pass it off to another maintainer as fast as possible ;)
b) Program defensively

Once Jeff has MWDMA and ATAPI in the new SATA/ATA driver he wrote then migration might be an even better option. It'll certainly be easier to do a lot of things right with the newest SATA code than with the current IDE layer.

Thanks for the reply Alan,

That probably is the best idea, rather than rely upon the code to do the right thing in all places. That being said, I think the following code needs to be added to ide_end_drive_cmd(), since ide_error() calls ide_end_drive_cmd() after ide_dump_status() has left the registers pointing at the high order part of the LBA. Using the taskfile ioctl will then lead to hobRegister[] and subsequently hob_ports[] being loaded with the wrong values.

BR

2.6============================================

@@ -197,6 +197,8 @@
                                args->hobRegister[IDE_DATA_OFFSET] 
 = (data >> 8) & 0xFF;
                        }
                        args->tfRegister[IDE_ERROR_OFFSET]   = err;
+                       /* Be sure we're looking at the low order bits */
+                       hwif->OUTB(drive->ctl & ~0x80,IDE_CONTROL_REG);
                        args->tfRegister[IDE_NSECTOR_OFFSET] = 

hwif->INB(IDE_NSECTOR_REG);
                        args->tfRegister[IDE_SECTOR_OFFSET]  = 
hwif->INB(IDE_SECTOR_REG);
                        args->tfRegister[IDE_LCYL_OFFSET]    = 

hwif->INB(IDE_LCYL_REG);

2.4============================================

@@ -165,6 +165,9 @@
 

args->hobRegister[IDE_DATA_OFFSET_HOB] = (data >> 8) & 0xFF;
                                }
                                args->tfRegister[IDE_ERROR_OFFSET]   = err;
+                               /* Be sure we're looking at the low order bits
+                                */
+                               hwif->OUTB(drive->ctl & ~0x80,IDE_CONTROL_REG);
                                args->tfRegister[IDE_NSECTOR_OFFSET] = 
hwif->INB(IDE_NSECTOR_REG);
                                args->tfRegister[IDE_SECTOR_OFFSET]  = 
hwif->INB(IDE_SECTOR_REG);
                                args->tfRegister[IDE_LCYL_OFFSET]    = 

hwif->INB(IDE_LCYL_REG);

BTW MWDMA is already done and checked in :)

Do ATAPI and I'll be glad to help move the other drivers over. I need hotplug for my PIIX controller and SI680!

ATAPI works too..... assuming your CD/DVD drive never encounters a CHECK CONDITION requiring REQUEST SENSE to be issued... ;-)

Jeff

Heh. Where should one start looking to get this enabled? I have to admit that I have given the code only a few minutes viewing.

++doug

top of include/linux/libata.h:

/*
* compile-time options
*/

#undef ATA_FORCE_PIO            /* do not configure or use DMA */
#undef ATA_DEBUG                /* debugging output */
#undef ATA_VERBOSE_DEBUG        /* yet more debugging output */
#undef ATA_IRQ_TRAP             /* define to ack screaming irqs */
#undef ATA_NDEBUG               /* define to disable quick runtime checks */
#undef ATA_ENABLE_ATAPI         /* define to enable ATAPI support */
#undef ATA_ENABLE_PATA          /* define to enable PATA support in some

* low-level drivers */
#undef ATAPI_ENABLE_DMADIR /* enables ATAPI DMADIR bridge support */