Re: [PATCH] (IDE) restore access to low order LBA following error

Re: [PATCH] (IDE) restore access to low order LBA following error

[Alan Cox]

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.

[PATCH] (IDE) restore access to low order LBA following error

[Brett Russ]

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============================================
--- linux-2.4.26/drivers/ide/ide-disk.c Fri Nov 28 13:26:20 2003
+++ linux/drivers/ide/ide-disk.c        Thu Aug  5 11:25:58 2004
@@ -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,
--- linux-2.4.26/drivers/ide/ide.c      Wed Feb 18 08:36:31 2004
+++ linux/drivers/ide/ide.c     Thu Aug  5 11:37:04 2004
@@ -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",
--- linux-2.4.26/drivers/ide/ide-taskfile.c     Fri Jun 13 10:51:33 2003
+++ linux/drivers/ide/ide-taskfile.c    Thu Aug  5 11:27:59 2004
@@ -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============================================
--- linux-2.6.7/drivers/ide/ide.c       Wed Jun 16 01:19:03 2004
+++ linux/drivers/ide/ide.c     Thu Aug  5 11:09:25 2004
@@ -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",
--- linux-2.6.7/drivers/ide/ide-disk.c  Wed Jun 16 01:19:17 2004
+++ linux/drivers/ide/ide-disk.c        Thu Aug  5 11:10:28 2004
@@ -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,

Re: [PATCH] (IDE) restore access to low order LBA following error

[Brett Russ]

Alan Cox wrote:
> 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

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============================================
--- linux-2.6.8-rc3/drivers/ide/ide-io.c        Tue Aug  3 17:28:20 2004
+++ linux/drivers/ide/ide-io.c  Thu Aug  5 16:28:38 2004
@@ -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============================================
--- linux-2.4.27-rc5/drivers/ide/ide-io.c       Fri Nov 28 13:26:20 2003
+++ linux/drivers/ide/ide-io.c  Thu Aug  5 16:33:15 2004
@@ -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);

Re: [PATCH] (IDE) restore access to low order LBA following error

[Jeff Garzik]

Alan Cox wrote:
> 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.


BTW MWDMA is already done and checked in :)

Re: [PATCH] (IDE) restore access to low order LBA following error

[Alan Cox]

On Gwe, 2004-08-06 at 00:13, Jeff Garzik wrote:
> Alan Cox wrote:
> > 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.
> 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!

Re: [PATCH] (IDE) restore access to low order LBA following error

[Jeff Garzik]

Alan Cox wrote:
> On Gwe, 2004-08-06 at 00:13, Jeff Garzik wrote:
> 
>>Alan Cox wrote:
>>
>>>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.
>>
>>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

Re: [PATCH] (IDE) restore access to low order LBA following error

[Doug Maxey]

On Thu, 05 Aug 2004 21:45:59 EDT, Jeff Garzik wrote:
>ATAPI works too.....  assuming your CD/DVD drive never encounters a 
>CHECK CONDITION requiring REQUEST SENSE to be issued...  ;-)

  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

Re: [PATCH] (IDE) restore access to low order LBA following error

[Jeff Garzik]

Doug Maxey wrote:
> On Thu, 05 Aug 2004 21:45:59 EDT, Jeff Garzik wrote:
> 
>>ATAPI works too.....  assuming your CD/DVD drive never encounters a 
>>CHECK CONDITION requiring REQUEST SENSE to be issued...  ;-)
> 
> 
>   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.

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 */