Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

On 08/09/2013 06:03 PM, Tejun Heo wrote:

>> From: Marc Carino <marc.ceeeee@gmail.com>

>> SATA 3.1 added an "auxiliary" field to the host-to-device FIS.

>> Since there is no analog between the new field and the ATA
>> taskfile, a new element was added to 'struct ata_queued_cmd."

> Ummm... I'm sorry but I'm gonna do 180 from Sergei on this one.  The
> auxiliary field is part of ata taskfile for all intents and purposes.

    Which would be another abuse of the ATA taskfile term (reminds me of SFF 
abuse wehre everybody hushed me down saying that it's too late to change 
anything and for all intenets and purposes SFF means "taskfile based" for 
everybody). As Marc rightly said, this field won't be deliverable by the usual 
taskfile methods. Moreover, it won't be used in qc->result_tf' and so will 
waste 4 bytes for no reason.

> FIS is the new command structure anyway and struct ata_taskfile proper
> should be able to describe the command with ata_queuedcmd providing
> the surrounding context.

    It depends on your definition of surrounding context. In my definition, 
'flags' and 'protocol' fields should be a part of surrounding context. 'ctl' 
too sicen the device control register was never considered a part of the ATA 
taskfile and it's not used in 'qc->result_tf' (as well as the other two 
fields), hence just wasting memory for no reason.

> The argument that ata_taskfile shouldn't
> contain anything which wasn't in PATA taskfile is bogus as it already
> contains ATA_TFLAG_*.

    I regret that I haven't done my patches earlier to get rid of this example 
of abuse of the ATA taskfile before.

> So, please put the aux field into ata_taskfile.  That's where it
> belongs.

    Sigh.

> Thanks.

WBR, Sergei