[BUG] libahci returns stale result tf much of the time.

[BUG] libahci returns stale result tf much of the time.

[Mark Lord]

I've been trying to understand numerous bug reports I've seen for hdparm
over the past couple of months.  I think they probably all boil down to
the new libahci code not returning a correct result_tf in response
to successful ATA_16/ATA_12 passthru commands.

It seems to return a stale result_tf much of the time.
perfectly reproduceable here on my Jmicron JMB360 PCIe SATA interface.

I am totally lost when looking at libahci code though.
Anyone got any ideas where the bug might be there??

Thanks

Re: [BUG] libahci returns stale result tf much of the time.

[Seed]

010 at 3:34 PM, Mark Lord <kernel@teksavvy.com> wrote:
> I am totally lost when looking at libahci code though.
> Anyone got any ideas where the bug might be there??

Maybe in libata-scsi.c function ata_scsi_pass_thru?

--

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-09-24 01:49 AM, Seed wrote:
> 010 at 3:34 PM, Mark Lord<kernel@teksavvy.com>  wrote:
>> I am totally lost when looking at libahci code though.
>> Anyone got any ideas where the bug might be there??
>
> Maybe in libata-scsi.c function ata_scsi_pass_thru?


No, things are fine there, and work properly for non-AHCI chipsets.
The bug seems to be in the libahci code somewhere.

Cheers

Re: [BUG] libahci returns stale result tf much of the time.

[Seed]

On Fri, Sep 24, 2010 at 4:27 PM, Mark Lord <kernel@teksavvy.com> wrote:
> The bug seems to be in the libahci code somewhere.

ahci_qc_fill_rtf seems to be the function converting FIS to result_tf.

--

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-09-24 03:01 AM, Seed wrote:
> On Fri, Sep 24, 2010 at 4:27 PM, Mark Lord<kernel@teksavvy.com>  wrote:
>> The bug seems to be in the libahci code somewhere.
>
> ahci_qc_fill_rtf seems to be the function converting FIS to result_tf.
..

Yeah, that's as far as I got with it last night.
If you apply this patch (below), it makes the problem more "obvious",
though I still don't see exactly how that struct gets updated.

--- a/drivers/ata/libahci.c	2010-09-24 02:40:24.722887047 -0400
+++ b/drivers/ata/libahci.c	2010-09-24 09:10:21.761520099 -0400
@@ -1838,6 +1838,7 @@
  		d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
  
  	ata_tf_from_fis(d2h_fis, &qc->result_tf);
+	memset(d2h_fis, 0xfd, AHCI_RX_FIS_SZ);
  	return true;
  }
  

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-09-24 09:11 AM, Mark Lord wrote:
> On 10-09-24 03:01 AM, Seed wrote:
>> On Fri, Sep 24, 2010 at 4:27 PM, Mark Lord<kernel@teksavvy.com> wrote:
>>> The bug seems to be in the libahci code somewhere.
>>
>> ahci_qc_fill_rtf seems to be the function converting FIS to result_tf.
> ..
>
> Yeah, that's as far as I got with it last night.
> If you apply this patch (below), it makes the problem more "obvious",
> though I still don't see exactly how that struct gets updated.
>
> --- a/drivers/ata/libahci.c 2010-09-24 02:40:24.722887047 -0400
> +++ b/drivers/ata/libahci.c 2010-09-24 09:10:21.761520099 -0400
> @@ -1838,6 +1838,7 @@
> d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
>
> ata_tf_from_fis(d2h_fis, &qc->result_tf);
> + memset(d2h_fis, 0xfd, AHCI_RX_FIS_SZ);
> return true;
> }
>
..

And here's an example of the bug, which should work (as a demo)
for most folks out there with the same controller ahci / JMB360:

Here, I'll use hdparm to do a "set acoustic" command
on a drive which does NOT have the "Acoustic Management" feature set.
Just look for the fd fd fd strings in the returned data,
and notice how the final IDENTIFY at the end works, but returns
bad ATA status 0x51 from the stale result_tf:

[~] hdparm --verbose -M128 /dev/sdb

/dev/sdb:
  setting acoustic management to 128
outgoing cdb:  85 06 20 00 42 00 80 00 00 00 00 00 00 40 ef 00
SG_IO: ATA_16 status=0x2, host_status=0x0, driver_status=0x8
SG_IO: sb[]:  72 00 00 00 00 00 00 0e 09 0c 00 00 00 80 00 00 00 00 00 00 e0 50 00 00 00 00 00 00 00 00 00 00
SG_IO: desc[]:  09 0c 00 00 00 80 00 00 00 00 00 00 e0 50
       ATA_16 stat=50 err=00 nsect=80 lbal=00 lbam=00 lbah=00 dev=e0
