libata PIO fixes (revised)

Jeff,

The PIO fixes revised according to the previous reviews.

1/3 pio1.diff:
- Modify the "if conditions" in __atapi_pio_bytes() to be more robust.

2/3 pio2.diff:
- Add ATA_FLAG_IO_32BIT to libata.h for hosts with 32-bit data register.
- Modify ata_mmio_data_xfer() and ata_pio_data_xfer() to handle buffer with length not aligned to 32-bit boundary.

3/3 pio3.diff:
- Modify __atapi_pio_bytes() to handle the case when device returns/needs extra data.

(Patch diff'ed against 2.6.13-rc4 tree, 02459eaab98a6a57717bc0cacede148fc76af881)

Tested ok on x86 with Promise PDC20275 and LG DVD-Multi drive.

For your review and advice, thanks.

Albert

Jeff,

PATCH 1/3: "if condition" enhancement.

Changes:

Modify __atapi_pio_bytes() to make the "if condition"
more robust, in case of something like (bytes > qc->nbytes), (qc->cursg_ofs > sg->length) or (count > bytes) happens.

Those conditions won't happen in the current libata code. Just to be safe (paranoid).

For your review, thanks.

Albert

@@ -2603,7 +2603,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:

@@ -2624,11 +2624,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;
 	}

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

Jeff,

PATCH 2/3: ata_data_xfer() fix

Changes:
- Add ATA_FLAG_IO_32BIT to libata.h for hosts with 32-bit data register.
- Modify ata_mmio_data_xfer() and ata_pio_data_xfer() to handle buffer length not aligned to 32-bit boundary.

This patch does not reuse ap->pad as alignment buffer since four bytes of local variable seems good enough.

For your review, thanks.

Albert

@@ -113,6 +113,7 @@
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
+	ATA_FLAG_IO_32BIT	= (1 << 9), /* data register can do 32-bit IO */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */

32-bit IO needs to be a separate patch.

I made a design decision to -not- support 32bit IO, since it is not universally supported by all host controllers. It is much easier to say "use DMA" instead.

libata would need additional checks, following similar logic in drivers/ide, to do 32-bit IO.

Jeff

Jeff,

PATCH 3/3:

Sometimes the device returns/needs extra data than expected.

Changes:
- Modify __atapi_pio_bytes() to handle the case when device returns/needs extra data.
- for read case, discard trailing data from the device
- for write case, padding zero data to the device

For your review, thanks.

Albert

@@ -2722,6 +2722,29 @@
 		ap->pio_task_state = PIO_ST_LAST;
 
 next_sg:
+	if (unlikely(qc->cursg >= qc->n_elem)) {
+		/* 
+		 * The end of qc->sg is reached and the device expects
+		 * more data to transfer. In order not to overrun qc->sg
+		 * and fulfill length specified in the byte count register,
+		 *    - for read case, discard trailing data from the device
+		 *    - for write case, padding zero data to the device
+		 */
+		u16 pad_buf[1] = { 0 };
+		unsigned int words = bytes >> 1;
+		unsigned int i;
+
+		if (words) /* warning if bytes > 1 */
+			printk(KERN_WARNING "ata%u: %u bytes trailing data\n", 
+			       ap->id, bytes);
+
+		for (i = 0; i < words; i++)
+			ata_data_xfer(ap, (unsigned char*)pad_buf, 2, do_write);
+
+		ap->pio_task_state = PIO_ST_LAST;
+		return;
+	}
+
 	sg = &qc->sg[qc->cursg];
 
 	page = sg->page;

This patch looks pretty OK to me.

I'll await your responses, to my responses, on the other two patches.

Jeff