ahci, SActive flag, and the HD activity LED

ahci, SActive flag, and the HD activity LED

[Martin Wilck]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

Hello Jeff, hello Jens, hello everybody,

I am referring to the debate about whether or not setting the SActive 
bit for non-NCQ ATA commands (e.g. http://lkml.org/lkml/2005/5/26/142).

In our machines, this behavior of the Linux AHCI driver causes the HD 
activity LED to stay on all the time. If I apply the attached trivial 
patch (this is for the RedHat EL4.0-U1 kernel), the LED behaves nicely.

Jeff has stated in the above thread that "SActive is intentionally used 
for non-NCQ devices". However I find clear indication in the specs that 
the SActive flag should be set if and only if tagged queuing is being 
used, and only for a specified subset of commands that support queuing 
(http://www.t13.org/docs2005/D1699r1e-ATA8-ACS.pdf, secs. 4.19 and 
4.20). The current mainline driver doesn't use queuing.

If I am reading the specs correctly, that'd mean the ahci driver is 
wrong in setting the SActive bit. Could you please comment? Jeff, in 
particular, could you please give more detail why you say this flag is 
"intentionally used"?

Regards
Martin

-- 
Martin Wilck                Phone: +49 5251 8 15113
Fujitsu Siemens Computers   Fax:   +49 5251 8 20409
Heinz-Nixdorf-Ring 1        mailto:Martin.Wilck@Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy


[-- Attachment #2: ahci_sactive.diff --]
[-- Type: text/x-patch, Size: 383 bytes --]

--- ahci.c.orig	2005-06-13 11:39:26.000000000 +0200
+++ ahci.c	2005-08-02 10:48:47.000000000 +0200
@@ -691,9 +703,6 @@
 	struct ata_port *ap = qc->ap;
 	void *port_mmio = (void *) ap->ioaddr.cmd_addr;
 
-	writel(1, port_mmio + PORT_SCR_ACT);
-	readl(port_mmio + PORT_SCR_ACT);	/* flush */
-
 	writel(1, port_mmio + PORT_CMD_ISSUE);
 	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
 

Re: ahci, SActive flag, and the HD activity LED

[Jens Axboe]

On Tue, Aug 02 2005, Martin Wilck wrote:
> Hello Jeff, hello Jens, hello everybody,
> 
> I am referring to the debate about whether or not setting the SActive 
> bit for non-NCQ ATA commands (e.g. http://lkml.org/lkml/2005/5/26/142).
> 
> In our machines, this behavior of the Linux AHCI driver causes the HD 
> activity LED to stay on all the time. If I apply the attached trivial 
> patch (this is for the RedHat EL4.0-U1 kernel), the LED behaves nicely.
> 
> Jeff has stated in the above thread that "SActive is intentionally used 
> for non-NCQ devices". However I find clear indication in the specs that 
> the SActive flag should be set if and only if tagged queuing is being 
> used, and only for a specified subset of commands that support queuing 
> (http://www.t13.org/docs2005/D1699r1e-ATA8-ACS.pdf, secs. 4.19 and 
> 4.20). The current mainline driver doesn't use queuing.
> 
> If I am reading the specs correctly, that'd mean the ahci driver is 
> wrong in setting the SActive bit. Could you please comment? Jeff, in 
> particular, could you please give more detail why you say this flag is 
> "intentionally used"?

I completely agree, that was my reading of the spec as well and hence my
original posts about this in the NCQ thread.

-- 
Jens Axboe

Re: ahci, SActive flag, and the HD activity LED

[Martin Wilck]

Jens Axboe wrote:

>>If I am reading the specs correctly, that'd mean the ahci driver is 
>>wrong in setting the SActive bit.
> 
> I completely agree, that was my reading of the spec as well and hence my
> original posts about this in the NCQ thread.

Have you (or has anybody else) also seen the wrong behavior of the 
activity LED?

Regards
Martin

-- 
Martin Wilck                Phone: +49 5251 8 15113
Fujitsu Siemens Computers   Fax:   +49 5251 8 20409
Heinz-Nixdorf-Ring 1        mailto:Martin.Wilck@Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy

Re: ahci, SActive flag, and the HD activity LED

[Jens Axboe]

On Wed, Aug 03 2005, Martin Wilck wrote:
> Jens Axboe wrote:
> 
> >>If I am reading the specs correctly, that'd mean the ahci driver is 
> >>wrong in setting the SActive bit.
> >
> >I completely agree, that was my reading of the spec as well and hence my
> >original posts about this in the NCQ thread.
> 
> Have you (or has anybody else) also seen the wrong behavior of the 
> activity LED?

No, but I have observed that SActive never gets cleared by the device
for non-NCQ commands (which is probably which gets you the stuck LED on
some systems?), which to me is another indication that we should not be
setting the tag bits for those commands.

-- 
Jens Axboe

Re: ahci, SActive flag, and the HD activity LED

[Pasi Kärkkäinen]

On Wed, Aug 03, 2005 at 07:17:13AM +0200, Martin Wilck wrote:
> Jens Axboe wrote:
> 
> >>If I am reading the specs correctly, that'd mean the ahci driver is 
> >>wrong in setting the SActive bit.
> >
> >I completely agree, that was my reading of the spec as well and hence my
> >original posts about this in the NCQ thread.
> 
> Have you (or has anybody else) also seen the wrong behavior of the 
> activity LED?
>

I have a box with ICH6R and AHCI in use with Linux 2.6.11, using seagate NCQ
sata drives. the HD activity LED is on all the time..

- Pasi Kärkkäinen
       
                                   ^
                                .     .
                                 Linux
                              /    -    \
                             Choice.of.the
                           .Next.Generation.

Re: ahci, SActive flag, and the HD activity LED

[André Tomt]

Martin Wilck wrote:
> Jens Axboe wrote:
> 
>>> If I am reading the specs correctly, that'd mean the ahci driver is 
>>> wrong in setting the SActive bit.
>>
>>
>> I completely agree, that was my reading of the spec as well and hence my
>> original posts about this in the NCQ thread.
> 
> 
> Have you (or has anybody else) also seen the wrong behavior of the 
> activity LED?

I have, Asus P5LD2 i945P, ICH7R AHCI controller, Hitachi Deskstar T7K250 
250GB "SATA2" drives. Kernel 2.6.12.x

Re: ahci, SActive flag, and the HD activity LED

[Adam Goode]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Wed, 2005-08-03 at 07:17 +0200, Martin Wilck wrote:
> Jens Axboe wrote:
> 
> >>If I am reading the specs correctly, that'd mean the ahci driver is 
> >>wrong in setting the SActive bit.
> > 
> > I completely agree, that was my reading of the spec as well and hence my
> > original posts about this in the NCQ thread.
> 
> Have you (or has anybody else) also seen the wrong behavior of the 
> activity LED?

Yes, Dell Precision 380, ICH7R AHCI controller, SATA non-NCQ Western
Digital drive.


Adam


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: ahci, SActive flag, and the HD activity LED

[Matthias Schniedermeyer]

Martin Wilck wrote:
> Jens Axboe wrote:
> 
>>> If I am reading the specs correctly, that'd mean the ahci driver is 
>>> wrong in setting the SActive bit.
>>
>>
>> I completely agree, that was my reading of the spec as well and hence my
>> original posts about this in the NCQ thread.
> 
> Have you (or has anybody else) also seen the wrong behavior of the 
> activity LED?

925X-Chipset
Lspci says: 8086:2652
Intel Corp. 82801FR/FRW (ICH6R/ICH6RW) SATA Controller (rev 03)
HDD:
Western Digital WD2000JD-00H, i believe this HDD is non-NCQ.

Kernels: 2.6.10 - 2.6.12
The Activity-LED has burned like a light-bulb every since i have that 
computer. (Excluding the few seconds before booting Linux. :-) )



Bis denn

-- 
Real Programmers consider "what you see is what you get" to be just as
bad a concept in Text Editors as it is in women. No, the Real Programmer
wants a "you asked for it, you got it" text editor -- complicated,
cryptic, powerful, unforgiving, dangerous.

[PATCH] Fix HD activity LED with ahci

[Martin Wilck]

[-- Attachment #1: Type: text/plain, Size: 128 bytes --]

All right,

this looks like a pretty broad agreement on this issue.
Jeff, would you please apply this patch?

Regards,
Martin



[-- Attachment #2: ahci_sactive.diff --]
[-- Type: text/x-patch, Size: 1257 bytes --]

Patch: fix wrong HD activity control by ahci driver
Signed-off-by: Martin.Wilck@fujitsu-siemens.com

The ahci driver 1.0 sets the SActive bit on every transaction,
causing the LED to light up. The SActive bit is used only for
native command queuing (NCQ) which the current driver version 
doesn't implement. Resetting the SActive bit is the device's 
responsibility (by sending a "Set Device Bits FIS" to the
host adapter) but this is not required in response to 
non-NCQ commands, and (most) devices don't. Thus the LED 
stays always on. This patch fixes the LED behavior.

Spec references:
http://www.intel.com/technology/serialata/pdf/rev1_1.pdf, sec. 3.3.13, 5.5.1
http://www.serialata.org/docs/serialata10a.pdf
http://www.intel.com/design/storage/papers/25266401.pdf

--- linux-2.6.13-rc5/drivers/scsi/ahci.c.orig	2005-08-04 08:14:44.000000000 +0200
+++ linux-2.6.13-rc5/drivers/scsi/ahci.c	2005-08-04 08:19:06.000000000 +0200
@@ -696,9 +696,6 @@ static int ahci_qc_issue(struct ata_queu
 	struct ata_port *ap = qc->ap;
 	void *port_mmio = (void *) ap->ioaddr.cmd_addr;
 
-	writel(1, port_mmio + PORT_SCR_ACT);
-	readl(port_mmio + PORT_SCR_ACT);	/* flush */
-
 	writel(1, port_mmio + PORT_CMD_ISSUE);
 	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
 

Re: ahci, SActive flag, and the HD activity LED

[Eric D. Mudama]

On 8/3/05, Jens Axboe <axboe@suse.de> wrote:
> On Wed, Aug 03 2005, Martin Wilck wrote:
> > Have you (or has anybody else) also seen the wrong behavior of the
> > activity LED?
> 
> No, but I have observed that SActive never gets cleared by the device
> for non-NCQ commands (which is probably which gets you the stuck LED on
> some systems?), which to me is another indication that we should not be
> setting the tag bits for those commands.

The drives won't send a SetBits FIS when not using NCQ, as it can
really confuse some host adapters that don't understand NCQ.  I'd
imagine you're correct that the driver shouldn't be setting the bit in
the first place.

--eric

Re: [PATCH] Fix HD activity LED with ahci

[Jeff Garzik]

Martin Wilck wrote:
> All right,
> 
> this looks like a pretty broad agreement on this issue.
> Jeff, would you please apply this patch?
> 
> Regards,
> Martin
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Patch: fix wrong HD activity control by ahci driver
> Signed-off-by: Martin.Wilck@fujitsu-siemens.com
> 
> The ahci driver 1.0 sets the SActive bit on every transaction,
> causing the LED to light up. The SActive bit is used only for
> native command queuing (NCQ) which the current driver version 
> doesn't implement. Resetting the SActive bit is the device's 
> responsibility (by sending a "Set Device Bits FIS" to the
> host adapter) but this is not required in response to 
> non-NCQ commands, and (most) devices don't. Thus the LED 
> stays always on. This patch fixes the LED behavior.
> 
> Spec references:
> http://www.intel.com/technology/serialata/pdf/rev1_1.pdf, sec. 3.3.13, 5.5.1
> http://www.serialata.org/docs/serialata10a.pdf
> http://www.intel.com/design/storage/papers/25266401.pdf
> 
> --- linux-2.6.13-rc5/drivers/scsi/ahci.c.orig	2005-08-04 08:14:44.000000000 +0200
> +++ linux-2.6.13-rc5/drivers/scsi/ahci.c	2005-08-04 08:19:06.000000000 +0200
> @@ -696,9 +696,6 @@ static int ahci_qc_issue(struct ata_queu
>  	struct ata_port *ap = qc->ap;
>  	void *port_mmio = (void *) ap->ioaddr.cmd_addr;
>  
> -	writel(1, port_mmio + PORT_SCR_ACT);
> -	readl(port_mmio + PORT_SCR_ACT);	/* flush */
> -
>  	writel(1, port_mmio + PORT_CMD_ISSUE);
>  	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */

To answer the question everybody was asking, this line was in the code 
because it was in the patch that got AHCI working.

I'm inclined to apply the above patch, but I'll wait until 2.6.13, so 
that we can get some decent testing.

	Jeff

Re: [PATCH] Fix HD activity LED with ahci

[Jeff Garzik]

applied

Re: [PATCH] Fix HD activity LED with ahci

[Martin Wilck]

Jeff Garzik wrote:

> To answer the question everybody was asking, this line was in the code 
> because it was in the patch that got AHCI working.
> 
> I'm inclined to apply the above patch, but I'll wait until 2.6.13, so 
> that we can get some decent testing.

Great. As far as the testing is concerned, we have had no problems with 
the patch here.

Thanks, Martin

-- 
Martin Wilck                Phone: +49 5251 8 15113
Fujitsu Siemens Computers   Fax:   +49 5251 8 20409
Heinz-Nixdorf-Ring 1        mailto:Martin.Wilck@Fujitsu-Siemens.com
D-33106 Paderborn           http://www.fujitsu-siemens.com/primergy