outgoing cdb:  85 08 2e 00 00 00 00 00 00 00 00 00 00 40 ec 00
SG_IO: ATA_16 status=0x2, host_status=0x0, driver_status=0x8
SG_IO: sb[]:  72 0b 47 00 00 00 00 0e 09 0c 00 fd 00 fd 00 fd 00 fd 00 fd fd fd 00 00 00 00 00 00 00 00 00 00
incoming_data:  5a 04 ff 3f c8 37 10 00 00 00 00 00 3f 00 00 00 00 00 00 00 20 20 20 20 20 20 4e 56 43 36 48 4d 43 43 54 44 32 59 44 4e 03 00 04 3e 04 00 33 56 4f 33 36 41 41 4d 44 48 37 53 32 32 31 35 56 32 53 4c 38 41 20 30 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 10 80 00 00 00 2f 00 40 00 02 00 02 07 00 ff 3f 10 00 3f 00 10 fc fb 00 00 01 40 41 61 0e 00 00 07 00 03 00 78 00 78 00 f0 00 78 00 00 00 00 00 00 00 00 00 00 00 00 00 1f 00 02 00 00 00 00 00 00 00 7c 00 19 00 eb 74 ea 7f 23 40 e9 74 02 3e 23 40 3f 20 21 00 00 00 00 00 fe ff 00 00 80 80 00 00 00 00 00 00 00 00 00 00 40 41 61 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 0b 00 00 0
 0 00 00 82 3a b1 0c 60 fe 01 00 00 40 00 00 00 00 00 00 00 00 f7 01 04 2a 00 14 00 04 80 02 7f 3f c0 00 40 00 00 ac 00 80 00 00 4f 33 36 43 00 00 13 c0 00 00 00 00 00 00 00 00 00 00 00 00 00
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a5 c0
SG_IO: desc[]:  09 0c 00 fd 00 fd 00 fd 00 fd 00 fd fd fd
       ATA_16 stat=fd err=fd nsect=fd lbal=fd lbam=fd lbah=fd dev=fd
I/O error, ata_op=0xec ata_status=0xfd ata_error=0xfd
outgoing cdb:  85 08 2e 00 00 00 00 00 00 00 00 00 00 40 a1 00
SG_IO: ATA_16 status=0x2, host_status=0x0, driver_status=0x8
SG_IO: sb[]:  72 0b 00 00 00 00 00 0e 09 0c 00 04 00 00 00 00 00 00 00 00 e0 51 00 00 00 00 00 00 00 00 00 00
incoming_data:  5a 04 ff 3f c8 37 10 00 00 00 00 00 3f 00 00 00 00 00 00 00 20 20 20 20 20 20 4e 56 43 36 48 4d 43 43 54 44 32 59 44 4e 03 00 04 3e 04 00 33 56 4f 33 36 41 41 4d 44 48 37 53 32 32 31 35 56 32 53 4c 38 41 20 30 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 10 80 00 00 00 2f 00 40 00 02 00 02 07 00 ff 3f 10 00 3f 00 10 fc fb 00 00 01 40 41 61 0e 00 00 07 00 03 00 78 00 78 00 f0 00 78 00 00 00 00 00 00 00 00 00 00 00 00 00 1f 00 02 00 00 00 00 00 00 00 7c 00 19 00 eb 74 ea 7f 23 40 e9 74 02 3e 23 40 3f 20 21 00 00 00 00 00 fe ff 00 00 80 80 00 00 00 00 00 00 00 00 00 00 40 41 61 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 0b 00 00 0
 0 00 00 82 3a b1 0c 60 fe 01 00 00 40 00 00 00 00 00 00 00 00 f7 01 04 2a 00 14 00 04 80 02 7f 3f c0 00 40 00 00 ac 00 80 00 00 4f 33 36 43 00 00 13 c0 00 00 00 00 00 00 00 00 00 00 00 00 00
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a5 c0
SG_IO: desc[]:  09 0c 00 04 00 00 00 00 00 00 00 00 e0 51
       ATA_16 stat=51 err=04 nsect=00 lbal=00 lbam=00 lbah=00 dev=e0
I/O error, ata_op=0xa1 ata_status=0x51 ata_error=0x04
  HDIO_DRIVE_CMD(identify) failed: Input/output error

Re: [BUG] libahci returns stale result tf much of the time.

[Robert Hancock]

