[PATCH/RFC] libata: honor the transfer cycle time speficied by the EIDE device

[PATCH/RFC] libata: honor the transfer cycle time speficied by the EIDE device

[Albert Lee]

Jeff,

  The following code segment is not functional because the transfer cycle time speficied by 
  the EIDE device is later overwritten by ata_timing_quantize():

	/*
	 * If the drive is an EIDE drive, it can tell us it needs extended
	 * PIO/MW_DMA cycle timing.
	 */
	if (adev->id[ATA_ID_FIELD_VALID] & 2) {	/* EIDE drive */
		memset(&p, 0, sizeof(p));
		(snip)
		ata_timing_merge(&p, t, t, ATA_TIMING_CYCLE | ATA_TIMING_CYC8B);
               <== uninitialized "t" is used here
	}

	/*
	 * Convert the timing to bus clock counts.
	 */
	ata_timing_quantize(s, t, T, UT);  <== t is overwritten by quantized s

  The patch has been submitted for ide-timing.h before:
  http://marc.theaimsgroup.com/?l=linux-ide&m=110820013425454&w=2
  Resubmitted for libata.

Changes:
  - Minor fix to honor the following transfer cycle time speficied by the device
    - id[65]: Minimum Multiword DMA transfer cycle time per word
    - id[67]: Minimum PIO transfer cycle time without flow control
    - id[68]: Minimum PIO transfer cycle time with IORDY

Patch against the mainline tree
(f6ff56cd56b83d8edf4b3cffc5c53c56b37a5081)
For your review, thanks.

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

=======

--- linux/drivers/scsi/libata-core.c	2005-11-16 16:26:39.000000000 +0800
+++ time/drivers/scsi/libata-core.c	2005-11-16 16:30:59.000000000 +0800
@@ -1570,11 +1570,13 @@ int ata_timing_compute(struct ata_device
 
 	/*
 	 * Find the mode. 
-	*/
+	 */
 
 	if (!(s = ata_timing_find_mode(speed)))
 		return -EINVAL;
 
+	memcpy(t, s, sizeof(*s));
+
 	/*
 	 * If the drive is an EIDE drive, it can tell us it needs extended
 	 * PIO/MW_DMA cycle timing.
@@ -1595,7 +1597,7 @@ int ata_timing_compute(struct ata_device
 	 * Convert the timing to bus clock counts.
 	 */
 
-	ata_timing_quantize(s, t, T, UT);
+	ata_timing_quantize(t, t, T, UT);
 
 	/*
 	 * Even in DMA/UDMA modes we still use PIO access for IDENTIFY, S.M.A.R.T

Re: [PATCH/RFC] libata: honor the transfer cycle time speficied by the EIDE device

[Jeff Garzik]

Albert Lee wrote:
> Jeff,
> 
>   The following code segment is not functional because the transfer cycle time speficied by 
>   the EIDE device is later overwritten by ata_timing_quantize():
> 
> 	/*
> 	 * If the drive is an EIDE drive, it can tell us it needs extended
> 	 * PIO/MW_DMA cycle timing.
> 	 */
> 	if (adev->id[ATA_ID_FIELD_VALID] & 2) {	/* EIDE drive */
> 		memset(&p, 0, sizeof(p));
> 		(snip)
> 		ata_timing_merge(&p, t, t, ATA_TIMING_CYCLE | ATA_TIMING_CYC8B);
>                <== uninitialized "t" is used here
> 	}
> 
> 	/*
> 	 * Convert the timing to bus clock counts.
> 	 */
> 	ata_timing_quantize(s, t, T, UT);  <== t is overwritten by quantized s
> 
>   The patch has been submitted for ide-timing.h before:
>   http://marc.theaimsgroup.com/?l=linux-ide&m=110820013425454&w=2
>   Resubmitted for libata.
> 
> Changes:
>   - Minor fix to honor the following transfer cycle time speficied by the device
>     - id[65]: Minimum Multiword DMA transfer cycle time per word
>     - id[67]: Minimum PIO transfer cycle time without flow control
>     - id[68]: Minimum PIO transfer cycle time with IORDY
> 
> Patch against the mainline tree
> (f6ff56cd56b83d8edf4b3cffc5c53c56b37a5081)
> For your review, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> =======
> 
> --- linux/drivers/scsi/libata-core.c	2005-11-16 16:26:39.000000000 +0800
> +++ time/drivers/scsi/libata-core.c	2005-11-16 16:30:59.000000000 +0800
> @@ -1570,11 +1570,13 @@ int ata_timing_compute(struct ata_device
>  
>  	/*
>  	 * Find the mode. 
> -	*/
> +	 */
>  
>  	if (!(s = ata_timing_find_mode(speed)))
>  		return -EINVAL;
>  
> +	memcpy(t, s, sizeof(*s));
> +
>  	/*
>  	 * If the drive is an EIDE drive, it can tell us it needs extended
>  	 * PIO/MW_DMA cycle timing.
> @@ -1595,7 +1597,7 @@ int ata_timing_compute(struct ata_device
>  	 * Convert the timing to bus clock counts.
>  	 */
>  
> -	ata_timing_quantize(s, t, T, UT);
> +	ata_timing_quantize(t, t, T, UT);


Applied to upstream-fixes branch.  Let's see if Alan screams...  :)

	Jeff