[PATCH 0/2] libata: minor pio fixes (resend)

[PATCH 0/2] libata: minor pio fixes (resend)

[Albert Lee]

Minor pio fixes:
1/2: move ata_altstatus() to pio data xfer functions
2/2: change the last state of pio read to HSM_ST_IDLE

(The previous "remove unneeded ata_altstatus() from ata_hsm_qc_complete()"
has been accepted.)

Patch against the libata-dev tree for your review.

[PATCH 1/2] libata: move ata_altstatus() to pio data xfer functions

[Albert Lee]

Patch 1/2:
 Move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions
like ata_pio_sectors() and atapi_pio_bytes() where it makes more sense.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
atapi_send_cdb() already calls ata_altstatus() inside.
This patch makes ata_pio_sectors() and atapi_pio_bytes() do the same.

diff -Nrup 00_libata-dev/drivers/ata/libata-core.c 01_move_altstatus/drivers/ata/libata-core.c
--- 00_libata-dev/drivers/ata/libata-core.c	2007-07-04 11:26:30.000000000 +0800
+++ 01_move_altstatus/drivers/ata/libata-core.c	2007-07-04 11:32:36.000000000 +0800
@@ -4524,6 +4524,8 @@ static void ata_pio_sectors(struct ata_q
 			ata_pio_sector(qc);
 	} else
 		ata_pio_sector(qc);
+
+	ata_altstatus(ap); /* flush */
 }
 
 /**
@@ -4698,6 +4700,7 @@ static void atapi_pio_bytes(struct ata_q
 	VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
 
 	__atapi_pio_bytes(qc, bytes);
+	ata_altstatus(ap); /* flush */
 
 	return;
 
@@ -4869,7 +4872,6 @@ fsm_start:
 			 */
 			ap->hsm_task_state = HSM_ST;
 			ata_pio_sectors(qc);
-			ata_altstatus(ap); /* flush */
 		} else
 			/* send CDB */
 			atapi_send_cdb(ap, qc);
@@ -4950,7 +4952,6 @@ fsm_start:
 
 				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
 					ata_pio_sectors(qc);
-					ata_altstatus(ap);
 					status = ata_wait_idle(ap);
 				}
 
@@ -4970,13 +4971,11 @@ fsm_start:
 			if (ap->hsm_task_state == HSM_ST_LAST &&
 			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
 				/* all data read */
-				ata_altstatus(ap);
 				status = ata_wait_idle(ap);
 				goto fsm_start;
 			}
 		}
 
-		ata_altstatus(ap); /* flush */
 		poll_next = 1;
 		break;
 

Re: [PATCH 1/2] libata: move ata_altstatus() to pio data xfer functions

[Jeff Garzik]

Albert Lee wrote:
> Patch 1/2:
>  Move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions
> like ata_pio_sectors() and atapi_pio_bytes() where it makes more sense.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> atapi_send_cdb() already calls ata_altstatus() inside.
> This patch makes ata_pio_sectors() and atapi_pio_bytes() do the same.

applied

[PATCH 2/2] libata: change the last state of pio read to HSM_ST_IDLE

[Albert Lee]

Patch 2/2:
  After reading the last pio data block, the HSM is waiting for device
to be idle, not waiting for the last interrupt.

This patch changes the state after "PIO data-in" to HSM_ST_IDLE instead
of HSM_ST_LAST for accuracy.

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

diff -Nrup 01_move_altstatus/drivers/ata/libata-core.c 02_pio_read_state/drivers/ata/libata-core.c
--- 01_move_altstatus/drivers/ata/libata-core.c	2007-07-04 11:32:36.000000000 +0800
+++ 02_pio_read_state/drivers/ata/libata-core.c	2007-07-04 11:33:53.000000000 +0800
@@ -4462,7 +4462,7 @@ static void ata_pio_sector(struct ata_qu
 	unsigned char *buf;
 
 	if (qc->curbytes == qc->nbytes - qc->sect_size)
-		ap->hsm_task_state = HSM_ST_LAST;
+		ap->hsm_task_state = do_write ? HSM_ST_LAST : HSM_ST_IDLE;
 
 	page = sg[qc->cursg].page;
 	offset = sg[qc->cursg].offset + qc->cursg_ofs;
@@ -4811,6 +4811,8 @@ int ata_hsm_move(struct ata_port *ap, st
 	 */
 	WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
 
+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
+
 fsm_start:
 	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
 		ap->print_id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -4968,8 +4970,7 @@ fsm_start:
 
 			ata_pio_sectors(qc);
 
-			if (ap->hsm_task_state == HSM_ST_LAST &&
-			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
+			if (ap->hsm_task_state == HSM_ST_IDLE) {
 				/* all data read */
 				status = ata_wait_idle(ap);
 				goto fsm_start;
@@ -4980,6 +4981,7 @@ fsm_start:
 		break;
 
 	case HSM_ST_LAST:
+	case HSM_ST_IDLE:
 		if (unlikely(!ata_ok(status))) {
 			qc->err_mask |= __ac_err_mask(status);
 			ap->hsm_task_state = HSM_ST_ERR;

Re: [PATCH 2/2] libata: change the last state of pio read to HSM_ST_IDLE

[Jeff Garzik]

Albert Lee wrote:
> Patch 2/2:
>   After reading the last pio data block, the HSM is waiting for device
> to be idle, not waiting for the last interrupt.
> 
> This patch changes the state after "PIO data-in" to HSM_ST_IDLE instead
> of HSM_ST_LAST for accuracy.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

Is this still needed?

Re: [PATCH 2/2] libata: change the last state of pio read to HSM_ST_IDLE

[Albert Lee]

Jeff Garzik wrote:
> Albert Lee wrote:
> 
>> Patch 2/2:
>>   After reading the last pio data block, the HSM is waiting for device
>> to be idle, not waiting for the last interrupt.
>>
>> This patch changes the state after "PIO data-in" to HSM_ST_IDLE instead
>> of HSM_ST_LAST for accuracy.
>>
>> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> 
> Is this still needed?
> 

Not quite needed; it only makes the state transition after reading the
last PIO block more accurate.

However, if we want to do part of the irq PIO in the workqueue sometime
in the future, this patch will be needed (otherwise the HSM might think
it expecting another irq). For the time being, maybe we can just skip
this patch until sometime it's really needed.
--
albert


 

Re: [PATCH 2/2] libata: change the last state of pio read to HSM_ST_IDLE

[Jeff Garzik]

Albert Lee wrote:
> Jeff Garzik wrote:
>> Albert Lee wrote:
>>
>>> Patch 2/2:
>>>   After reading the last pio data block, the HSM is waiting for device
>>> to be idle, not waiting for the last interrupt.
>>>
>>> This patch changes the state after "PIO data-in" to HSM_ST_IDLE instead
>>> of HSM_ST_LAST for accuracy.
>>>
>>> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
>>
>> Is this still needed?
>>
> 
> Not quite needed; it only makes the state transition after reading the
> last PIO block more accurate.
> 
> However, if we want to do part of the irq PIO in the workqueue sometime
> in the future, this patch will be needed (otherwise the HSM might think
> it expecting another irq). For the time being, maybe we can just skip
> this patch until sometime it's really needed.

OK, thanks.

	Jeff