On 09/24/2010 07:24 AM, Mark Lord wrote:
> On 10-09-24 09:11 AM, Mark Lord wrote:
>> On 10-09-24 03:01 AM, Seed wrote:
>>> On Fri, Sep 24, 2010 at 4:27 PM, Mark Lord<kernel@teksavvy.com> wrote:
>>>> The bug seems to be in the libahci code somewhere.
>>>
>>> ahci_qc_fill_rtf seems to be the function converting FIS to result_tf.
>> ..
>>
>> Yeah, that's as far as I got with it last night.
>> If you apply this patch (below), it makes the problem more "obvious",
>> though I still don't see exactly how that struct gets updated.
>>
>> --- a/drivers/ata/libahci.c 2010-09-24 02:40:24.722887047 -0400
>> +++ b/drivers/ata/libahci.c 2010-09-24 09:10:21.761520099 -0400
>> @@ -1838,6 +1838,7 @@
>> d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
>>
>> ata_tf_from_fis(d2h_fis, &qc->result_tf);
>> + memset(d2h_fis, 0xfd, AHCI_RX_FIS_SZ);
>> return true;
>> }
>>
> ..
>
> And here's an example of the bug, which should work (as a demo)
> for most folks out there with the same controller ahci / JMB360:
>
> Here, I'll use hdparm to do a "set acoustic" command
> on a drive which does NOT have the "Acoustic Management" feature set.
> Just look for the fd fd fd strings in the returned data,
> and notice how the final IDENTIFY at the end works, but returns
> bad ATA status 0x51 from the stale result_tf:

The d2h_fis area is supposed to be updated by the controller with the 
last FIS received from the device. Maybe this controller just isn't 
doing that for some reason?

Re: [BUG] libahci returns stale result tf much of the time.

[Tejun Heo]

Hello,

On 09/25/2010 01:26 AM, Robert Hancock wrote:
>> And here's an example of the bug, which should work (as a demo)
>> for most folks out there with the same controller ahci / JMB360:
>>
>> Here, I'll use hdparm to do a "set acoustic" command
>> on a drive which does NOT have the "Acoustic Management" feature set.
>> Just look for the fd fd fd strings in the returned data,
>> and notice how the final IDENTIFY at the end works, but returns
>> bad ATA status 0x51 from the stale result_tf:
> 
> The d2h_fis area is supposed to be updated by the controller with
> the last FIS received from the device. Maybe this controller just
> isn't doing that for some reason?

Hmm... one possibility is that the controller takes some time to
update the area and the driver is reading it off too early.  Maybe
adding a delay would resolve the issue?  Mark, do you know whether
this problem is isolated to JMB360?

Thanks.

-- 
tejun

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-10-04 05:18 AM, Tejun Heo wrote:
> Hello,
>
> On 09/25/2010 01:26 AM, Robert Hancock wrote:
>>> And here's an example of the bug, which should work (as a demo)
>>> for most folks out there with the same controller ahci / JMB360:
>>>
>>> Here, I'll use hdparm to do a "set acoustic" command
>>> on a drive which does NOT have the "Acoustic Management" feature set.
>>> Just look for the fd fd fd strings in the returned data,
>>> and notice how the final IDENTIFY at the end works, but returns
>>> bad ATA status 0x51 from the stale result_tf:
>>
>> The d2h_fis area is supposed to be updated by the controller with
>> the last FIS received from the device. Maybe this controller just
>> isn't doing that for some reason?
>
> Hmm... one possibility is that the controller takes some time to
> update the area and the driver is reading it off too early.  Maybe
> adding a delay would resolve the issue?  Mark, do you know whether
> this problem is isolated to JMB360?
..

That's my theory, too (slow updating of the area).
I haven't pursued it further yet, but I will.

This is really disruptive for me here, as my primary eSATA
adaptor in my notebook is JMB360, and it gets used a LOT.

Cheers

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-10-04 01:01 PM, Mark Lord wrote:
> On 10-10-04 05:18 AM, Tejun Heo wrote:
>> Hello,
>>
>> On 09/25/2010 01:26 AM, Robert Hancock wrote:
>>>> And here's an example of the bug, which should work (as a demo)
>>>> for most folks out there with the same controller ahci / JMB360:
>>>>
>>>> Here, I'll use hdparm to do a "set acoustic" command
>>>> on a drive which does NOT have the "Acoustic Management" feature set.
>>>> Just look for the fd fd fd strings in the returned data,
>>>> and notice how the final IDENTIFY at the end works, but returns
>>>> bad ATA status 0x51 from the stale result_tf:
>>>
>>> The d2h_fis area is supposed to be updated by the controller with
>>> the last FIS received from the device. Maybe this controller just
>>> isn't doing that for some reason?
>>
>> Hmm... one possibility is that the controller takes some time to
>> update the area and the driver is reading it off too early. Maybe
>> adding a delay would resolve the issue? Mark, do you know whether
>> this problem is isolated to JMB360?
> ..
>
> That's my theory, too (slow updating of the area).
> I haven't pursued it further yet, but I will.
>
> This is really disruptive for me here, as my primary eSATA
> adaptor in my notebook is JMB360, and it gets used a LOT.

