libata: bump transfer chunk size if it's odd

None of the drives I have follows what the standard says about
transfer chunk size.  Of the four SATA and six PATA ATAPI devices
tested, four ignore transfer chunk size completely and the ones which
honor it don't behave according to the spec when it's odd.

According to the spec, transfer chunk size can be odd if the amount of
data to transfer equals or is smaller than the chunk size and the
device can indicate the same odd number and transfer the whole thing
at one go with a pad byte appended.  However, in reality, none of the
drives I have does that.  They all indicate and transfer even number
of bytes one byte shorter than the chunk size first; then indicate and
transfer two bytes, which is clearly out of spec.

In addition to unnecessary second PIO data phase, this also creates a
weird problem when combined with SATA controllers which perform PIO
via DMA.  Some of these controllers use actualy number of bytes
received to update DMA pointer so chunks which are sized 4n + 2 makes
DMA pointer off by two bytes.  This causes data corruption and buffer
overruns.

This patch rounds nbytes up to the nearest even number such that ATAPI
devices don't split data transfer for the last odd byte.  This
shouldn't confuse controllers which depend on transfer chunk size as
devices will report the rounded-up number, actually transfer that much
and padding buffer is there to receive them.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Signed-off-by: Jeff Garzik <jeff@garzik.org>

1 files changed, 32 insertions, 3 deletions

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a45f6ac3b245..a883bb03d4c7 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2485,11 +2485,40 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
         if (!using_pio && ata_check_atapi_dma(qc))
                 using_pio = 1;
 
-        /* Some controller variants snoop this value for Packet transfers
-           to do state machine and FIFO management. Thus we want to set it
-           properly, and for DMA where it is effectively meaningless */
+        /* Some controller variants snoop this value for Packet
+         * transfers to do state machine and FIFO management.  Thus we
+         * want to set it properly, and for DMA where it is
+         * effectively meaningless.
+         */
         nbytes = min(qc->nbytes, (unsigned int)63 * 1024);
 
+        /* Most ATAPI devices which honor transfer chunk size don't
+         * behave according to the spec when odd chunk size which
+         * matches the transfer length is specified.  If the number of
+         * bytes to transfer is 2n+1.  According to the spec, what
+         * should happen is to indicate that 2n+1 is going to be
+         * transferred and transfer 2n+2 bytes where the last byte is
+         * padding.
+         *
+         * In practice, this doesn't happen.  ATAPI devices first
+         * indicate and transfer 2n bytes and then indicate and
+         * transfer 2 bytes where the last byte is padding.
+         *
+         * This inconsistency confuses several controllers which
+         * perform PIO using DMA such as Intel AHCIs and sil3124/32.
+         * These controllers use actual number of transferred bytes to
+         * update DMA poitner and transfer of 4n+2 bytes make those
+         * controller push DMA pointer by 4n+4 bytes because SATA data
+         * FISes are aligned to 4 bytes.  This causes data corruption
+         * and buffer overrun.
+         *
+         * Always setting nbytes to even number solves this problem
+         * because then ATAPI devices don't have to split data at 2n
+         * boundaries.
+         */
+        if (nbytes & 0x1)
+                nbytes++;
+
         qc->tf.lbam = (nbytes & 0xFF);
         qc->tf.lbah = (nbytes >> 8);