[PATCH 0/4] Resubmit ATAPI PIO mode fixes

[PATCH 0/4] Resubmit ATAPI PIO mode fixes

[Albert Lee]

Hi Jeff,

   Resubmit the previous ATAPI PIO mode fixes for the new linux-2.6.git tree.
(HEAD eae936e21bd726f9d9555f2262d439fbcd61dccf)

Albert

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

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 <albertcc@tw.ibm.com>


[-- Attachment #2: 01_sg_fix.diff --]
[-- Type: text/plain, Size: 655 bytes --]

--- 10_atapi_pio_fix/drivers/scsi/libata-core.c	2005-06-06 13:37:50.000000000 +0800
+++ 11_atapi_pio_sg_fix/drivers/scsi/libata-core.c	2005-06-06 13:45:33.000000000 +0800
@@ -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;
 	}
 }

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

[Bartlomiej Zolnierkiewicz]

Hi,

On 6/6/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> 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 <albertcc@tw.ibm.com>
> 
> 
> 
> --- 10_atapi_pio_fix/drivers/scsi/libata-core.c 2005-06-06 13:37:50.000000000 +0800
> +++ 11_atapi_pio_sg_fix/drivers/scsi/libata-core.c      2005-06-06 13:45:33.000000000 +0800
> @@ -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;
>         }
>  }
> 

Looks good.  Jeff, please apply.

Ack-ed by: Bartlomiej Zolnierkiewicz <bzolnier@elka.pw.edu.pl>

Minor CodingStyle nitpick - braces can be dropped:

        if (bytes)
                goto next_sg;

Bartlomiej

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

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 <albertcc@tw.ibm.com>

[-- Attachment #2: 02_if_fix.diff --]
[-- Type: text/plain, Size: 763 bytes --]

--- 11_atapi_pio_sg_fix/drivers/scsi/libata-core.c	2005-06-06 13:45:33.000000000 +0800
+++ 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c	2005-06-06 13:52:11.000000000 +0800
@@ -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;
 	}
 }

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

[Bartlomiej Zolnierkiewicz]

Hi,

On 6/6/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> 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

> 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 <albertcc@tw.ibm.com>
> 
> 
> --- 11_atapi_pio_sg_fix/drivers/scsi/libata-core.c      2005-06-06 13:45:33.000000000 +0800
> +++ 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c    2005-06-06 13:52:11.000000000 +0800
> @@ -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;
>         }
>  }
>

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

[Albert Lee]

Hi Bart:

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

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

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

[Bartlomiej Zolnierkiewicz]

On 6/8/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> Hi Bart:
> 
> >
> >
> > (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
> >
> >
> 
> 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

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

[Albert Lee]

Hi Bart:

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

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

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

[Bartlomiej Zolnierkiewicz]

On 6/8/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> 
> Hi Bart:
> 
> >>
> >>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
> >
> 
> 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. :(

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.

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

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]



Bartlomiej Zolnierkiewicz wrote:

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

[-- Attachment #2: 04_odd_data.diff --]
[-- Type: text/plain, Size: 741 bytes --]

--- 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c	2005-06-09 11:05:10.000000000 +0800
+++ 14_atapi_pio_odd_handling/drivers/scsi/libata-core.c	2005-06-09 11:04:09.000000000 +0800
@@ -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);

[-- Attachment #3: 03_trailing_data.diff --]
[-- Type: text/plain, Size: 738 bytes --]

--- 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c	2005-06-06 13:52:11.000000000 +0800
+++ 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c	2005-06-09 11:05:10.000000000 +0800
@@ -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;

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

[Bartlomiej Zolnierkiewicz]

On 6/9/05, Albert Lee <albertcc@tw.ibm.com> wrote:

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

Fine with me.

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

[Bartlomiej Zolnierkiewicz]

On 6/8/05, Albert Lee <albertcc@tw.ibm.com> wrote:

> 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

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

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 <albertcc@tw.ibm.com>

[-- Attachment #2: 03_trailing_data.diff --]
[-- Type: text/plain, Size: 769 bytes --]

--- 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c	2005-06-06 13:52:11.000000000 +0800
+++ 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c	2005-06-06 13:57:18.000000000 +0800
@@ -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;

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

[Bartlomiej Zolnierkiewicz]

Hi,

On 6/6/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> 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 <albertcc@tw.ibm.com>
> 
> 
> --- 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c    2005-06-06 13:52:11.000000000 +0800
> +++ 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c      2005-06-06 13:57:18.000000000 +0800
> @@ -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;
> 

Looks good.  Jeff, please apply.

Ack-ed by: Bartlomiej Zolnierkiewicz <bzolnier@elka.pw.edu.pl>

Bartlomiej

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

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 <albertcc@tw.ibm.com>

[-- Attachment #2: 04_odd_data.diff --]
[-- Type: text/plain, Size: 560 bytes --]

--- 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c	2005-06-06 13:57:18.000000000 +0800
+++ 14_atapi_pio_odd_handling/drivers/scsi/libata-core.c	2005-06-06 14:54:36.000000000 +0800
@@ -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;

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

[Bartlomiej Zolnierkiewicz]

Hi,

On 6/6/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> 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 <albertcc@tw.ibm.com>
> 
> 
> --- 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c      2005-06-06 13:57:18.000000000 +0800
> +++ 14_atapi_pio_odd_handling/drivers/scsi/libata-core.c        2005-06-06 14:54:36.000000000 +0800
> @@ -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;
> 

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

Ack-ed by: Bartlomiej Zolnierkiewicz <bzolnier@elka.pw.edu.pl>

Bartlomiej

Re: [PATCH 0/4] Resubmit ATAPI PIO mode fixes

[Albert Lee]

Hi Jeff,

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

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

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 <albertcc@tw.ibm.com>


[-- Attachment #2: 02_if_fix.diff --]
[-- Type: text/plain, Size: 763 bytes --]

--- 11_atapi_pio_sg_fix/drivers/scsi/libata-core.c	2005-06-10 14:51:51.000000000 +0800
+++ 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c	2005-06-06 13:52:11.000000000 +0800
@@ -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;
 	}
 }

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

[Bartlomiej Zolnierkiewicz]

n 6/10/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> 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.

> Changes:
> - Fix the "if conditions" to make the "if conditions" more robust.
> 
> 
> Albert
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> 
> 
> --- 11_atapi_pio_sg_fix/drivers/scsi/libata-core.c      2005-06-10 14:51:51.000000000 +0800
> +++ 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c    2005-06-06 13:52:11.000000000 +0800
> @@ -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;
>         }
>  }
> 
> 
>

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

[Albert Lee]

Hi Bart:

> n 6/10/05, Albert Lee <albertcc@tw.ibm.com> wrote:
> 
>>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.
> 
> 
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

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

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 <albertcc@tw.ibm.com>


[-- Attachment #2: 03_trailing_data.diff --]
[-- Type: text/plain, Size: 738 bytes --]

--- 12_atapi_pio_if_condition_fix/drivers/scsi/libata-core.c	2005-06-06 13:52:11.000000000 +0800
+++ 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c	2005-06-09 11:05:10.000000000 +0800
@@ -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;

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

[Albert Lee]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

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 <albertcc@tw.ibm.com>

[-- Attachment #2: 04_odd_data.diff --]
[-- Type: text/plain, Size: 953 bytes --]

--- 13_atapi_pio_trailing_data_handling/drivers/scsi/libata-core.c	2005-06-09 11:05:10.000000000 +0800
+++ 14_atapi_pio_odd_handling/drivers/scsi/libata-core.c	2005-06-10 15:08:24.000000000 +0800
@@ -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,