Okay, I just now ran a quick test on another machine with AHCI: Same bug:

00:1f.2 SATA controller: Intel Corporation 82801HR/HO/HH (ICH8R/DO/DH) 6 port SATA AHCI Controller (rev 02)

So it's probably universal.  Try it with this simple test:

hdparm -I /dev/sdX   ## this works
hdparm -Z /dev/sdX   ## this will fail --> obsolete vendor-specific command
hdparm -I /dev/sdX   ## this fails when following the above

hdparm -z /dev/sdX   ## force some regular I/O, to clear the bad state from the driver
hdparm -I /dev/sdX   ## this now works again

Cheers
-ml

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-10-04 01:06 PM, Mark Lord wrote:
..
>> That's my theory, too (slow updating of the area).
>> I haven't pursued it further yet, but I will.
>>
>> This is really disruptive for me here, as my primary eSATA
>> adaptor in my notebook is JMB360, and it gets used a LOT.
>
> Okay, I just now ran a quick test on another machine with AHCI: Same bug:
>
> 00:1f.2 SATA controller: Intel Corporation 82801HR/HO/HH (ICH8R/DO/DH) 6 port SATA AHCI Controller (rev 02)
>
> So it's probably universal. Try it with this simple test:
>
> hdparm -I /dev/sdX ## this works
> hdparm -Z /dev/sdX ## this will fail --> obsolete vendor-specific command
> hdparm -I /dev/sdX ## this fails when following the above
>
> hdparm -z /dev/sdX ## force some regular I/O, to clear the bad state from the driver
> hdparm -I /dev/sdX ## this now works again


The issue also seems to be present in 2.6.32.8 on that same Intel AHCI platform,
so it probably has nothing to do with the libahci split.

Cheers

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-10-04 01:31 PM, Mark Lord wrote:
> On 10-10-04 01:06 PM, Mark Lord wrote:
> ..
>>> That's my theory, too (slow updating of the area).
>>> I haven't pursued it further yet, but I will.
>>>
>>> This is really disruptive for me here, as my primary eSATA
>>> adaptor in my notebook is JMB360, and it gets used a LOT.
>>
>> Okay, I just now ran a quick test on another machine with AHCI: Same bug:
>>
>> 00:1f.2 SATA controller: Intel Corporation 82801HR/HO/HH (ICH8R/DO/DH) 6 port SATA AHCI Controller (rev 02)
>>
>> So it's probably universal. Try it with this simple test:
>>
>> hdparm -I /dev/sdX ## this works
>> hdparm -Z /dev/sdX ## this will fail --> obsolete vendor-specific command
>> hdparm -I /dev/sdX ## this fails when following the above
>>
>> hdparm -z /dev/sdX ## force some regular I/O, to clear the bad state from the driver
>> hdparm -I /dev/sdX ## this now works again
>
>
> The issue also seems to be present in 2.6.32.8 on that same Intel AHCI platform,
> so it probably has nothing to do with the libahci split.


I've dug a bit deeper, and worked around the issue.
It seems that libata is not always happy about DATA-xfer commands
that also have the check-sense bit set (0x20 in cdb[2]).
Perhaps that's not even valid in SCSI ??

Anyway, I'm updating hdparm to only set that bit for non-DATA commands,
and leave it unset for data commands (eg. IDENTIFY).
That's how libata itself does things internally.

Seems to fix whatever the issue was.

Cheers

Re: [BUG] libahci returns stale result tf much of the time.

[Jeff Garzik]

On 10/04/2010 02:50 PM, Mark Lord wrote:
> I've dug a bit deeper, and worked around the issue.
> It seems that libata is not always happy about DATA-xfer commands
> that also have the check-sense bit set (0x20 in cdb[2]).
> Perhaps that's not even valid in SCSI ??


Very interesting...  poking at this now on pre- and post-libahci 
platforms.  As you said, don't see much difference in behavior WRT 
libahci -- which I expected/hoped, since that split shouldn't have 
changed behavior at all[1].

	Jeff


[1] though clearly there were bugs, eg. the SHT issue, so nothing's out 
of the question

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-10-04 03:27 PM, Jeff Garzik wrote:
> On 10/04/2010 02:50 PM, Mark Lord wrote:
>> I've dug a bit deeper, and worked around the issue.
>> It seems that libata is not always happy about DATA-xfer commands
>> that also have the check-sense bit set (0x20 in cdb[2]).
>> Perhaps that's not even valid in SCSI ??
>
>
> Very interesting... poking at this now on pre- and post-libahci platforms. As you said, don't see much difference in behavior WRT libahci -- which I expected/hoped, since that split shouldn't have changed behavior at all[1].


