Resubmit ATAPI PIO mode fixes

Hi Jeff,

Problem:
Incorrect md5sum when using ATAPI PIO mode to verify a distro CD.

Root cause:
sg traverse problem.
In __atapi_pio_bytes(), if qc->cursg++ is increased and "goto next_page" is executed, then sg is not updated to the new qc->cursg and the old sg is overwritten with the new data.

Changes:
- Replace "goto next_page" with "goto next_sg" to make sg updated.

Attached please find the patch against the linux-2.6.git tree for your review. Thanks.

Albert

Signed-off-by Albert Lee <[email protected]>

drivers/scsi/libata-core.c 2005-06-06 13:45:33.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-06-06 13:37:50.000000000 +0800 +1 -1

@@ -2577,7 +2577,6 @@
 next_sg:
        sg = &qc->sg[qc->cursg];
 
-next_page:
        page = sg->page;
        offset = sg->offset + qc->cursg_ofs;
 

@@ -2585,6 +2584,7 @@
        page = nth_page(page, (offset >> PAGE_SHIFT));
        offset %= PAGE_SIZE;
 
+       /* don't overrun current sg */
        count = min(sg->length - qc->cursg_ofs, bytes);
 
        /* don't cross page boundaries */

Hi Jeff,

Problem:

   In __atapi_pio_bytes(), when (bytes > qc->nbytes) or (qc->cursg_ofs > sg->length) or

(count > bytes), the if condition is not handled properly.

Changes:
- Fix the "if conditions" to make the "if conditions" more robust.

Attached please find the patch against the linux-2.6.git tree for your review. Thanks.

Albert

Signed-off-by Albert Lee <[email protected]>

drivers/scsi/libata-core.c 2005-06-06 13:52:11.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-06-06 13:45:33.000000000 +0800 +5 -4

@@ -2571,7 +2571,7 @@
        unsigned char *buf;
        unsigned int offset, count;
 
-       if (qc->curbytes == qc->nbytes - bytes)
+       if (qc->curbytes + bytes >= qc->nbytes)
                ap->pio_task_state = PIO_ST_LAST;
 
 next_sg:

@@ -2592,11 +2592,10 @@
 
        buf = kmap(page) + offset;
 
-       bytes -= count;
        qc->curbytes += count;
        qc->cursg_ofs += count;
 
