[PATCH 0/2] libata: ata_pio_task() accessing 'ap->pio_task_state' fix

[PATCH 0/2] libata: ata_pio_task() accessing 'ap->pio_task_state' fix

[Albert Lee]

Jeff,

During PIO, after calling ata_poll_qc_complete(), the next command might be
running and the value of 'ap->pio_task_state' might have been changed.
Accessing 'ap->pio_task_state' is not safe at this point.

Ex.
qc 1 completed. queuing a final task with ap->pio_task_state == PIO_ST_IDLE.
qc 2 started, queuing a new task with ap->pio_task_state set to PIO_ST.
qc 1 read ap->pio_task_state as PIO_ST; not PIO_ST_IDLE as expected.
=> 2 qc running in the workqueue with pio_task_state PIO_ST.

Changes:
  1/2: Modify ata_pio_complete() and ata_pio_block() to return
       whether qc has been completed.

  2/2: Modify ata_pio_task() to check the return value. Only queue next 
step and
        access 'ap->pio_task_state' if the command is not completed.

Patch for 2.6.13 (80ac2912f846c01d702774bb6aa7100ec71e88b9).
Tested on x86 with Promise PDC20275 and LG DVD-Multi drive.
For your review and advice, thanks.

Albert

[PATCH 1/2] libata: ata_pio_complete() and ata_pio_block() return value

[Albert Lee]

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

Jeff,

PATCH 1/2:
Modify ata_pio_complete() and ata_pio_block() to return
whether qc has been completed.

For your review, thanks.

Albert

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



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

--- linux/drivers/scsi/libata-core.c.ori	2005-09-02 17:59:15.000000000 +0800
+++ pio1/drivers/scsi/libata-core.c	2005-09-02 18:37:26.000000000 +0800
@@ -2461,9 +2461,12 @@
  *
  *	LOCKING:
  *	None.  (executing in kernel thread context)
+ *
+ *	RETURNS:
+ *	Zero if qc completed, non-zero otherwise.
  */
 
-static void ata_pio_complete (struct ata_port *ap)
+static int ata_pio_complete (struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
 	u8 drv_stat;
@@ -2482,14 +2485,14 @@
 		if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
 			ap->pio_task_state = PIO_ST_LAST_POLL;
 			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return;
+			return 1; /* qc not completed */
 		}
 	}
 
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
 		ap->pio_task_state = PIO_ST_ERR;
-		return;
+		return 1; /* qc not completed */
 	}
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -2498,6 +2501,7 @@
 	ap->pio_task_state = PIO_ST_IDLE;
 
 	ata_poll_qc_complete(qc, drv_stat);
+	return 0; /* qc completed */
 }
 
 
@@ -2813,9 +2817,12 @@
  *
  *	LOCKING:
  *	None.  (executing in kernel thread context)
+ *
+ *	RETURNS:
+ *	Zero if qc completed, non-zero otherwise.
  */
 
-static void ata_pio_block(struct ata_port *ap)
+static int ata_pio_block(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
 	u8 status;
@@ -2835,7 +2842,7 @@
 		if (status & ATA_BUSY) {
 			ap->pio_task_state = PIO_ST_POLL;
 			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return;
+			return 1; /* qc not completed */
 		}
 	}
 
@@ -2848,7 +2855,7 @@
 			ap->pio_task_state = PIO_ST_IDLE;
 
 			ata_poll_qc_complete(qc, status);
-			return;
+			return 0; /* qc completed */
 		}
 
 		atapi_pio_bytes(qc);
@@ -2856,11 +2863,13 @@
 		/* handle BSY=0, DRQ=0 as error */
 		if ((status & ATA_DRQ) == 0) {
 			ap->pio_task_state = PIO_ST_ERR;
-			return;
+			return 1; /* qc not completed */
 		}
 
 		ata_pio_sector(qc);
 	}
+
+	return 1; /* qc not completed */
 }
 
 static void ata_pio_error(struct ata_port *ap)

[PATCH 2/2] libata: ata_pio_task() fix

[Albert Lee]

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

Jeff,

PATCH 2/2:
Modify ata_pio_task() to check the return value of patch 1/2
before queuing next step and using the value of 'ap->pio_task_state'.

For your review, thanks.

Albert

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



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

