Re: [PATCH v1] Fix sector number translation in sense buffer for SATA drives

[Dan Williams <dan.j.williams@xxxxxxxxx>]

On Thu, Jun 21, 2012 at 5:23 PM, Praveen Murali <pmurali@xxxxxxxxxxxx> wrote:
> On Thu, 2012-06-21 at 17:04 -0700, Dan Williams wrote:
>> On Thu, Jun 21, 2012 at 4:42 PM, Praveen Murali <pmurali@xxxxxxxxxxxx> wrote:
>> > For my large capacity drives connected to my SAS HBA i get improper
>> > sector values when i use hdparm to get the native max. Also, the drives
>> > have known bad sectors, but when read fails, the sector information
>> > printed in the sense buffer is incorrect.
>> >  In fill_result_tf() of libata-core.c the flags for the resultant tf is
>> > copied from the request tf before calling the qc_fill_rtf() for the
>> > driver. In case of SAS we convert the FIS to tf after receiving the
>> > response and memcpy this tf over to the result tf in
>> > sas_ata_qc_fill_rtf(); which results in overwriting the flags
>> > for rtf. As a result, when ata_tf_read_block() is called by
>> > ata_gen_ata_sense() ata_tf_read_block() does not find ATA_TFLAG_LBA or
>> > ATA_TFLAG_LBA48 in the rtf flags. Hence, the returned block value is not
>> > calculated correctly.
>> >
>> > The fix sets the flags in result tf from tf after calling qc_fill_rtf()
>> > to take care of overwrites (as in libsas). Also, if the overwrite was
>> > intentional, we will only setup the right bits here.
>>
>> Hmm, I think libsas is at fault here and should be using
>> ata_tf_from_fis() like the other ->qc_fill_rtf() implementations.
> libsas calls ata_tf_from_fis in sas_ata.c sas_ata_task_done() to copy
> the result FIS. But then it copies this information to sata_dev.tf in
> the domain device. It mem copies this (sata_dev.tf) over to the rtf in
> sas_ata_qc_fill_rtf().
>  Another way to fix this might be to save the flags in sata_dev.tf from
> qc->tf in sas_ata_task_done() before/after it calls ata_tf_from_fis().
> This was it can still keep doing the mem copy and the order in libata
> need not be changed.
> What do you think?

tf->flags is an internal detail of libata to record context about the
in-flight command.  So just fixing sas_ata_qc_fill_rtf() to not
trample on the non-fis fields in the tf should be all that is needed,
i.e. just a copy from the original qc.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html