From: David Milburn <dmilburn@redhat.com>
Date: Tue, 2 Dec 2008 19:13:20 -0600
Subject: [ata] libata: lba_28_ok sector off by one
Message-id: 20081203011320.GA16714@dhcp-210.hsv.redhat.com
O-Subject: [RHEL5.3 PATCH] libata: lba_28_ok sector off by one
Bugzilla: 464868
RH-Acked-by: Alan Cox <alan@redhat.com>
RH-Acked-by: Jeff Garzik <jgarzik@redhat.com>
RH-Acked-by: John Feeney <jfeeney@redhat.com>

According to ATA-7 spec (4.2.1), the maximum user addressable
sector in 28-bit addressing plus one is 0FFFFFFFH. RHEL5 lba-28_ok
is off by one resulting in device errors (ABRT, section 4.2.2).
Customer has verified a 2.6.18-124.el5 test kernel built with this
patch, backport of the following upstream commit. This fixes
BZ 464868.

commit 97b697a11b07e2ebfa69c488132596cc5eb24119
Author: Taisuke Yamada <tai@rakugaki.org>
Date:   Sat Sep 13 16:46:15 2008 -0400

    [libata] LBA28/LBA48 off-by-one bug in ata.h

    I recently bought 3 HGST P7K500-series 500GB SATA drives and
    had trouble accessing the block right on the LBA28-LBA48 border.
    Here's how it fails (same for all 3 drives):

      # dd if=/dev/sdc bs=512 count=1 skip=268435455 > /dev/null
      dd: reading `/dev/sdc': Input/output error
      0+0 records in
      0+0 records out
      0 bytes (0 B) copied, 0.288033 seconds, 0.0 kB/s
      # dmesg
      ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x0
      ata1.00: BMDMA stat 0x25
      ata1.00: cmd c8/00:08:f8:ff:ff/00:00:00:00:00/ef tag 0 dma 4096 in
      res 51/04:08:f8:ff:ff/00:00:00:00:00/ef Emask 0x1 (device error)
      ata1.00: status: { DRDY ERR }
      ata1.00: error: { ABRT }
      ata1.00: configured for UDMA/33
      ata1: EH complete
      ...

    After some investigations, it turned out this seems to be caused
    by misinterpretation of the ATA specification on LBA28 access.
    Following part is the code in question:

      === include/linux/ata.h ===
      static inline int lba_28_ok(u64 block, u32 n_block)
      {
        /* check the ending block number */
        return ((block + n_block - 1) < ((u64)1 << 28)) && (n_block <= 256);
      }

    HGST drive (sometimes) fails with LBA28 access of {block = 0xfffffff,
    n_block = 1}, and this behavior seems to be comformant. Other drives,
    including other HGST drives are not that strict, through.

    >From the ATA specification:
    (http://www.t13.org/Documents/UploadedDocuments/project/d1410r3b-ATA-ATAPI-6

      8.15.29  Word (61:60): Total number of user addressable sectors
      This field contains a value that is one greater than the total number
      of user addressable sectors (see 6.2). The maximum value that shall
      be placed in this field is 0FFFFFFFh.

    So the driver shouldn't use the value of 0xfffffff for LBA28 request
    as this exceeds maximum user addressable sector. The logical maximum
    value for LBA28 is 0xffffffe.

    The obvious fix is to cut "- 1" part, and the patch attached just do
    that. I've been using the patched kernel for about a month now, and
    the same fix is also floating on the net for some time. So I believe
    this fix works reliably.

    Just FYI, many Windows/Intel platform users also seems to be struck
    by this, and HGST has issued a note pointing to Intel ICH8/9 driver.

      "28-bit LBA command is being used to access LBAs 29-bits in length"
    http://www.hitachigst.com/hddt/knowtree.nsf/cffe836ed7c12018862565b000530c74

    Also, *BSDs seems to have similar fix included sometime around ~2004,
    through I have not checked out exact portion of the code.

    Signed-off-by: Taisuke Yamada <tai@rakugaki.org>
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

Please review and ack.

Thanks,
David

 include/linux/ata.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/ata.h b/include/linux/ata.h
index 1c622e2..636589b 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -682,7 +682,7 @@ static inline int ata_ok(u8 status)
 static inline int lba_28_ok(u64 block, u32 n_block)
 {
 	/* check the ending block number */
-	return ((block + n_block - 1) < ((u64)1 << 28)) && (n_block <= 256);
+	return ((block + n_block) < ((u64)1 << 28)) && (n_block <= 256);
 }
 
 static inline int lba_48_ok(u64 block, u32 n_block)