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 +1 -3

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

@@ -2609,8 +2609,6 @@
 	kunmap(page);
 
 	if (bytes) {
-		if (qc->cursg_ofs < sg->length)
-			goto next_page;
 		goto next_sg;
 	}
 }

Bartlomiej Zolnierkiewicz

2005-06-06 09:25 UTC #2 lore raw

Re: [PATCH 1/4] sg traverse fix for __atapi_pio_bytes()

Hi,

Quoted 41 lines · Albert Lee

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

drivers/scsi/libata-core.c 2005-06-06 13:45:33.000000000 +0800 line 2614

Quoted 8 lines

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 +0 -2
@@ -2609,8 +2609,6 @@
        kunmap(page);
 
        if (bytes) {
-               if (qc->cursg_ofs < sg->length)
-                       goto next_page;
                goto next_sg;
        }
 }

Looks good. Jeff, please apply.

Ack-ed by: Bartlomiej Zolnierkiewicz <[email protected]>

Minor CodingStyle nitpick - braces can be dropped:

        if (bytes)
                goto next_sg;

Bartlomiej

Albert Lee

author

2005-06-06 07:58 UTC #3 lore raw

PATCH 2/4 if condition fix for __atapi_pio_bytes()

In reply to Albert Lee

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 +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;
 	}
 }

Bartlomiej Zolnierkiewicz

2005-06-06 09:33 UTC #4 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

Hi,

Quoted 5 lines · Albert Lee

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.

(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

Quoted 46 lines

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;
        }
 }

Albert Lee

author

2005-06-08 03:48 UTC #5 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

Hi Bart:

Quoted 7 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...

Quoted 8 lines

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

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

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.

Albert

Bartlomiej Zolnierkiewicz

2005-06-08 09:48 UTC #6 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

Quoted 14 lines · Albert Lee

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 7 lines

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.

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

[ unless I'm missing something ]

Thanks,
Bartlomiej

Albert Lee

author

2005-06-08 11:59 UTC #7 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

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

Bartlomiej Zolnierkiewicz

2005-06-08 13:33 UTC #8 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

Quoted 40 lines · Albert Lee

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

Quoted 1 line

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

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... :).

Thanks.

Quoted 10 lines

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

Albert Lee

author

2005-06-09 04:06 UTC #9 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

Quoted 8 lines · Bartlomiej Zolnierkiewicz

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... :).

Hi Bart,

How about moving the odd length rounding up code to ata_mmio_data_xfer() and ata_pio_data_xfer()? It seems this can make the code of patch #4 simplified.
Also we can change the rounding in the trailing data handling code
from 'words = (bytes + 1)>> 1 ' to 'words = bytes >> 1'.

Ex. If bytes is 52, and buffer length is 51,
ata_data_xfer() will be called with 51. ata_data_xfer() will actually transfer 52 bytes.

1 byte left for the trailing data handling. Since
we know ata_data_xfer() round up one more byte, so in the trailing data handling code, we can safely round down one less byte by 'words = bytes >> 1'.

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

Albert

drivers/scsi/libata-core.c +2 -2

@@ -2491,7 +2491,7 @@
 			       unsigned int buflen, int write_data)
 {
 	unsigned int i;
-	unsigned int words = buflen >> 1;
+	unsigned int words = (buflen+1) >> 1;
 	u16 *buf16 = (u16 *) buf;
 	void __iomem *mmio = (void __iomem *)ap->ioaddr.data_addr;

@@ -2507,7 +2507,7 @@
 static void ata_pio_data_xfer(struct ata_port *ap, unsigned char *buf,
 			      unsigned int buflen, int write_data)
 {
-	unsigned int dwords = buflen >> 1;
+	unsigned int dwords = (buflen+1) >> 1;
 
 	if (write_data)
 		outsw(ap->ioaddr.data_addr, buf, dwords);

drivers/scsi/libata-core.c +17 -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;
+		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++) {
+			ata_data_xfer(ap, pad_buf, sizeof(pad_buf), do_write);
+		}
+
+		ap->pio_task_state = PIO_ST_LAST;
+		return;
+	}
+
 	sg = &qc->sg[qc->cursg];
 
 	page = sg->page;