-       if (qc->cursg_ofs == sg->length) {
+       if (qc->cursg_ofs >= sg->length) {
                qc->cursg++;
                qc->cursg_ofs = 0;
        }

@@ -2608,7 +2607,9 @@
 
        kunmap(page);
 
-       if (bytes) {
+       if (bytes > count) {
+               bytes -= count;
+
                goto next_sg;
        }
 }

Hi Jeff,

Problem:
Sometimes CD-ROM drives will ask/return more data than expected.

Changes:
- Handle the trailing data if the end of qc->sg has been reached.

Attached please find the patch against the linux-2.6.git tree for your review. Thanks.

Albert

Signed-off-by Albert Lee <[email protected]>

drivers/scsi/libata-core.c 2005-06-06 13:57:18.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-06-06 13:52:11.000000000 +0800 +10 -0

@@ -2575,6 +2575,23 @@
                ap->pio_task_state = PIO_ST_LAST;
 
 next_sg:
+       /* check whether qc->sg is full */
+       if (unlikely(qc->cursg >= qc->n_elem)) {
+               unsigned char pad_buf[2];
+               unsigned int words = (bytes+1) >> 1; /* pad to word boundary */
+               unsigned int i;
+
+               DPRINTK("ata%u: padding %u bytes\n", ap->id, bytes);
+
+               memset(&pad_buf, 0, sizeof(pad_buf));
+               for (i = 0; i < words; i++) {

Hi Jeff,

Problem:
- When odd length data is transfered, the last byte of data is lost.

Changes:
- When an odd length data transfer is seen, round it to the next even length. (The same as atapi_input_bytes() does in the ide-iops.)

Attached please find the patch against the linux-2.6.git tree for your review. Thanks.

Albert

Signed-off-by Albert Lee <[email protected]>

drivers/scsi/libata-core.c 2005-06-06 14:54:36.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-06-06 13:57:18.000000000 +0800 +0 -0

@@ -2607,6 +2607,13 @@
        /* don't cross page boundaries */
        count = min(count, (unsigned int)PAGE_SIZE - offset);
 

(bytes > qc->nbytes) condition can happen and this part of the patch is fine with me
but I fail to see how (qc->cursg_ofs > sg->length) or (count > bytes) can happen...

	count = min(sg->length - qc->cursg_ofs, bytes);

	/* don't cross page boundaries */
	count = min(count, (unsigned int)PAGE_SIZE - offset);

Bartlomiej

Hi Bart:

Quoted 12 lines

(bytes > qc->nbytes) condition can happen and this part of the patch is fine with me
but I fail to see how (qc->cursg_ofs > sg->length) or (count > bytes) can happen...

      count = min(sg->length - qc->cursg_ofs, bytes);

      /* don't cross page boundaries */
      count = min(count, (unsigned int)PAGE_SIZE - offset);

Quoted 3 lines

Bartlomiej

Normally (qc->cursg_ofs > sg->length) or (count > bytes) won't happen. However, if we apply patch [4/4], which overrun the odd-length buffer by one byte, then (qc->cursg_ofs > sg->length) and (count > bytes) could happen.

Quoted 4 lines

Normally (qc->cursg_ofs > sg->length) or (count > bytes) won't happen. However, if we apply patch [4/4], which overrun the odd-length buffer by one byte, then (qc->cursg_ofs > sg->length) and (count > bytes) could happen.

If so wouldn't it be easier to round 'bytes' instead of 'count' in the patch #4?

* this patch (#2) gets simpler
* no need to round 'bytes' in the patch #3
* rounding can be moved from "sg loop" to the beginning of __atapi_pio_bytes() in the patch #4

Hi Bart:

Quoted 6 lines

Quoted 4 lines

Normally (qc->cursg_ofs > sg->length) or (count > bytes) won't happen. However, if we apply patch [4/4], which overrun the odd-length buffer by one byte, then (qc->cursg_ofs > sg->length) and (count > bytes) could happen.

Quoted 7 lines

If so wouldn't it be easier to round 'bytes' instead of 'count' in the patch #4?

* this patch (#2) gets simpler
* no need to round 'bytes' in the patch #3
* rounding can be moved from "sg loop" to the beginning of __atapi_pio_bytes() in the patch #4

ata_data_xfer() also does rounding: 'unsigned int words = buflen >> 1;'
If we only round 'bytes' and moves rounding from "sg loop" to the beginning of __atapi_pio_bytes(), then the last 1 bytes of odd length buffer will still be lost.

Ex. say 'sg->length' is 51, 'qc->cursg_ofs' is 0 and 'bytes' is 52 (rounded), after the min()

         /* don't overrun current sg */
        count = min(sg->length - qc->cursg_ofs, bytes);

      'count' will be 51, and ata_data_xfer() will only transfer 50 bytes.

52 - 51 = 1 (1 byte trailing) So the trailing handling code will be invoked and eat the 51th and 52th byte.
The 51th byte is lost. :(

Maybe we can round both 'sg->length' and 'bytes'?
Ex.

        bytes += bytes & 0x01;
         ....
loop:
        ....
        /* don't overrun current sg */
        sg->length += (sg->length) & 0x01;  // Round sg->length
        count = min(sg->length - qc->cursg_ofs, bytes);
      => both 'count' and 'byte' will be 52 here

   However, this will introduce another rounding into the loop. :(

BTW, some cd-rom drive acutally rounds odd length from 51 to 52. (eg. asus crw-5232as on my test machine). Overrunning the count and sg (sg->length is 51) by one byte as done in patch #4 can save us one loop in the trailing handling code. Ex. count 51 rounded to 52; byte is 52 rounded by device
=> byte -= count is zero and no need to go the trailing handling code.

Albert

ata_data_xfer() also does rounding: 'unsigned int words = buflen >> 1;'

Another idea - move rounding just before ata_data_xfer() so 'count' will stay rounded only for the duration of ata_data_xfer() ('count' is reset for every sg)
and we won't have to worry about sg->length and 'bytes'.

Or just leave it as it is and reorder patches... :).

Attached please find the revised patch #3 and #4 for your review.

I applied patch #1 of this series.

Could you please wait a couple days (for your patch #1 to appear in upstream), and then resend the rest of this patch series?

Thanks for your patience,

Jeff

Hi Jeff,

Resend the patch #2 against the linux-2.6.git tree (0086b5ec7834b78358dea3f713275a9ae2b229ec).

Problem:

    In __atapi_pio_bytes(), when (bytes > qc->nbytes) or (qc->cursg_ofs > sg->length) or

(count > bytes), the if condition is not handled properly.

Changes:
- Fix the "if conditions" to make the "if conditions" more robust.

Albert

Signed-off-by Albert Lee <[email protected]>

drivers/scsi/libata-core.c 2005-06-06 13:52:11.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-06-10 14:51:51.000000000 +0800 +5 -4

@@ -2571,7 +2571,7 @@
        unsigned char *buf;
        unsigned int offset, count;
 
-       if (qc->curbytes == qc->nbytes - bytes)
+       if (qc->curbytes + bytes >= qc->nbytes)
                ap->pio_task_state = PIO_ST_LAST;
 
 next_sg:

@@ -2592,11 +2592,10 @@
 
        buf = kmap(page) + offset;
 
-       bytes -= count;
        qc->curbytes += count;
        qc->cursg_ofs += count;
 
-       if (qc->cursg_ofs == sg->length) {
+       if (qc->cursg_ofs >= sg->length) {
                qc->cursg++;
                qc->cursg_ofs = 0;
        }

@@ -2608,7 +2607,9 @@
 
        kunmap(page);
 
-       if (bytes) {
+       if (bytes > count) {
+               bytes -= count;
+
                goto next_sg;
        }
 }

Quoted 2 lines · Albert Lee

Hi Jeff,

Quoted 5 lines

Resend the patch #2 against the linux-2.6.git tree (0086b5ec7834b78358dea3f713275a9ae2b229ec).

Problem:

   In __atapi_pio_bytes(), when (bytes > qc->nbytes) or (qc->cursg_ofs > sg->length) or

(count > bytes), the if condition is not handled properly.

Only (bytes > qc->nbytes) can happen now.