--- pio1/drivers/scsi/libata-core.c	2005-09-02 18:37:26.000000000 +0800
+++ pio2/drivers/scsi/libata-core.c	2005-09-02 18:37:19.000000000 +0800
@@ -2893,21 +2893,20 @@
 {
 	struct ata_port *ap = _data;
 	unsigned long timeout = 0;
+	int has_next = 0;
 
 	switch (ap->pio_task_state) {
-	case PIO_ST_IDLE:
-		return;
-
 	case PIO_ST:
-		ata_pio_block(ap);
+		has_next = ata_pio_block(ap);
 		break;
 
 	case PIO_ST_LAST:
-		ata_pio_complete(ap);
+		has_next = ata_pio_complete(ap);
 		break;
 
 	case PIO_ST_POLL:
 	case PIO_ST_LAST_POLL:
+		has_next = 1;
 		timeout = ata_pio_poll(ap);
 		break;
 
@@ -2915,13 +2914,18 @@
 	case PIO_ST_ERR:
 		ata_pio_error(ap);
 		return;
+	default:
+		printk(KERN_ERR "ata%u unknown PIO task state %u\n", 
+		       ap->id, ap->pio_task_state);
+		return;
 	}
 
-	if (timeout)
-		queue_delayed_work(ata_wq, &ap->pio_task,
-				   timeout);
-	else
-		queue_work(ata_wq, &ap->pio_task);
+	if (has_next) {
+		if (timeout)
+			queue_delayed_work(ata_wq, &ap->pio_task, timeout);
+		else
+			queue_work(ata_wq, &ap->pio_task);
+	}
 }
 
 static void atapi_request_sense(struct ata_port *ap, struct ata_device *dev,

Re: [PATCH 0/2] libata: ata_pio_task() accessing 'ap->pio_task_state' fix

[Jeff Garzik]

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

Albert Lee wrote:
> Jeff,
> 
> During PIO, after calling ata_poll_qc_complete(), the next command might be
> running and the value of 'ap->pio_task_state' might have been changed.
> Accessing 'ap->pio_task_state' is not safe at this point.
> 
> Ex.
> qc 1 completed. queuing a final task with ap->pio_task_state == 
> PIO_ST_IDLE.
> qc 2 started, queuing a new task with ap->pio_task_state set to PIO_ST.
> qc 1 read ap->pio_task_state as PIO_ST; not PIO_ST_IDLE as expected.
> => 2 qc running in the workqueue with pio_task_state PIO_ST.
> 
> Changes:
>  1/2: Modify ata_pio_complete() and ata_pio_block() to return
>       whether qc has been completed.
> 
>  2/2: Modify ata_pio_task() to check the return value. Only queue next 
> step and
>        access 'ap->pio_task_state' if the command is not completed.

I would prefer something more like the attached patch.  This patch does 
two things:
* eliminate needless queueing
* don't race with qc-complete in ata_pio_complete()

This patch is COMPLETELY UNTESTED.  Please use the attached as a base, 
to replace the patch series you have submitted.  I won't check it in, as 
I would like to hear feedback and get some review.

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1833 bytes --]

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2467,7 +2467,7 @@ static unsigned long ata_pio_poll(struct
  *	None.  (executing in kernel thread context)
  */
 
-static void ata_pio_complete (struct ata_port *ap)
+static int ata_pio_complete (struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
 	u8 drv_stat;
@@ -2486,14 +2486,14 @@ static void ata_pio_complete (struct ata
 		if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
 			ap->pio_task_state = PIO_ST_LAST_POLL;
 			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return;
+			return 0;
 		}
 	}
 
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
 		ap->pio_task_state = PIO_ST_ERR;
-		return;
+		return 0;
 	}
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -2502,6 +2502,10 @@ static void ata_pio_complete (struct ata
 	ap->pio_task_state = PIO_ST_IDLE;
 
 	ata_poll_qc_complete(qc, drv_stat);
+
+	/* another command may start at this point */
+
+	return 1;
 }
 
 