Yeah.  Non-data commands still get a (correct) updated result_tf from AHCI,
but data commands don't get one, unless they fail.

Weird, but I've worked around it now.

Cheers

Re: [BUG] libahci returns stale result tf much of the time.

[Jeff Garzik]

On 10/04/2010 03:35 PM, Mark Lord wrote:
> On 10-10-04 03:27 PM, Jeff Garzik wrote:
>> On 10/04/2010 02:50 PM, Mark Lord wrote:
>>> I've dug a bit deeper, and worked around the issue.
>>> It seems that libata is not always happy about DATA-xfer commands
>>> that also have the check-sense bit set (0x20 in cdb[2]).
>>> Perhaps that's not even valid in SCSI ??
>>
>>
>> Very interesting... poking at this now on pre- and post-libahci
>> platforms. As you said, don't see much difference in behavior WRT
>> libahci -- which I expected/hoped, since that split shouldn't have
>> changed behavior at all[1].
>
>
> Yeah. Non-data commands still get a (correct) updated result_tf from AHCI,
> but data commands don't get one, unless they fail.
>
> Weird, but I've worked around it now.

Might be that we just get a single-bit "OK" notification from hardware 
for successfully completed commands, a la NCQ's SDB FIS.

Which opcodes are you using?  I see set-acoustic-mgmt in one email...

	Jeff

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-10-04 03:42 PM, Jeff Garzik wrote:
> On 10/04/2010 03:35 PM, Mark Lord wrote:
>> Yeah. Non-data commands still get a (correct) updated result_tf from AHCI,
>> but data commands don't get one, unless they fail.
>>
>> Weird, but I've worked around it now.
>
> Might be that we just get a single-bit "OK" notification from hardware for successfully completed commands, a la NCQ's SDB FIS.
>
> Which opcodes are you using? I see set-acoustic-mgmt in one email...


opcode 0xec : IDENTIFY

The result_tf it receives is from whatever non-data command last preceeds it.
So if the previous non-data command failed (eg. hdparm -Z),
then the IDENTIFY command actually works, but appears to fail.

Cheers

Re: [BUG] libahci returns stale result tf much of the time.

[Tejun Heo]

Hello,

On 10/04/2010 09:51 PM, Mark Lord wrote:
>> Which opcodes are you using? I see set-acoustic-mgmt in one email...
> 
> opcode 0xec : IDENTIFY
> 
> The result_tf it receives is from whatever non-data command last preceeds it.
> So if the previous non-data command failed (eg. hdparm -Z),
> then the IDENTIFY command actually works, but appears to fail.

I've been thinking about it and it occurred to me that PIO data
commands don't use D2H Reg FIS for command completion.  I don't recall
exactly but the PIO Setup FIS contains the result register, right?  It
seems like we should look at different places or simulate output
depending on the command executed.  Hmmm....

Thanks.

-- 
tejun

Re: [BUG] libahci returns stale result tf much of the time.

[Mark Lord]

On 10-10-05 03:07 AM, Tejun Heo wrote:
> Hello,
>
> On 10/04/2010 09:51 PM, Mark Lord wrote:
>>> Which opcodes are you using? I see set-acoustic-mgmt in one email...
>>
>> opcode 0xec : IDENTIFY
>>
>> The result_tf it receives is from whatever non-data command last preceeds it.
>> So if the previous non-data command failed (eg. hdparm -Z),
>> then the IDENTIFY command actually works, but appears to fail.
>
> I've been thinking about it and it occurred to me that PIO data
> commands don't use D2H Reg FIS for command completion.  I don't recall
> exactly but the PIO Setup FIS contains the result register, right?  It
> seems like we should look at different places or simulate output
> depending on the command executed.  Hmmm....
..

Yeah, maybe something like that.

But do note that I have verified that "hdparm -C" does work correctly,
and is dependent upon correctly returned result_tf values.

It's not a PIO "data" command, though.  Does it get issued the same way?

Re: [BUG] libahci returns stale result tf much of the time.

[Tejun Heo]

Hello,

On 10/05/2010 04:06 PM, Mark Lord wrote:
> Yeah, maybe something like that.
> 
> But do note that I have verified that "hdparm -C" does work correctly,
> and is dependent upon correctly returned result_tf values.
> 
> It's not a PIO "data" command, though.  Does it get issued the same way?

PIO Setup FIS is not used for nodata commands.  IIRC, nodata uses D2H
Reg FIS for command completion.  I think the only exception is PIO
data commands.  The weird hack is to satisfy PIO data command protocol
timing requirement stemming from PATA specification.  Eh... ugly.

Thanks.

-- 
tejun

Re: [BUG] libahci returns stale result tf much of the time.