Bartlomiej Zolnierkiewicz

2005-06-09 06:54 UTC #10 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

Quoted 1 line · Albert Lee

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

Fine with me.

Bartlomiej Zolnierkiewicz

2005-06-08 13:39 UTC #11 lore raw

Re: [PATCH 2/4] if condition fix for __atapi_pio_bytes()

In reply to Albert Lee

Quoted 1 line · Albert Lee

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

Indeed and atapi_[input,output]_bytes() always do rounding.

What about simple...

count++; /* round odd transfers */
ata_data_xfer(ap, buf, count, do_write);

Bartlomiej

Albert Lee

author

2005-06-06 08:00 UTC #12 lore raw

PATCH 3/4 trailing data handling fix for __atapi_pio_bytes()

In reply to Albert Lee

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 +17 -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++) {
+			ata_data_xfer(ap, pad_buf, sizeof(pad_buf), do_write);
+		}
+
+		ap->pio_task_state = PIO_ST_LAST;
+		return;
+	}
+
 	sg = &qc->sg[qc->cursg];
 
 	page = sg->page;

Bartlomiej Zolnierkiewicz

2005-06-06 09:38 UTC #13 lore raw

Re: [PATCH 3/4] trailing data handling fix for __atapi_pio_bytes()

Hi,

Quoted 32 lines · Albert Lee

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++) {

drivers/scsi/libata-core.c 2005-06-06 13:57:18.000000000 +0800 line 2597

Quoted 10 lines

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 +7 -0
+                       ata_data_xfer(ap, pad_buf, sizeof(pad_buf), do_write);
+               }
+
+               ap->pio_task_state = PIO_ST_LAST;
+               return;
+       }
+
        sg = &qc->sg[qc->cursg];
 
        page = sg->page;

Looks good. Jeff, please apply.

Ack-ed by: Bartlomiej Zolnierkiewicz <[email protected]>

Bartlomiej

Albert Lee

author

2005-06-06 08:01 UTC #14 lore raw

PATCH 4/4 odd length data handling for __atapi_pio_bytes()

In reply to Albert Lee

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 +7 -0

@@ -2607,6 +2607,13 @@
 	/* don't cross page boundaries */
 	count = min(count, (unsigned int)PAGE_SIZE - offset);
 
+	/* handle the odd condition */
+	if (unlikely(count & 0x01)) {
+		DPRINTK("ata%u: odd count %u rounded: qc->nbytes %u, bytes %u\n",
+			ap->id, count, qc->nbytes, bytes);
+		count++;
+	}
+
 	buf = kmap(page) + offset;
 
 	qc->curbytes += count;

Bartlomiej Zolnierkiewicz

2005-06-06 09:40 UTC #15 lore raw

Re: [PATCH 4/4] odd length data handling for __atapi_pio_bytes()

Hi,

Quoted 23 lines · Albert Lee

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);
 

drivers/scsi/libata-core.c 2005-06-06 14:54:36.000000000 +0800 line 2619

Quoted 10 lines

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 +7 -0

+       /* handle the odd condition */
+       if (unlikely(count & 0x01)) {
+               DPRINTK("ata%u: odd count %u rounded: qc->nbytes %u, bytes %u\n",
+                       ap->id, count, qc->nbytes, bytes);
+               count++;
+       }
+
        buf = kmap(page) + offset;
 
        qc->curbytes += count;

