* [BUG] libahci returns stale result tf much of the time. @ 2010-09-24 5:34 Mark Lord 2010-09-24 5:49 ` Seed 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-09-24 5:34 UTC (permalink / raw) To: Tejun Heo, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-09-24 5:34 [BUG] libahci returns stale result tf much of the time Mark Lord @ 2010-09-24 5:49 ` Seed 2010-09-24 6:27 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Seed @ 2010-09-24 5:49 UTC (permalink / raw) To: Mark Lord; +Cc: Tejun Heo, Jeff Garzik, IDE/ATA development list 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? -- ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-09-24 5:49 ` Seed @ 2010-09-24 6:27 ` Mark Lord 2010-09-24 7:01 ` Seed 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-09-24 6:27 UTC (permalink / raw) To: Seed; +Cc: Tejun Heo, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-09-24 6:27 ` Mark Lord @ 2010-09-24 7:01 ` Seed 2010-09-24 13:11 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Seed @ 2010-09-24 7:01 UTC (permalink / raw) To: Mark Lord; +Cc: Tejun Heo, Jeff Garzik, IDE/ATA development list 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. -- ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-09-24 7:01 ` Seed @ 2010-09-24 13:11 ` Mark Lord 2010-09-24 13:24 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-09-24 13:11 UTC (permalink / raw) To: Seed; +Cc: Tejun Heo, Jeff Garzik, IDE/ATA development list 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; } ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-09-24 13:11 ` Mark Lord @ 2010-09-24 13:24 ` Mark Lord 2010-09-24 23:26 ` Robert Hancock 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-09-24 13:24 UTC (permalink / raw) To: Seed; +Cc: Tejun Heo, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-09-24 13:24 ` Mark Lord @ 2010-09-24 23:26 ` Robert Hancock 2010-10-04 9:18 ` Tejun Heo 0 siblings, 1 reply; 28+ messages in thread From: Robert Hancock @ 2010-09-24 23:26 UTC (permalink / raw) To: Mark Lord; +Cc: Seed, Tejun Heo, Jeff Garzik, IDE/ATA development list 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? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-09-24 23:26 ` Robert Hancock @ 2010-10-04 9:18 ` Tejun Heo 2010-10-04 17:01 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Tejun Heo @ 2010-10-04 9:18 UTC (permalink / raw) To: Robert Hancock; +Cc: Mark Lord, Seed, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 9:18 ` Tejun Heo @ 2010-10-04 17:01 ` Mark Lord 2010-10-04 17:06 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-04 17:01 UTC (permalink / raw) To: Tejun Heo; +Cc: Robert Hancock, Seed, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 17:01 ` Mark Lord @ 2010-10-04 17:06 ` Mark Lord 2010-10-04 17:31 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-04 17:06 UTC (permalink / raw) To: Tejun Heo; +Cc: Robert Hancock, Seed, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 17:06 ` Mark Lord @ 2010-10-04 17:31 ` Mark Lord 2010-10-04 18:50 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-04 17:31 UTC (permalink / raw) To: Tejun Heo; +Cc: Robert Hancock, Seed, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 17:31 ` Mark Lord @ 2010-10-04 18:50 ` Mark Lord 2010-10-04 19:27 ` Jeff Garzik 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-04 18:50 UTC (permalink / raw) To: Tejun Heo; +Cc: Robert Hancock, Seed, Jeff Garzik, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 18:50 ` Mark Lord @ 2010-10-04 19:27 ` Jeff Garzik 2010-10-04 19:35 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Jeff Garzik @ 2010-10-04 19:27 UTC (permalink / raw) To: Mark Lord; +Cc: Tejun Heo, Robert Hancock, Seed, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 19:27 ` Jeff Garzik @ 2010-10-04 19:35 ` Mark Lord 2010-10-04 19:42 ` Jeff Garzik 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-04 19:35 UTC (permalink / raw) To: Jeff Garzik; +Cc: Tejun Heo, Robert Hancock, Seed, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 19:35 ` Mark Lord @ 2010-10-04 19:42 ` Jeff Garzik 2010-10-04 19:51 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Jeff Garzik @ 2010-10-04 19:42 UTC (permalink / raw) To: Mark Lord; +Cc: Tejun Heo, Robert Hancock, Seed, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 19:42 ` Jeff Garzik @ 2010-10-04 19:51 ` Mark Lord 2010-10-05 7:07 ` Tejun Heo 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-04 19:51 UTC (permalink / raw) To: Jeff Garzik; +Cc: Tejun Heo, Robert Hancock, Seed, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-04 19:51 ` Mark Lord @ 2010-10-05 7:07 ` Tejun Heo 2010-10-05 14:06 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Tejun Heo @ 2010-10-05 7:07 UTC (permalink / raw) To: Mark Lord; +Cc: Jeff Garzik, Robert Hancock, Seed, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-05 7:07 ` Tejun Heo @ 2010-10-05 14:06 ` Mark Lord 2010-10-05 16:06 ` Tejun Heo 0 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-05 14:06 UTC (permalink / raw) To: Tejun Heo; +Cc: Jeff Garzik, Robert Hancock, Seed, IDE/ATA development list 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? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-05 14:06 ` Mark Lord @ 2010-10-05 16:06 ` Tejun Heo 2010-10-06 1:00 ` Robert Hancock 0 siblings, 1 reply; 28+ messages in thread From: Tejun Heo @ 2010-10-05 16:06 UTC (permalink / raw) To: Mark Lord; +Cc: Jeff Garzik, Robert Hancock, Seed, IDE/ATA development list 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [BUG] libahci returns stale result tf much of the time. 2010-10-05 16:06 ` Tejun Heo @ 2010-10-06 1:00 ` Robert Hancock 2010-10-14 8:53 ` [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command Tejun Heo 0 siblings, 1 reply; 28+ messages in thread From: Robert Hancock @ 2010-10-06 1:00 UTC (permalink / raw) To: Tejun Heo; +Cc: Mark Lord, Jeff Garzik, Seed, IDE/ATA development list 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? ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-06 1:00 ` Robert Hancock @ 2010-10-14 8:53 ` Tejun Heo 2010-10-14 21:38 ` Mark Lord ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Tejun Heo @ 2010-10-14 8:53 UTC (permalink / raw) To: Jeff Garzik Cc: Robert Hancock, Mark Lord, Seed, IDE/ATA development list, stable 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; } ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-14 8:53 ` [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command Tejun Heo @ 2010-10-14 21:38 ` Mark Lord 2010-10-15 0:24 ` Robert Hancock 2010-10-15 0:32 ` Robert Hancock 2010-10-15 9:00 ` [PATCH UPDATED " Tejun Heo 2 siblings, 1 reply; 28+ messages in thread From: Mark Lord @ 2010-10-14 21:38 UTC (permalink / raw) To: Tejun Heo Cc: Jeff Garzik, Robert Hancock, Seed, IDE/ATA development list, stable 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-14 21:38 ` Mark Lord @ 2010-10-15 0:24 ` Robert Hancock 2010-10-15 8:45 ` Tejun Heo 0 siblings, 1 reply; 28+ messages in thread From: Robert Hancock @ 2010-10-15 0:24 UTC (permalink / raw) To: Mark Lord; +Cc: Tejun Heo, Jeff Garzik, Seed, IDE/ATA development list, stable 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-15 0:24 ` Robert Hancock @ 2010-10-15 8:45 ` Tejun Heo 2010-10-15 22:14 ` Mark Lord 0 siblings, 1 reply; 28+ messages in thread From: Tejun Heo @ 2010-10-15 8:45 UTC (permalink / raw) To: Robert Hancock Cc: Mark Lord, Jeff Garzik, Seed, IDE/ATA development list, stable 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-15 8:45 ` Tejun Heo @ 2010-10-15 22:14 ` Mark Lord 0 siblings, 0 replies; 28+ messages in thread From: Mark Lord @ 2010-10-15 22:14 UTC (permalink / raw) To: Tejun Heo Cc: Robert Hancock, Jeff Garzik, Seed, IDE/ATA development list, stable 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-14 8:53 ` [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command Tejun Heo 2010-10-14 21:38 ` Mark Lord @ 2010-10-15 0:32 ` Robert Hancock 2010-10-15 8:45 ` Tejun Heo 2010-10-15 9:00 ` [PATCH UPDATED " Tejun Heo 2 siblings, 1 reply; 28+ messages in thread From: Robert Hancock @ 2010-10-15 0:32 UTC (permalink / raw) To: Tejun Heo; +Cc: Jeff Garzik, Mark Lord, Seed, IDE/ATA development list, stable 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; > } > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-15 0:32 ` Robert Hancock @ 2010-10-15 8:45 ` Tejun Heo 0 siblings, 0 replies; 28+ messages in thread From: Tejun Heo @ 2010-10-15 8:45 UTC (permalink / raw) To: Robert Hancock Cc: Jeff Garzik, Mark Lord, Seed, IDE/ATA development list, stable 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH UPDATED #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command 2010-10-14 8:53 ` [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command Tejun Heo 2010-10-14 21:38 ` Mark Lord 2010-10-15 0:32 ` Robert Hancock @ 2010-10-15 9:00 ` Tejun Heo 2 siblings, 0 replies; 28+ messages in thread From: Tejun Heo @ 2010-10-15 9:00 UTC (permalink / raw) To: Jeff Garzik Cc: Robert Hancock, Mark Lord, Seed, IDE/ATA development list, stable 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; } ^ permalink raw reply related [flat|nested] 28+ messages in thread
end of thread, other threads:[~2010-10-15 22:14 UTC | newest] Thread overview: 28+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-24 5:34 [BUG] libahci returns stale result tf much of the time Mark Lord 2010-09-24 5:49 ` Seed 2010-09-24 6:27 ` Mark Lord 2010-09-24 7:01 ` Seed 2010-09-24 13:11 ` Mark Lord 2010-09-24 13:24 ` Mark Lord 2010-09-24 23:26 ` Robert Hancock 2010-10-04 9:18 ` Tejun Heo 2010-10-04 17:01 ` Mark Lord 2010-10-04 17:06 ` Mark Lord 2010-10-04 17:31 ` Mark Lord 2010-10-04 18:50 ` Mark Lord 2010-10-04 19:27 ` Jeff Garzik 2010-10-04 19:35 ` Mark Lord 2010-10-04 19:42 ` Jeff Garzik 2010-10-04 19:51 ` Mark Lord 2010-10-05 7:07 ` Tejun Heo 2010-10-05 14:06 ` Mark Lord 2010-10-05 16:06 ` Tejun Heo 2010-10-06 1:00 ` Robert Hancock 2010-10-14 8:53 ` [PATCH #upstream-fixes] libahci: fix result_tf handling after an ATA PIO data-in command Tejun Heo 2010-10-14 21:38 ` Mark Lord 2010-10-15 0:24 ` Robert Hancock 2010-10-15 8:45 ` Tejun Heo 2010-10-15 22:14 ` Mark Lord 2010-10-15 0:32 ` Robert Hancock 2010-10-15 8:45 ` Tejun Heo 2010-10-15 9:00 ` [PATCH UPDATED " Tejun Heo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).