Re: [PATCH] libata: add TRIM support

[Jeff Garzik <jgarzik@pobox.com>]

On 11/17/2009 09:32 AM, Christoph Hellwig wrote:
> On Mon, Nov 16, 2009 at 10:34:36PM -0500, Jeff Garzik wrote:
>> args->id[] access should be via ata_id_* functions from include/linux/ata.h.
>>
>> Create new ata_id_* as needed.
>
> Fixed.
>
>>> +	tf->lbam = 0;
>>> +	tf->hob_lbah = 0;
>>> +	tf->lbah = 0;
>>
>> taskfile is pre-zeroed for you (ata_scsi_qc_new ->  ata_qc_new_init ->
>> ata_qc_reinit ->  ata_tf_init), so zap all those zeroing lines.
>
> Thanks, tried to figure out if it was but after going a few levels deep
> I gave up.

Understandable ;-)


>>> +	tf->device = ATA_LBA;
>>
>> __do not__ overwrite tf->device value.  It is already assigned a useful
>> value, which you just stomped.
>>
>>
>>> +	tf->device = dev->devno ?
>>> +		tf->device | ATA_DEV1 : tf->device&   ~ATA_DEV1;
>>
>> delete this; already done in ata_tf_init()
>>
>>
>>> +	qc->sect_size = ATA_SECT_SIZE;
>>
>> delete this; already done in ata_qc_reinit()
>
> All this is copy and paste from ata_scsi_pass_thru, but I'll happily fix
> it up.

Thanks.

ata_scsi_pass_thru() isn't the best model, since it assumes some 
registers -- notably Device -- are accurately filled in by userland. 
That is the purpose of the pass-through "command," after all.

Pretty much all other sources use the tf->device provided by 
ata_tf_init(), OR'd as appropriate with additional bits.

	Jeff