Re: [PATCH 13/22] libata: implement qc->result_tf

[Tejun Heo <htejun@gmail.com>]

Albert Lee wrote:
> Tejun Heo wrote:
>> Albert Lee wrote:
>>
>>> This patch makes qc->tf well preserved, except we still have
>>> qc->tf got overwritten by ata_pio_bytes().
>>>
>>> Maybe we also need the attached follow-up patch?
>>>
>>> -- 
>>> albert
>>>
>>> --- 02_atapi_pio_bytes/drivers/scsi/libata-core.c    2006-05-18
>>> 14:20:32.000000000 +0800
>>> +++ 03_atapi_pio_bytes/drivers/scsi/libata-core.c    2006-05-18
>>> 14:25:41.000000000 +0800
>>> @@ -3861,10 +3861,10 @@ static void atapi_pio_bytes(struct ata_q
>>>      unsigned int ireason, bc_lo, bc_hi, bytes;
>>>      int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
>>>  
>>> -    ap->ops->tf_read(ap, &qc->tf);
>>> -    ireason = qc->tf.nsect;
>>> -    bc_lo = qc->tf.lbam;
>>> -    bc_hi = qc->tf.lbah;
>>> +    ap->ops->tf_read(ap, &qc->result_tf);
>>> +    ireason = qc->result_tf.nsect;
>>> +    bc_lo = qc->result_tf.lbam;
>>> +    bc_hi = qc->result_tf.lbah;
>>>      bytes = (bc_hi << 8) | bc_lo;
>>>  
>>>      /* shall be cleared to zero, indicating xfer of data */
>>>
>> I actually think above should be done with local variable.  It's not
>> comannd TF nor result TF.
>>
> 
> You're right. But local variable adds 18+ extra bytes to the kernel stack.
> And
> - For normal completion, qc->result_tf is not relevant.
> - For error, qc->result_tf is later overwritten by ata_qc_complete().
> Maybe we can use qc->result_tf for temp storage of intermediate TF here
> and save some stack usage?

Yeah, maybe.  But, please note in the code that the use is not to record 
result.

-- 
tejun