[PATCH 0/3] libata PIO fixes (revised)

[PATCH 0/3] libata PIO fixes (revised)

[Albert Lee]

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

[PATCH 1/3] libata "if condition" enhancement

[Albert Lee]

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

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

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



[-- Attachment #2: pio1.diff --]
[-- Type: text/plain, Size: 732 bytes --]

--- linux/drivers/scsi/libata-core.c	2005-08-01 15:35:17.000000000 +0800
+++ 01_pio_ifcond/drivers/scsi/libata-core.c	2005-08-01 15:52:29.000000000 +0800
@@ -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;
 	}
 }

[PATCH 2/3] libata ata_data_xfer() fix

[Albert Lee]

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

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

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

[-- Attachment #2: pio2.diff --]
[-- Type: text/plain, Size: 4984 bytes --]

--- linux/include/linux/libata.h	2005-08-01 15:35:39.000000000 +0800
+++ 02_pio_odd/include/linux/libata.h	2005-07-19 18:33:01.000000000 +0800
@@ -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? */
--- 01_pio_ifcond/drivers/scsi/libata-core.c	2005-08-01 15:52:29.000000000 +0800
+++ 02_pio_odd/drivers/scsi/libata-core.c	2005-08-01 16:14:28.000000000 +0800
@@ -2519,43 +2519,146 @@
 #endif /* __BIG_ENDIAN */
 }
 
+/**
+ *	ata_mmio_data_xfer - Transfer data by MMIO
+ *	@ap: port to read/write
+ *	@buf: data buffer
+ *	@buflen: buffer length
+ *	@do_write: read/write
+ *
+ *	Transfer data from/to the device data register by MMIO.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ */
+
 static void ata_mmio_data_xfer(struct ata_port *ap, unsigned char *buf,
 			       unsigned int buflen, int write_data)
 {
-	unsigned int i;
-	unsigned int words = buflen >> 1;
-	u16 *buf16 = (u16 *) buf;
+	unsigned int words;
+	unsigned int dwords = buflen >> 2;
+	unsigned int trailing = buflen & 0x03;
 	void __iomem *mmio = (void __iomem *)ap->ioaddr.data_addr;
 
-	if (write_data) {
-		for (i = 0; i < words; i++)
-			writew(le16_to_cpu(buf16[i]), mmio);
+	/* Transfer multiple of 4 bytes */
+	if (ap->flags & ATA_FLAG_IO_32BIT) {
+		if (write_data)
+			iowrite32_rep(mmio, buf, dwords);
+		else
+			ioread32_rep(mmio, buf, dwords);
 	} else {
-		for (i = 0; i < words; i++)
-			buf16[i] = cpu_to_le16(readw(mmio));
+		words  = dwords << 1;
+
+		if (write_data)
+			iowrite16_rep(mmio, buf, words);
+		else
+			ioread16_rep(mmio, buf, words);
+	}
+
+	/* Transfer trailing 1-3 bytes */
+	if (unlikely(trailing)) {
+		u16 buf16[2] = { 0, 0 };
+		unsigned char *trailing_buf = buf + buflen - trailing;
+
+		words = (trailing + 1) >> 1;
+
+		if (write_data) {
+			memcpy(buf16, trailing_buf, trailing);
+			iowrite16_rep(mmio, buf16, words);
+		} else {
+			ioread16_rep(mmio, buf16, words);
+			memcpy(trailing_buf, buf16, trailing);
+		}
 	}
 }
 
+/**
+ *	ata_pio_data_xfer - Transfer data by PIO
+ *	@ap: port to read/write
+ *	@buf: data buffer
+ *	@buflen: buffer length
+ *	@do_write: read/write
+ *
+ *	Transfer data from/to the device data register by PIO.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ */
+
 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 >> 2;
+	unsigned int trailing = buflen & 0x03;
 
-	if (write_data)
-		outsw(ap->ioaddr.data_addr, buf, dwords);
-	else
-		insw(ap->ioaddr.data_addr, buf, dwords);
+	/* Transfer multiple of 4 bytes */
+	if (ap->flags & ATA_FLAG_IO_32BIT) {
+		if (write_data)
+			outsl(ap->ioaddr.data_addr, buf, dwords);
+		else
+			insl(ap->ioaddr.data_addr, buf, dwords);
+
+	} else {
+		if (write_data)
+			outsw(ap->ioaddr.data_addr, buf, dwords << 1);
+		else
+			insw(ap->ioaddr.data_addr, buf, dwords << 1);
+	}
+
+	/* Transfer trailing 1-3 bytes */
+	if (unlikely(trailing)) {
+		u16 buf16[2] = { 0, 0 };
+		unsigned char *trailing_buf = buf + buflen - trailing;
+		unsigned int words = (trailing + 1) >> 1;
+
+		if (write_data) {
+			memcpy(buf16, trailing_buf, trailing);
+			outsw(ap->ioaddr.data_addr, buf16, words);
+		} else {
+			insw(ap->ioaddr.data_addr, buf16, words);
+			memcpy(trailing_buf, buf16, trailing);
+		}
+	}
 }
 
+/**
+ *	ata_data_xfer - Transfer data from/to the data register.
+ *	@ap: port to read/write
+ *	@buf: data buffer
+ *	@buflen: buffer length
+ *	@do_write: read/write
+ *
+ *	Transfer data from/to the device data register.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ */
+
 static void ata_data_xfer(struct ata_port *ap, unsigned char *buf,
 			  unsigned int buflen, int do_write)
 {
+	/* 4 byte alignment assumed */
+	assert(~((unsigned long)buf & 0x03));
+
 	if (ap->flags & ATA_FLAG_MMIO)
 		ata_mmio_data_xfer(ap, buf, buflen, do_write);
 	else
 		ata_pio_data_xfer(ap, buf, buflen, do_write);
 }
 
+/**
+ *	ata_pio_sector - Transfer ATA_SECT_SIZE (512 bytes) of data.
+ *	@qc: Command on going
+ *
+ *	Transfer ATA_SECT_SIZE of data from/to the ATA device.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
 static void ata_pio_sector(struct ata_queued_cmd *qc)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
@@ -2594,6 +2697,18 @@
 	kunmap(page);
 }
 
+/**
+ *	__atapi_pio_bytes - Transfer data from/to the ATAPI device.
+ *	@qc: Command on going
+ *	@bytes: number of bytes
+ *
+ *	Transfer Transfer data from/to the ATAPI device.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ */
+
 static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);

[PATCH 3/3] libata handle the case when device returns/needs extra data

[Albert Lee]

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

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

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



[-- Attachment #2: pio3.diff --]
[-- Type: text/plain, Size: 1006 bytes --]

--- 02_pio_odd/drivers/scsi/libata-core.c	2005-08-01 16:14:28.000000000 +0800
+++ 03_pio_extra_data/drivers/scsi/libata-core.c	2005-08-01 21:46:37.000000000 +0800
@@ -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;

Re: [PATCH 2/3] libata ata_data_xfer() fix

[Jeff Garzik]

Albert Lee wrote:
> 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.

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

Re: [PATCH 3/3] libata handle the case when device returns/needs extra data

[Jeff Garzik]

Albert Lee wrote:
> 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

This patch looks pretty OK to me.

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

	Jeff