Looks good. Jeff, please apply.
[ Hm, I'm not very creative... ]

Ack-ed by: Bartlomiej Zolnierkiewicz <[email protected]>

Bartlomiej

A message that is not in the archive (sent off-list or not yet received): [email protected]

Albert Lee

author

2005-06-10 06:08 UTC #17 lore raw

Hi Jeff,

Quoted 12 lines

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

Thanks for reviewing/accepting patch #1. :)
I'll resend other patches later once patch #1 merged to the mainline.

Albert

Albert Lee

author

2005-06-10 07:38 UTC #18 lore raw

PATCH 2/4 libata: if condition fix for __atapi_pio_bytes()

In reply to a message not in the archive

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 +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;
 	}
 }

Bartlomiej Zolnierkiewicz

2005-06-10 07:57 UTC #19 lore raw

Re: [PATCH 2/4] libata: if condition fix for __atapi_pio_bytes()

Quoted 7 lines · Albert Lee

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.

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

Quoted 48 lines

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;
        }
 }

Albert Lee

author

2005-06-10 08:32 UTC #20 lore raw

Re: [PATCH 2/4] libata: if condition fix for __atapi_pio_bytes()

Hi Bart:

Quoted 4 lines

Quoted 2 lines · Albert Lee
Hi Jeff,

Quoted 10 lines

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.

Yes exactly.

However, could the patch be kept as is?
Maybe I'm worried too much about something that won't happen. It makes me feel safer using "if (qc->cursg_ofs >= sg->length)" than using "if (qc->cursg_ofs == sg->length)".
Using ">=" also make it clearer that we don't want to overrun the buffer.

The correct description of the patch is corrected per your advice:

Problem:
In __atapi_pio_bytes(), when (bytes > qc->nbytes), the if condition is not handled properly.

Changes:
- Fix the "if condition" for the (bytes > qc->nbytes) situation.
- Althought (qc->cursg_ofs > sg->length) and (count > bytes) situation won't happen, the "if conditions" are also changed for safty.

Albert

Albert Lee

author

2005-06-10 07:45 UTC #21 lore raw

PATCH 3/4 libata: trailing data handling fix for __atapi_pio_bytes()

In reply to a message not in the archive

Hi Jeff,

Resend the patch #3.

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.

Albert

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

drivers/scsi/libata-core.c +17 -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;
+		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++) {
+			ata_data_xfer(ap, pad_buf, sizeof(pad_buf), do_write);
+		}
+
+		ap->pio_task_state = PIO_ST_LAST;
+		return;
+	}
+
 	sg = &qc->sg[qc->cursg];
 
 	page = sg->page;

Albert Lee

author

2005-06-10 07:47 UTC #22 lore raw

PATCH 4/4 libata: odd length data handling for __atapi_pio_bytes()

In reply to a message not in the archive

Hi Jeff,

Resend the patch #4.

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 up to the next even length. The same as atapi_input_bytes() in the ide-iops does.
This may overrun the odd length buffer by one byte.

- Rename 'dwords' to 'words' in ata_pio_data_xfer()

Albert

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

drivers/scsi/libata-core.c +4 -4

@@ -2491,7 +2491,7 @@
 			       unsigned int buflen, int write_data)
 {
 	unsigned int i;
-	unsigned int words = buflen >> 1;
+	unsigned int words = (buflen+1) >> 1;
 	u16 *buf16 = (u16 *) buf;
 	void __iomem *mmio = (void __iomem *)ap->ioaddr.data_addr;

@@ -2507,12 +2507,12 @@
 static void ata_pio_data_xfer(struct ata_port *ap, unsigned char *buf,
 			      unsigned int buflen, int write_data)
 {
-	unsigned int dwords = buflen >> 1;
+	unsigned int words = (buflen+1) >> 1;
 
 	if (write_data)
-		outsw(ap->ioaddr.data_addr, buf, dwords);
+		outsw(ap->ioaddr.data_addr, buf, words);
 	else
-		insw(ap->ioaddr.data_addr, buf, dwords);
+		insw(ap->ioaddr.data_addr, buf, words);
 }
 
 static void ata_data_xfer(struct ata_port *ap, unsigned char *buf,