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

[Albert Lee <albertcc@tw.ibm.com>]

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