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>