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

[Albert Lee <albertcc@tw.ibm.com>]

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