Skip to contentSkip to conversation

libata: minor pio fixes (resend)

PATCH Applied Albert Lee 2 participants 7 messages Jul 4, 2007 – Oct 3, 2007 libata

lore
Share & tools
lore

Albert Lee

author
#0 lore raw

cover libata: minor pio fixes (resend)

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.

author
#1 lore raw

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

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 <[email protected]>

---
atapi_send_cdb() already calls ata_altstatus() inside. This patch makes ata_pio_sectors() and atapi_pio_bytes() do the same.

drivers/ata/libata-core.c +3 -4
diff -Nrup 00_libata-dev/drivers/ata/libata-core.c 01_move_altstatus/drivers/ata/libata-core.c
@@ -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;
Applied
#2 lore raw

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

Quoted 8 lines · 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 <[email protected]>

---
atapi_send_cdb() already calls ata_altstatus() inside. This patch makes ata_pio_sectors() and atapi_pio_bytes() do the same.

applied

author
#3 lore raw

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

In reply to 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 <[email protected]>

---

drivers/ata/libata-core.c +5 -3
diff -Nrup 01_move_altstatus/drivers/ata/libata-core.c 02_pio_read_state/drivers/ata/libata-core.c
@@ -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;
#4 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Is this still needed?

author
#5 lore raw

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

Quoted 6 lines · Jeff Garzik
Quoted 4 lines · 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.

Quoted 8 lines
Quoted 4 lines

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 <[email protected]>

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

#6 lore raw

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

Quoted 15 lines · Albert Lee
Quoted 6 lines · Jeff Garzik
Quoted 4 lines · 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.

Quoted 7 lines
Quoted 4 lines

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 <[email protected]>

Is this still needed?

Quoted 7 lines

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