[Robert Hancock]

On Tue, Oct 5, 2010 at 10:06 AM, Tejun Heo <htejun@gmail.com> wrote:
> Hello,
>
> On 10/05/2010 04:06 PM, Mark Lord wrote:
>> Yeah, maybe something like that.
>>
>> But do note that I have verified that "hdparm -C" does work correctly,
>> and is dependent upon correctly returned result_tf values.
>>
>> It's not a PIO "data" command, though.  Does it get issued the same way?
>
> PIO Setup FIS is not used for nodata commands.  IIRC, nodata uses D2H
> Reg FIS for command completion.  I think the only exception is PIO
> data commands.  The weird hack is to satisfy PIO data command protocol
> timing requirement stemming from PATA specification.  Eh... ugly.

From my reading of the SATA spec, with the PIO data-in protocol, the
device can raise a D2H register FIS prior to transmitting data if the
command is aborted due to an error, but for normal completion, the
state machine just goes back to idle after the last data FIS is
transmitted and there's no final D2H register FIS - the final status
is set by the last PIO Setup FIS received. According to the AHCI spec,
the host adapter is supposed to copy the PIO Setup FIS to memory as
well. Presumably when retrieving the result taskfile for such PIO
data-in transfers we should be pulling it from that area, not the D2H
register FIS area, at least if the device transferred any data?

[PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Tejun Heo]

ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
command.  The host is supposed to take the E_Status of the preceding
PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
after a successful ATA PIO data-in command.

Without this patch, result_tf for such a command is filled with the
content of the previous D2H Reg FIS which belongs to a previous
command, which can make the command incorrectly seen as failed.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Mark Lord <kernel@teksavvy.com>
Cc: Robert Hancock <hancockrwd@gmail.com>
Cc: stable@kernel.org
---
 drivers/ata/ahci.h    |    1 +
 drivers/ata/libahci.c |   17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index e5fdeeb..d1a0f5b 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -72,6 +72,7 @@ enum {
 	AHCI_CMD_RESET		= (1 << 8),
 	AHCI_CMD_CLR_BUSY	= (1 << 10),

+	RX_FIS_PIO_SETUP	= 0x20,	/* offset of PIO Setup FIS data */
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 	RX_FIS_SDB		= 0x58, /* offset of SDB FIS data */
 	RX_FIS_UNK		= 0x60, /* offset of Unknown FIS data */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 8eea309..0c482a0 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1830,12 +1830,23 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;
-	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
+	u8 *rx_fis = pp->rx_fis;

 	if (pp->fbs_enabled)
-		d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
+		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
+
+	/*
+	 * After a successful execution of an ATA PIO data-in command,
+	 * the device doesn't send D2H Reg FIS to update the TF and
+	 * the host should take Status from E_Status of the preceding
+	 * PIO Setup FIS.
+	 */
+	if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
+	    !(qc->flags & ATA_QCFLAG_FAILED))
+		qc->result_tf.command = (rx_fis + RX_FIS_PIO_SETUP)[15];
+	else
+		ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);

-	ata_tf_from_fis(d2h_fis, &qc->result_tf);
 	return true;
 }

Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Mark Lord]