@@ -2887,7 +2891,12 @@ static void ata_pio_error(struct ata_por
 static void ata_pio_task(void *_data)
 {
 	struct ata_port *ap = _data;
-	unsigned long timeout = 0;
+	unsigned long timeout;
+	int qc_completed;
+
+fsm_start:
+	timeout = 0;
+	qc_completed = 0;
 
 	switch (ap->pio_task_state) {
 	case PIO_ST_IDLE:
@@ -2898,7 +2907,7 @@ static void ata_pio_task(void *_data)
 		break;
 
 	case PIO_ST_LAST:
-		ata_pio_complete(ap);
+		qc_completed = ata_pio_complete(ap);
 		break;
 
 	case PIO_ST_POLL:
@@ -2915,8 +2924,8 @@ static void ata_pio_task(void *_data)
 	if (timeout)
 		queue_delayed_work(ata_wq, &ap->pio_task,
 				   timeout);
-	else
-		queue_work(ata_wq, &ap->pio_task);
+	else if (!completed)
+		goto fsm_start;
 }
 
 static void atapi_request_sense(struct ata_port *ap, struct ata_device *dev,

Re: [PATCH 0/2] libata: ata_pio_task() accessing 'ap->pio_task_state' fix

[Albert Lee]

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

Jeff,

> Albert Lee wrote:
>
>> Jeff,
>>
>> During PIO, after calling ata_poll_qc_complete(), the next command 
>> might be
>> running and the value of 'ap->pio_task_state' might have been changed.
>> Accessing 'ap->pio_task_state' is not safe at this point.
>>
>> Ex.
>> qc 1 completed. queuing a final task with ap->pio_task_state == 
>> PIO_ST_IDLE.
>> qc 2 started, queuing a new task with ap->pio_task_state set to PIO_ST.
>> qc 1 read ap->pio_task_state as PIO_ST; not PIO_ST_IDLE as expected.
>> => 2 qc running in the workqueue with pio_task_state PIO_ST.
>>
>> Changes:
>>  1/2: Modify ata_pio_complete() and ata_pio_block() to return
>>       whether qc has been completed.
>>
>>  2/2: Modify ata_pio_task() to check the return value. Only queue 
>> next step and
>>        access 'ap->pio_task_state' if the command is not completed.
>
>
> I would prefer something more like the attached patch.  This patch 
> does two things:
> * eliminate needless queueing
> * don't race with qc-complete in ata_pio_complete()

The patch looks good and clearer. :)

Only one minor addition for your review:

* ata_pio_block() changed to go to PIO_ST_LAST state, instead of
   going to PIO_ST_IDLE state directly and calling ata_poll_qc_complete().

i.e.
@@ -2845,9 +2852,7 @@
     if (is_atapi_taskfile(&qc->tf)) {
         /* no more data to transfer or unsupported ATAPI command */
         if ((status & ATA_DRQ) == 0) {
-            ap->pio_task_state = PIO_ST_IDLE;
-
-            ata_poll_qc_complete(qc, status);
+            ap->pio_task_state = PIO_ST_LAST;
             return;
         }

This is needed since ata_pio_block() might complete the qc.
This can also make the pio polling code go through the 
ata_pio_complete(), making the state transition explicit.

Attached please find the revised patch for your review.
(Patched for 2.6.13  80ac2912f846c01d702774bb6aa7100ec71e88b9).

>
> This patch is COMPLETELY UNTESTED.  Please use the attached as a base, 
> to replace the patch series you have submitted.  I won't check it in, 
> as I would like to hear feedback and get some review.
>

Tested ok on x86 PC. Will test on the big machines later.

Albert

(Revision based on Jeff's patch)
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>




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

--- linux/drivers/scsi/libata-core.c.ori	2005-09-02 17:59:15.000000000 +0800
+++ pio_poll/drivers/scsi/libata-core.c	2005-09-07 16:18:00.000000000 +0800
@@ -2461,9 +2461,12 @@
  *
  *	LOCKING:
  *	None.  (executing in kernel thread context)
+ *
+ *	RETURNS:
+ *	Non-zero if qc completed, zero otherwise.
  */
 
-static void ata_pio_complete (struct ata_port *ap)
+static int ata_pio_complete (struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
 	u8 drv_stat;
@@ -2482,14 +2485,14 @@
 		if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
 			ap->pio_task_state = PIO_ST_LAST_POLL;
 			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return;
+			return 0;
 		}
 	}
 
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
 		ap->pio_task_state = PIO_ST_ERR;
-		return;
+		return 0;
 	}
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -2498,6 +2501,10 @@
 	ap->pio_task_state = PIO_ST_IDLE;
 
 	ata_poll_qc_complete(qc, drv_stat);
+
+	/* another command may start at this point */
+
+	return 1;
 }
 
 
@@ -2845,9 +2852,7 @@
 	if (is_atapi_taskfile(&qc->tf)) {
 		/* no more data to transfer or unsupported ATAPI command */
 		if ((status & ATA_DRQ) == 0) {
-			ap->pio_task_state = PIO_ST_IDLE;
-
-			ata_poll_qc_complete(qc, status);
+			ap->pio_task_state = PIO_ST_LAST;
 			return;
 		}
 
@@ -2883,7 +2888,12 @@
 static void ata_pio_task(void *_data)
 {
 	struct ata_port *ap = _data;
-	unsigned long timeout = 0;
+	unsigned long timeout;
+	int qc_completed;
+
+fsm_start:
+	timeout = 0;
+	qc_completed = 0;
 
 	switch (ap->pio_task_state) {
 	case PIO_ST_IDLE:
@@ -2894,7 +2904,7 @@
 		break;
 
 	case PIO_ST_LAST:
-		ata_pio_complete(ap);
+		qc_completed = ata_pio_complete(ap);
 		break;
 
 	case PIO_ST_POLL:
@@ -2909,10 +2919,9 @@
 	}
 
 	if (timeout)
-		queue_delayed_work(ata_wq, &ap->pio_task,
-				   timeout);
-	else
-		queue_work(ata_wq, &ap->pio_task);
+		queue_delayed_work(ata_wq, &ap->pio_task, timeout);
+	else if (likely(!qc_completed))
+		goto fsm_start;
 }
 
 static void atapi_request_sense(struct ata_port *ap, struct ata_device *dev,

Re: [PATCH 0/2] libata: ata_pio_task() accessing 'ap->pio_task_state' fix

[Jeff Garzik]

Albert Lee wrote:
> Only one minor addition for your review:
> 
> * ata_pio_block() changed to go to PIO_ST_LAST state, instead of
>   going to PIO_ST_IDLE state directly and calling ata_poll_qc_complete().
> 
> i.e.
> @@ -2845,9 +2852,7 @@
>     if (is_atapi_taskfile(&qc->tf)) {
>         /* no more data to transfer or unsupported ATAPI command */
>         if ((status & ATA_DRQ) == 0) {
> -            ap->pio_task_state = PIO_ST_IDLE;
> -
> -            ata_poll_qc_complete(qc, status);
> +            ap->pio_task_state = PIO_ST_LAST;
>             return;
>         }

hmmmm.  I think that should be PIO_ST_ERR not PIO_ST_LAST.  Comments?

	Jeff

Re: [PATCH 0/2] libata: ata_pio_task() accessing 'ap->pio_task_state' fix

[Albert Lee]

Jeff Garzik wrote:

> Albert Lee wrote:
>
>> Only one minor addition for your review:
>>
>> * ata_pio_block() changed to go to PIO_ST_LAST state, instead of
>>   going to PIO_ST_IDLE state directly and calling 
>> ata_poll_qc_complete().
>>
>> i.e.
>> @@ -2845,9 +2852,7 @@
>>     if (is_atapi_taskfile(&qc->tf)) {
>>         /* no more data to transfer or unsupported ATAPI command */
>>         if ((status & ATA_DRQ) == 0) {
>> -            ap->pio_task_state = PIO_ST_IDLE;
>> -
>> -            ata_poll_qc_complete(qc, status);
>> +            ap->pio_task_state = PIO_ST_LAST;
>>             return;
>>         }
>
>
> hmmmm.  I think that should be PIO_ST_ERR not PIO_ST_LAST.  Comments?
>

Just tested it. Changing to PIO_ST_ERR will break REQUEST_SENSE and 
MODE_SENSE. :(
For REQUEST_SENSE and MODE_SENSE, the data returned from the device might be
less than the buffer provided and those two commands relies on DRQ == 0 
to mark the end of
the data transfer.

e.g. a sample REQUST SENSE transaction:
 1. ata_pio_task() entered with PIO_ST.
 2. ata_pio_block() called, 24 bytes received from the device.
 3. The buffer size is 96 bytes, so the state is kept as PIO_ST.
 4. ata_pio_task() entered with PIO_ST.
 5. ata_pio_block() called, DRQ == 0 to mark the end of data transfer.
 6. state changed to PIO_ST_LAST and command completed as successful.

DRQ == 0 in step 5 seems to be normal and should be treated as OK.

Albert