On 10-10-14 04:53 AM, Tejun Heo wrote:
> ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
> command.  The host is supposed to take the E_Status of the preceding
> PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
> after a successful ATA PIO data-in command.
>
> Without this patch, result_tf for such a command is filled with the
> content of the previous D2H Reg FIS which belongs to a previous
> command, which can make the command incorrectly seen as failed.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Reported-by: Mark Lord<kernel@teksavvy.com>
> Cc: Robert Hancock<hancockrwd@gmail.com>
> Cc: stable@kernel.org
> ---
>   drivers/ata/ahci.h    |    1 +
>   drivers/ata/libahci.c |   17 ++++++++++++++---
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index e5fdeeb..d1a0f5b 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -72,6 +72,7 @@ enum {
>   	AHCI_CMD_RESET		= (1<<  8),
>   	AHCI_CMD_CLR_BUSY	= (1<<  10),
>
> +	RX_FIS_PIO_SETUP	= 0x20,	/* offset of PIO Setup FIS data */
>   	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
>   	RX_FIS_SDB		= 0x58, /* offset of SDB FIS data */
>   	RX_FIS_UNK		= 0x60, /* offset of Unknown FIS data */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 8eea309..0c482a0 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1830,12 +1830,23 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
>   static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>   {
>   	struct ahci_port_priv *pp = qc->ap->private_data;
> -	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
> +	u8 *rx_fis = pp->rx_fis;
>
>   	if (pp->fbs_enabled)
> -		d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +
> +	/*
> +	 * After a successful execution of an ATA PIO data-in command,
> +	 * the device doesn't send D2H Reg FIS to update the TF and
> +	 * the host should take Status from E_Status of the preceding
> +	 * PIO Setup FIS.
> +	 */
> +	if (qc->tf.protocol == ATA_PROT_PIO&&  qc->dma_dir == DMA_FROM_DEVICE&&
> +	    !(qc->flags&  ATA_QCFLAG_FAILED))
..

I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO commands?
Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?

Thanks.

Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Robert Hancock]

On Thu, Oct 14, 2010 at 3:38 PM, Mark Lord <kernel@teksavvy.com> wrote:
> On 10-10-14 04:53 AM, Tejun Heo wrote:
>>
>> ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
>> command.  The host is supposed to take the E_Status of the preceding
>> PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
>> after a successful ATA PIO data-in command.
>>
>> Without this patch, result_tf for such a command is filled with the
>> content of the previous D2H Reg FIS which belongs to a previous
>> command, which can make the command incorrectly seen as failed.
>>
>> Signed-off-by: Tejun Heo<tj@kernel.org>
>> Reported-by: Mark Lord<kernel@teksavvy.com>
>> Cc: Robert Hancock<hancockrwd@gmail.com>
>> Cc: stable@kernel.org
>> ---
>>  drivers/ata/ahci.h    |    1 +
>>  drivers/ata/libahci.c |   17 ++++++++++++++---
>>  2 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>> index e5fdeeb..d1a0f5b 100644
>> --- a/drivers/ata/ahci.h
>> +++ b/drivers/ata/ahci.h
>> @@ -72,6 +72,7 @@ enum {
>>        AHCI_CMD_RESET          = (1<<  8),
>>        AHCI_CMD_CLR_BUSY       = (1<<  10),
>>
>> +       RX_FIS_PIO_SETUP        = 0x20, /* offset of PIO Setup FIS data */
>>        RX_FIS_D2H_REG          = 0x40, /* offset of D2H Register FIS data
>> */
>>        RX_FIS_SDB              = 0x58, /* offset of SDB FIS data */
>>        RX_FIS_UNK              = 0x60, /* offset of Unknown FIS data */
>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>> index 8eea309..0c482a0 100644
>> --- a/drivers/ata/libahci.c
>> +++ b/drivers/ata/libahci.c
>> @@ -1830,12 +1830,23 @@ static unsigned int ahci_qc_issue(struct
>> ata_queued_cmd *qc)
>>  static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>>  {
>>        struct ahci_port_priv *pp = qc->ap->private_data;
>> -       u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
>> +       u8 *rx_fis = pp->rx_fis;
>>
>>        if (pp->fbs_enabled)
>> -               d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
>> +               rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
>> +
>> +       /*
>> +        * After a successful execution of an ATA PIO data-in command,
>> +        * the device doesn't send D2H Reg FIS to update the TF and
>> +        * the host should take Status from E_Status of the preceding
>> +        * PIO Setup FIS.
>> +        */
>> +       if (qc->tf.protocol == ATA_PROT_PIO&&  qc->dma_dir ==
>> DMA_FROM_DEVICE&&
>> +           !(qc->flags&  ATA_QCFLAG_FAILED))
>
> ..
>
> I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO
> commands?
> Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?

No, PIO data-out commands send a D2H register FIS after completion. It
appears that PIO data-in is the only special case.

Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Robert Hancock]

On Thu, Oct 14, 2010 at 2:53 AM, Tejun Heo <tj@kernel.org> wrote:
> ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
> command.  The host is supposed to take the E_Status of the preceding
> PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes E_Status
> after a successful ATA PIO data-in command.
>
> Without this patch, result_tf for such a command is filled with the
> content of the previous D2H Reg FIS which belongs to a previous
> command, which can make the command incorrectly seen as failed.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Mark Lord <kernel@teksavvy.com>
> Cc: Robert Hancock <hancockrwd@gmail.com>
> Cc: stable@kernel.org
> ---
>  drivers/ata/ahci.h    |    1 +
>  drivers/ata/libahci.c |   17 ++++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index e5fdeeb..d1a0f5b 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -72,6 +72,7 @@ enum {
>        AHCI_CMD_RESET          = (1 << 8),
>        AHCI_CMD_CLR_BUSY       = (1 << 10),
>
> +       RX_FIS_PIO_SETUP        = 0x20, /* offset of PIO Setup FIS data */
>        RX_FIS_D2H_REG          = 0x40, /* offset of D2H Register FIS data */
>        RX_FIS_SDB              = 0x58, /* offset of SDB FIS data */
>        RX_FIS_UNK              = 0x60, /* offset of Unknown FIS data */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 8eea309..0c482a0 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1830,12 +1830,23 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
>  static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>  {
>        struct ahci_port_priv *pp = qc->ap->private_data;
> -       u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
> +       u8 *rx_fis = pp->rx_fis;
>
>        if (pp->fbs_enabled)
> -               d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +               rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> +
> +       /*
> +        * After a successful execution of an ATA PIO data-in command,
> +        * the device doesn't send D2H Reg FIS to update the TF and
> +        * the host should take Status from E_Status of the preceding
> +        * PIO Setup FIS.
> +        */
> +       if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
> +           !(qc->flags & ATA_QCFLAG_FAILED))
> +               qc->result_tf.command = (rx_fis + RX_FIS_PIO_SETUP)[15];

Shouldn't we be reading the entire TF from the PIO setup FIS and then
just replacing the command register value with the E_Status field? It
looks like that's the only field in the TF that's being set here.

> +       else
> +               ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
>
> -       ata_tf_from_fis(d2h_fis, &qc->result_tf);
>        return true;
>  }
>
>

Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Tejun Heo]

Hello,

On 10/15/2010 02:32 AM, Robert Hancock wrote:
>> +       if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
>> +           !(qc->flags & ATA_QCFLAG_FAILED))
>> +               qc->result_tf.command = (rx_fis + RX_FIS_PIO_SETUP)[15];
> 
> Shouldn't we be reading the entire TF from the PIO setup FIS and then
> just replacing the command register value with the E_Status field? It
> looks like that's the only field in the TF that's being set here.

Hmmm, yeah, probably that would be better.  I'll post the updated
version soon.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Tejun Heo]

On 10/15/2010 02:24 AM, Robert Hancock wrote:
>> I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO
>> commands?
>> Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?
> 
> No, PIO data-out commands send a D2H register FIS after completion. It
> appears that PIO data-in is the only special case.

Yeap, it's an exception only for PIO data-in's.

Thanks.

-- 
tejun

[PATCH UPDATED #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Tejun Heo]

ATA devices don't send D2H Reg FIS after an successful ATA PIO data-in
command.  The host is supposed to take the TF and E_Status of the
preceding PIO Setup FIS.  Update ahci_qc_fill_rtf() such that it takes
TF + E_Status from PIO Setup FIS after a successful ATA PIO data-in
command.

Without this patch, result_tf for such a command is filled with the
content of the previous D2H Reg FIS which belongs to a previous
command, which can make the command incorrectly seen as failed.

* Patch updated to grab the whole TF + E_Status from PIO Setup FIS
  instead of just E_Status as suggested by Robert Hancock.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Mark Lord <kernel@teksavvy.com>
Cc: Robert Hancock <hancockrwd@gmail.com>
Cc: stable@kernel.org
---
 drivers/ata/ahci.h    |    1 +
 drivers/ata/libahci.c |   18 +++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index e5fdeeb..d1a0f5b 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -72,6 +72,7 @@ enum {
 	AHCI_CMD_RESET		= (1 << 8),
 	AHCI_CMD_CLR_BUSY	= (1 << 10),

+	RX_FIS_PIO_SETUP	= 0x20,	/* offset of PIO Setup FIS data */
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 	RX_FIS_SDB		= 0x58, /* offset of SDB FIS data */
 	RX_FIS_UNK		= 0x60, /* offset of Unknown FIS data */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 8eea309..137514d 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1830,12 +1830,24 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;
-	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
+	u8 *rx_fis = pp->rx_fis;

 	if (pp->fbs_enabled)
-		d2h_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
+		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
+
+	/*
+	 * After a successful execution of an ATA PIO data-in command,
+	 * the device doesn't send D2H Reg FIS to update the TF and
+	 * the host should take TF and E_Status from the preceding PIO
+	 * Setup FIS.
+	 */
+	if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
+	    !(qc->flags & ATA_QCFLAG_FAILED)) {
+		ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
+		qc->result_tf.command = (rx_fis + RX_FIS_PIO_SETUP)[15];
+	} else
+		ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);

-	ata_tf_from_fis(d2h_fis, &qc->result_tf);
 	return true;
 }

Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command

[Mark Lord]

On 10-10-15 04:45 AM, Tejun Heo wrote:
> On 10/15/2010 02:24 AM, Robert Hancock wrote:
>>> I'm not certain, but doesn't this problem also affect DMA_TO_DEVICE PIO
>>> commands?
>>> Eg. SECURITY_ERASE_PREPARE and PIO_WRITE ?
>>
>> No, PIO data-out commands send a D2H register FIS after completion. It
>> appears that PIO data-in is the only special case.
>
> Yeap, it's an exception only for PIO data-in's.


Peachy.  Thanks for working on this.
I'm away from my setup right now, but will test as soon as I get back.

In the meanwhile, feel free to nag if you don't hear back by Mon/Tues.

Cheers