[RFC] add DMA setup FIS auto-activate feature

[RFC] add DMA setup FIS auto-activate feature

[Shaohua Li]

hi,
The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
transfers. I had an attempt to add it, though my test doesn't show obvious
performance improvement (not worse too), I wonder why not add this feature?

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2c6aeda..53cc355 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2303,6 +2303,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
 {
 	struct ata_port *ap = dev->link->ap;
 	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
+	const u16 *id = dev->id;
 
 	if (!ata_id_has_ncq(dev->id)) {
 		desc[0] = '\0';
@@ -2317,6 +2318,9 @@ static void ata_dev_config_ncq(struct ata_device *dev,
 		dev->flags |= ATA_DFLAG_NCQ;
 	}
 
+	if (ata_id_has_daa(id))
+		ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE, SATA_DAA);
+
 	if (hdepth >= ddepth)
 		snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
 	else
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 9c75921..bb5b6ba 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -306,6 +306,7 @@ enum {
 	/* SETFEATURE Sector counts for SATA features */
 	SATA_AN			= 0x05,  /* Asynchronous Notification */
 	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
+	SATA_DAA		= 0x02,  /* DMA Setup FIS Auto-Activate */
 
 	/* feature values for SET_MAX */
 	ATA_SET_MAX_ADDR	= 0x00,
@@ -525,6 +526,9 @@ static inline int ata_is_data(u8 prot)
 #define ata_id_has_atapi_AN(id)	\
 	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
 	  ((id)[78] & (1 << 5)) )
+#define ata_id_has_daa(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 2)) )
 #define ata_id_iordy_disable(id) ((id)[ATA_ID_CAPABILITY] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[ATA_ID_CAPABILITY] & (1 << 11))
 #define ata_id_u32(id,n)	\

Re: [RFC] add DMA setup FIS auto-activate feature

[Tejun Heo]

Shaohua Li wrote:
> hi,
> The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> transfers. I had an attempt to add it, though my test doesn't show obvious
> performance improvement (not worse too), I wonder why not add this feature?
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Eh... It's not clear whether all controllers support AA and it's also
not clear whether devices which claim to support AA actually work
properly when AA is enabled.  For optional features in ATA(PI), the
best we can do is probably to allow enabling it from userland or bios.
It used to be better for ATA compared to ATAPI but with the
introduction of SSDs, the situation has gotten worse rapidly.  :-(

Thanks.

-- 
tejun

Re: [RFC] add DMA setup FIS auto-activate feature

[Jeff Garzik]

Shaohua Li wrote:
> hi,
> The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> transfers. I had an attempt to add it, though my test doesn't show obvious
> performance improvement (not worse too), I wonder why not add this feature?
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

I agree, we should turn it on -- but doesn't this also have a controller 
dependency?

IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0 
hardware may not behave properly in response, no?

	Jeff

Re: [RFC] add DMA setup FIS auto-activate feature

[Shaohua Li]

On Thu, Jul 23, 2009 at 11:59:11AM +0800, Jeff Garzik wrote:
> Shaohua Li wrote:
> > hi,
> > The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> > transfers. I had an attempt to add it, though my test doesn't show obvious
> > performance improvement (not worse too), I wonder why not add this feature?
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> I agree, we should turn it on -- but doesn't this also have a controller 
> dependency?
> 
> IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0 
> hardware may not behave properly in response, no?
It appears the AHCI spec doesn't define a HBA capability about this
feature, but I'm likely wrong as I'm not quite familar with SATA.

Thanks,
Shaohua

Re: [RFC] add DMA setup FIS auto-activate feature

[Tejun Heo]

Shaohua Li wrote:
>> IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0 
>> hardware may not behave properly in response, no?
> It appears the AHCI spec doesn't define a HBA capability about this
> feature, but I'm likely wrong as I'm not quite familar with SATA.

IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement
the spec correctly, it should work.  But even for ahcis, if we enable
it by default, I'm fairly sure we'll be met by a number of unpleasant
surprises.  Not sure whether doing that would be worth the trouble or
not.

Thanks.

-- 
tejun

Re: [RFC] add DMA setup FIS auto-activate feature

[Jeff Garzik]

Tejun Heo wrote:
> IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement

AHCI?  Perhaps.   But AA is not in the _Serial ATA_ 1.0 spec...

It was added in SATA II, and the SATA II specs specifically mention that 
AA was not present in SATA 1.0.

	Jeff

Re: [RFC] add DMA setup FIS auto-activate feature

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement
> 
> AHCI?  Perhaps.   But AA is not in the _Serial ATA_ 1.0 spec...
> 
> It was added in SATA II, and the SATA II specs specifically mention that
> AA was not present in SATA 1.0.

Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
and let the drivers set it.  It would probably be better to set it
during -rc's and then disable it for release for a few devel cycles.

Thanks.

-- 
tejun

Re: [RFC] add DMA setup FIS auto-activate feature

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> AA was added in SATA II, and the SATA II specs specifically mention that
>> AA was not present in SATA 1.0.
> 
> Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
> all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
> and let the drivers set it.


Yep -- that was the logical conclusion of my rhetorical question at the 
beginning of this thread ;-)

	Jeff

Re: [RFC] add DMA setup FIS auto-activate feature

[Shaohua Li]

On Thu, Jul 23, 2009 at 02:48:08PM +0800, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Jeff Garzik wrote:
> >> AA was added in SATA II, and the SATA II specs specifically mention that
> >> AA was not present in SATA 1.0.
> > 
> > Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
> > all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
> > and let the drivers set it.
> 
> 
> Yep -- that was the logical conclusion of my rhetorical question at the 
> beginning of this thread ;-)
Something like below?

Add SATA DMA setup FIS auto-activate feature in AHCI.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 336eb1e..d5d67b8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2802,7 +2802,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* prepare host */
 	if (hpriv->cap & HOST_CAP_NCQ)
-		pi.flags |= ATA_FLAG_NCQ;
+		pi.flags |= ATA_FLAG_NCQ | ATA_FLAG_FPDMA_AA;
 
 	if (hpriv->cap & HOST_CAP_PMP)
 		pi.flags |= ATA_FLAG_PMP;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2c6aeda..62355cf 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2303,6 +2303,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
 {
 	struct ata_port *ap = dev->link->ap;
 	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
+	const u16 *id = dev->id;
 
 	if (!ata_id_has_ncq(dev->id)) {
 		desc[0] = '\0';
@@ -2317,6 +2318,10 @@ static void ata_dev_config_ncq(struct ata_device *dev,
 		dev->flags |= ATA_DFLAG_NCQ;
 	}
 
+	if ((ap->flags & ATA_FLAG_FPDMA_AA) && ata_id_has_fpdma_aa(id))
+		ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
+			SATA_FPDMA_AA);
+
 	if (hdepth >= ddepth)
 		snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
 	else
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 9c75921..f549405 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -306,6 +306,7 @@ enum {
 	/* SETFEATURE Sector counts for SATA features */
 	SATA_AN			= 0x05,  /* Asynchronous Notification */
 	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
+	SATA_FPDMA_AA		= 0x02,  /* DMA Setup FIS Auto-Activate */
 
 	/* feature values for SET_MAX */
 	ATA_SET_MAX_ADDR	= 0x00,
@@ -525,6 +526,9 @@ static inline int ata_is_data(u8 prot)
 #define ata_id_has_atapi_AN(id)	\
 	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
 	  ((id)[78] & (1 << 5)) )
+#define ata_id_has_fpdma_aa(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 2)) )
 #define ata_id_iordy_disable(id) ((id)[ATA_ID_CAPABILITY] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[ATA_ID_CAPABILITY] & (1 << 11))
 #define ata_id_u32(id,n)	\
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 79b6d7f..ca210d8 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -190,6 +190,7 @@ enum {
 	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1 << 11), /* don't spindown before poweroff */
 	ATA_FLAG_NO_HIBERNATE_SPINDOWN = (1 << 12), /* don't spindown before hibernation */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
+	ATA_FLAG_FPDMA_AA		= (1 << 14), /* driver supports Auto-Activate */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
 	ATA_FLAG_ACPI_SATA	= (1 << 17), /* need native SATA ACPI layout */

Re: [RFC] add DMA setup FIS auto-activate feature

[Robert Hancock]

On 07/23/2009 01:25 AM, Shaohua Li wrote:
> On Thu, Jul 23, 2009 at 02:48:08PM +0800, Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Jeff Garzik wrote:
>>>> AA was added in SATA II, and the SATA II specs specifically mention that
>>>> AA was not present in SATA 1.0.
>>> Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
>>> all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
>>> and let the drivers set it.
>>
>> Yep -- that was the logical conclusion of my rhetorical question at the
>> beginning of this thread ;-)
> Something like below?
>
> Add SATA DMA setup FIS auto-activate feature in AHCI.
>
> Signed-off-by: Shaohua Li<shaohua.li@intel.com>
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 336eb1e..d5d67b8 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -2802,7 +2802,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>   	/* prepare host */
>   	if (hpriv->cap&  HOST_CAP_NCQ)
> -		pi.flags |= ATA_FLAG_NCQ;
> +		pi.flags |= ATA_FLAG_NCQ | ATA_FLAG_FPDMA_AA;
>
>   	if (hpriv->cap&  HOST_CAP_PMP)
>   		pi.flags |= ATA_FLAG_PMP;
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 2c6aeda..62355cf 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2303,6 +2303,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>   {
>   	struct ata_port *ap = dev->link->ap;
>   	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
> +	const u16 *id = dev->id;
>
>   	if (!ata_id_has_ncq(dev->id)) {
>   		desc[0] = '\0';
> @@ -2317,6 +2318,10 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>   		dev->flags |= ATA_DFLAG_NCQ;
>   	}
>
> +	if ((ap->flags&  ATA_FLAG_FPDMA_AA)&&  ata_id_has_fpdma_aa(id))
> +		ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
> +			SATA_FPDMA_AA);
> +
>   	if (hdepth>= ddepth)
>   		snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
>   	else
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 9c75921..f549405 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -306,6 +306,7 @@ enum {
>   	/* SETFEATURE Sector counts for SATA features */
>   	SATA_AN			= 0x05,  /* Asynchronous Notification */
>   	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
> +	SATA_FPDMA_AA		= 0x02,  /* DMA Setup FIS Auto-Activate */
>
>   	/* feature values for SET_MAX */
>   	ATA_SET_MAX_ADDR	= 0x00,
> @@ -525,6 +526,9 @@ static inline int ata_is_data(u8 prot)
>   #define ata_id_has_atapi_AN(id)	\
>   	( (((id)[76] != 0x0000)&&  ((id)[76] != 0xffff))&&  \
>   	  ((id)[78]&  (1<<  5)) )
> +#define ata_id_has_fpdma_aa(id)	\
> +	( (((id)[76] != 0x0000)&&  ((id)[76] != 0xffff))&&  \
> +	  ((id)[78]&  (1<<  2)) )
>   #define ata_id_iordy_disable(id) ((id)[ATA_ID_CAPABILITY]&  (1<<  10))
>   #define ata_id_has_iordy(id) ((id)[ATA_ID_CAPABILITY]&  (1<<  11))
>   #define ata_id_u32(id,n)	\
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 79b6d7f..ca210d8 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -190,6 +190,7 @@ enum {
>   	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1<<  11), /* don't spindown before poweroff */
>   	ATA_FLAG_NO_HIBERNATE_SPINDOWN = (1<<  12), /* don't spindown before hibernation */
>   	ATA_FLAG_DEBUGMSG	= (1<<  13),
> +	ATA_FLAG_FPDMA_AA		= (1<<  14), /* driver supports Auto-Activate */
>   	ATA_FLAG_IGN_SIMPLEX	= (1<<  15), /* ignore SIMPLEX */
>   	ATA_FLAG_NO_IORDY	= (1<<  16), /* controller lacks iordy */
>   	ATA_FLAG_ACPI_SATA	= (1<<  17), /* need native SATA ACPI layout */

Seems reasonable to me, though we may want to print something out during 
device identification to indicate that we're using AA (like we do for 
ATAPI AN).

Re: [RFC] add DMA setup FIS auto-activate feature

[Jeff Garzik]

Robert Hancock wrote:
> Seems reasonable to me, though we may want to print something out during 
> device identification to indicate that we're using AA (like we do for 
> ATAPI AN).

Agreed -- the user needs some indication, somewhere, that this feature 
is enabled.

	Jeff

Re: [RFC] add DMA setup FIS auto-activate feature

[Tejun Heo]

Jeff Garzik wrote:
> Robert Hancock wrote:
>> Seems reasonable to me, though we may want to print something out
>> during device identification to indicate that we're using AA (like we
>> do for ATAPI AN).
> 
> Agreed -- the user needs some indication, somewhere, that this feature
> is enabled.

Also, the return value from ata_dev_set_feature() needs to be checked.
AC_ERR_DEV can be ignored.  Other errors should probably set
inhinit-AA bit and fail the current recovery trial.

Thanks.

-- 
tejun

Re: [RFC] add DMA setup FIS auto-activate feature

[Shaohua Li]

On Fri, Jul 24, 2009 at 04:52:32AM +0800, Tejun Heo wrote:
> Jeff Garzik wrote:
> > Robert Hancock wrote:
> >> Seems reasonable to me, though we may want to print something out
> >> during device identification to indicate that we're using AA (like we
> >> do for ATAPI AN).
> > 
> > Agreed -- the user needs some indication, somewhere, that this feature
> > is enabled.
> 
> Also, the return value from ata_dev_set_feature() needs to be checked.
> AC_ERR_DEV can be ignored.  Other errors should probably set
> inhinit-AA bit and fail the current recovery trial.
Ok, updated patch.

Add SATA DMA setup FIS auto-activate feature in AHCI.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 336eb1e..d5d67b8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2802,7 +2802,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* prepare host */
 	if (hpriv->cap & HOST_CAP_NCQ)
-		pi.flags |= ATA_FLAG_NCQ;
+		pi.flags |= ATA_FLAG_NCQ | ATA_FLAG_FPDMA_AA;
 
 	if (hpriv->cap & HOST_CAP_PMP)
 		pi.flags |= ATA_FLAG_PMP;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2c6aeda..68f60f3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2298,29 +2298,48 @@ static inline u8 ata_dev_knobble(struct ata_device *dev)
 	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
 }
 
-static void ata_dev_config_ncq(struct ata_device *dev,
+static int ata_dev_config_ncq(struct ata_device *dev,
 			       char *desc, size_t desc_sz)
 {
 	struct ata_port *ap = dev->link->ap;
 	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
+	unsigned int err_mask;
+	char *aa_desc = "";
 
 	if (!ata_id_has_ncq(dev->id)) {
 		desc[0] = '\0';
-		return;
+		return 0;
 	}
 	if (dev->horkage & ATA_HORKAGE_NONCQ) {
 		snprintf(desc, desc_sz, "NCQ (not used)");
-		return;
+		return 0;
 	}
 	if (ap->flags & ATA_FLAG_NCQ) {
 		hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
 		dev->flags |= ATA_DFLAG_NCQ;
 	}
 
+	if (!(dev->horkage & ATA_HORKAGE_FPDMA_AA) &&
+		(ap->flags & ATA_FLAG_FPDMA_AA) &&
+		ata_id_has_fpdma_aa(dev->id)) {
+		err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
+			SATA_FPDMA_AA);
+		if (err_mask) {
+			ata_dev_printk(dev, KERN_ERR, "failed to enable AA"
+				"(error_mask=0x%x)\n", err_mask);
+			if (err_mask != AC_ERR_DEV)
+				dev->horkage |= ATA_HORKAGE_FPDMA_AA;
+			return -EIO;
+		} else
+			aa_desc = ", AA";
+	}
+
 	if (hdepth >= ddepth)
-		snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
+		snprintf(desc, desc_sz, "NCQ (depth %d)%s", ddepth, aa_desc);
 	else
-		snprintf(desc, desc_sz, "NCQ (depth %d/%d)", hdepth, ddepth);
+		snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
+			ddepth, aa_desc);
+	return 0;
 }
 
 /**
@@ -2460,7 +2479,7 @@ int ata_dev_configure(struct ata_device *dev)
 
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;
-			char ncq_desc[20];
+			char ncq_desc[24];
 
 			lba_desc = "LBA";
 			dev->flags |= ATA_DFLAG_LBA;
@@ -2474,7 +2493,9 @@ int ata_dev_configure(struct ata_device *dev)
 			}
 
 			/* config NCQ */
-			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
+			rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
+			if (rc)
+				return rc;
 
 			/* print device info to dmesg */
 			if (ata_msg_drv(ap) && print_info) {
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 9c75921..f549405 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -306,6 +306,7 @@ enum {
 	/* SETFEATURE Sector counts for SATA features */
 	SATA_AN			= 0x05,  /* Asynchronous Notification */
 	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
+	SATA_FPDMA_AA		= 0x02,  /* DMA Setup FIS Auto-Activate */
 
 	/* feature values for SET_MAX */
 	ATA_SET_MAX_ADDR	= 0x00,
@@ -525,6 +526,9 @@ static inline int ata_is_data(u8 prot)
 #define ata_id_has_atapi_AN(id)	\
 	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
 	  ((id)[78] & (1 << 5)) )
+#define ata_id_has_fpdma_aa(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 2)) )
 #define ata_id_iordy_disable(id) ((id)[ATA_ID_CAPABILITY] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[ATA_ID_CAPABILITY] & (1 << 11))
 #define ata_id_u32(id,n)	\
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 79b6d7f..a8232b9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -190,6 +190,7 @@ enum {
 	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1 << 11), /* don't spindown before poweroff */
 	ATA_FLAG_NO_HIBERNATE_SPINDOWN = (1 << 12), /* don't spindown before hibernation */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
+	ATA_FLAG_FPDMA_AA		= (1 << 14), /* driver supports Auto-Activate */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
 	ATA_FLAG_ACPI_SATA	= (1 << 17), /* need native SATA ACPI layout */
@@ -386,6 +387,7 @@ enum {
 	ATA_HORKAGE_FIRMWARE_WARN = (1 << 12),	/* firmware update warning */
 	ATA_HORKAGE_1_5_GBPS	= (1 << 13),	/* force 1.5 Gbps */
 	ATA_HORKAGE_NOSETXFER	= (1 << 14),	/* skip SETXFER, SATA only */
+	ATA_HORKAGE_FPDMA_AA	= (1 << 15),	/* skip AA */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

Re: [RFC] add DMA setup FIS auto-activate feature

[Tejun Heo]

Hello, Shaohua.

Shaohua Li wrote:
> +	if (!(dev->horkage & ATA_HORKAGE_FPDMA_AA) &&
> +		(ap->flags & ATA_FLAG_FPDMA_AA) &&
> +		ata_id_has_fpdma_aa(dev->id)) {
> +		err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
> +			SATA_FPDMA_AA);
> +		if (err_mask) {
> +			ata_dev_printk(dev, KERN_ERR, "failed to enable AA"
> +				"(error_mask=0x%x)\n", err_mask);
> +			if (err_mask != AC_ERR_DEV)
> +				dev->horkage |= ATA_HORKAGE_FPDMA_AA;
> +			return -EIO;

You can continue if err_mask equals AC_ERR_DEV.  Also, the horkage
should probably named like ATA_HORKAGE_BROKEN_FPDMA_AA.

Thanks.

-- 
tejun

Re: [RFC] add DMA setup FIS auto-activate feature

[Shaohua Li]

On Sat, Jul 25, 2009 at 08:47:32AM +0800, Tejun Heo wrote:
> Hello, Shaohua.
> 
> Shaohua Li wrote:
> > +	if (!(dev->horkage & ATA_HORKAGE_FPDMA_AA) &&
> > +		(ap->flags & ATA_FLAG_FPDMA_AA) &&
> > +		ata_id_has_fpdma_aa(dev->id)) {
> > +		err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
> > +			SATA_FPDMA_AA);
> > +		if (err_mask) {
> > +			ata_dev_printk(dev, KERN_ERR, "failed to enable AA"
> > +				"(error_mask=0x%x)\n", err_mask);
> > +			if (err_mask != AC_ERR_DEV)
> > +				dev->horkage |= ATA_HORKAGE_FPDMA_AA;
> > +			return -EIO;
> 
> You can continue if err_mask equals AC_ERR_DEV.  Also, the horkage
> should probably named like ATA_HORKAGE_BROKEN_FPDMA_AA.
Thanks for the suggestion. Hope this version is ok.

Add SATA DMA setup FIS auto-activate feature in AHCI.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 336eb1e..d5d67b8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2802,7 +2802,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* prepare host */
 	if (hpriv->cap & HOST_CAP_NCQ)
-		pi.flags |= ATA_FLAG_NCQ;
+		pi.flags |= ATA_FLAG_NCQ | ATA_FLAG_FPDMA_AA;
 
 	if (hpriv->cap & HOST_CAP_PMP)
 		pi.flags |= ATA_FLAG_PMP;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2c6aeda..8d2a308 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2298,29 +2298,49 @@ static inline u8 ata_dev_knobble(struct ata_device *dev)
 	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
 }
 
-static void ata_dev_config_ncq(struct ata_device *dev,
+static int ata_dev_config_ncq(struct ata_device *dev,
 			       char *desc, size_t desc_sz)
 {
 	struct ata_port *ap = dev->link->ap;
 	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
+	unsigned int err_mask;
+	char *aa_desc = "";
 
 	if (!ata_id_has_ncq(dev->id)) {
 		desc[0] = '\0';
-		return;
+		return 0;
 	}
 	if (dev->horkage & ATA_HORKAGE_NONCQ) {
 		snprintf(desc, desc_sz, "NCQ (not used)");
-		return;
+		return 0;
 	}
 	if (ap->flags & ATA_FLAG_NCQ) {
 		hdepth = min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);
 		dev->flags |= ATA_DFLAG_NCQ;
 	}
 
+	if (!(dev->horkage & ATA_HORKAGE_BROKEN_FPDMA_AA) &&
+		(ap->flags & ATA_FLAG_FPDMA_AA) &&
+		ata_id_has_fpdma_aa(dev->id)) {
+		err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
+			SATA_FPDMA_AA);
+		if (err_mask) {
+			ata_dev_printk(dev, KERN_ERR, "failed to enable AA"
+				"(error_mask=0x%x)\n", err_mask);
+			if (err_mask != AC_ERR_DEV) {
+				dev->horkage |= ATA_HORKAGE_BROKEN_FPDMA_AA;
+				return -EIO;
+			}
+		} else
+			aa_desc = ", AA";
+	}
+
 	if (hdepth >= ddepth)
-		snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
+		snprintf(desc, desc_sz, "NCQ (depth %d)%s", ddepth, aa_desc);
 	else
-		snprintf(desc, desc_sz, "NCQ (depth %d/%d)", hdepth, ddepth);
+		snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
+			ddepth, aa_desc);
+	return 0;
 }
 
 /**
@@ -2460,7 +2480,7 @@ int ata_dev_configure(struct ata_device *dev)
 
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;
-			char ncq_desc[20];
+			char ncq_desc[24];
 
 			lba_desc = "LBA";
 			dev->flags |= ATA_DFLAG_LBA;
@@ -2474,7 +2494,9 @@ int ata_dev_configure(struct ata_device *dev)
 			}
 
 			/* config NCQ */
-			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
+			rc = ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
+			if (rc)
+				return rc;
 
 			/* print device info to dmesg */
 			if (ata_msg_drv(ap) && print_info) {
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 9c75921..f549405 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -306,6 +306,7 @@ enum {
 	/* SETFEATURE Sector counts for SATA features */
 	SATA_AN			= 0x05,  /* Asynchronous Notification */
 	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
+	SATA_FPDMA_AA		= 0x02,  /* DMA Setup FIS Auto-Activate */
 
 	/* feature values for SET_MAX */
 	ATA_SET_MAX_ADDR	= 0x00,
@@ -525,6 +526,9 @@ static inline int ata_is_data(u8 prot)
 #define ata_id_has_atapi_AN(id)	\
 	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
 	  ((id)[78] & (1 << 5)) )
+#define ata_id_has_fpdma_aa(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 2)) )
 #define ata_id_iordy_disable(id) ((id)[ATA_ID_CAPABILITY] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[ATA_ID_CAPABILITY] & (1 << 11))
 #define ata_id_u32(id,n)	\
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 79b6d7f..240dbef 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -190,6 +190,7 @@ enum {
 	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1 << 11), /* don't spindown before poweroff */
 	ATA_FLAG_NO_HIBERNATE_SPINDOWN = (1 << 12), /* don't spindown before hibernation */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
+	ATA_FLAG_FPDMA_AA		= (1 << 14), /* driver supports Auto-Activate */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
 	ATA_FLAG_ACPI_SATA	= (1 << 17), /* need native SATA ACPI layout */
@@ -386,6 +387,7 @@ enum {
 	ATA_HORKAGE_FIRMWARE_WARN = (1 << 12),	/* firmware update warning */
 	ATA_HORKAGE_1_5_GBPS	= (1 << 13),	/* force 1.5 Gbps */
 	ATA_HORKAGE_NOSETXFER	= (1 << 14),	/* skip SETXFER, SATA only */
+	ATA_HORKAGE_BROKEN_FPDMA_AA	= (1 << 15),	/* skip AA */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */

Re: [RFC] add DMA setup FIS auto-activate feature

[Tejun Heo]

Shaohua Li wrote:
> On Sat, Jul 25, 2009 at 08:47:32AM +0800, Tejun Heo wrote:
>> Hello, Shaohua.
>>
>> Shaohua Li wrote:
>>> +	if (!(dev->horkage & ATA_HORKAGE_FPDMA_AA) &&
>>> +		(ap->flags & ATA_FLAG_FPDMA_AA) &&
>>> +		ata_id_has_fpdma_aa(dev->id)) {
>>> +		err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
>>> +			SATA_FPDMA_AA);
>>> +		if (err_mask) {
>>> +			ata_dev_printk(dev, KERN_ERR, "failed to enable AA"
>>> +				"(error_mask=0x%x)\n", err_mask);
>>> +			if (err_mask != AC_ERR_DEV)
>>> +				dev->horkage |= ATA_HORKAGE_FPDMA_AA;
>>> +			return -EIO;
>> You can continue if err_mask equals AC_ERR_DEV.  Also, the horkage
>> should probably named like ATA_HORKAGE_BROKEN_FPDMA_AA.
> Thanks for the suggestion. Hope this version is ok.
> 
> Add SATA DMA setup FIS auto-activate feature in AHCI.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [RFC] add DMA setup FIS auto-activate feature

[Jeff Garzik]

Shaohua Li wrote:
> On Sat, Jul 25, 2009 at 08:47:32AM +0800, Tejun Heo wrote:
>> Hello, Shaohua.
>>
>> Shaohua Li wrote:
>>> +	if (!(dev->horkage & ATA_HORKAGE_FPDMA_AA) &&
>>> +		(ap->flags & ATA_FLAG_FPDMA_AA) &&
>>> +		ata_id_has_fpdma_aa(dev->id)) {
>>> +		err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
>>> +			SATA_FPDMA_AA);
>>> +		if (err_mask) {
>>> +			ata_dev_printk(dev, KERN_ERR, "failed to enable AA"
>>> +				"(error_mask=0x%x)\n", err_mask);
>>> +			if (err_mask != AC_ERR_DEV)
>>> +				dev->horkage |= ATA_HORKAGE_FPDMA_AA;
>>> +			return -EIO;
>> You can continue if err_mask equals AC_ERR_DEV.  Also, the horkage
>> should probably named like ATA_HORKAGE_BROKEN_FPDMA_AA.
> Thanks for the suggestion. Hope this version is ok.
> 
> Add SATA DMA setup FIS auto-activate feature in AHCI.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

applied to #upstream -- let's see what melts!  <grin>

Re: [RFC] add DMA setup FIS auto-activate feature

[Robert Hancock]

On 07/22/2009 11:45 PM, Tejun Heo wrote:
> Shaohua Li wrote:
>>> IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0
>>> hardware may not behave properly in response, no?
>> It appears the AHCI spec doesn't define a HBA capability about this
>> feature, but I'm likely wrong as I'm not quite familar with SATA.
>
> IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement
> the spec correctly, it should work.  But even for ahcis, if we enable
> it by default, I'm fairly sure we'll be met by a number of unpleasant
> surprises.  Not sure whether doing that would be worth the trouble or
> not.

There's definitely a controller dependency here, as SATA 1.0 hardware 
won't respond properly if this feature is turned on. Likely we need a 
host flag to indicate whether the controller supports auto-activate. It 
seems like AHCI shouldn't be a problem, but it's not clear if any other 
controller types would support it.

As far as busted hardware not handling it properly.. well if it just 
ignores the enabling then that's not a problem as things will just work 
as before. There could be devices that claim support, don't ignore it 
but don't handle it properly.. but realistically the only way we can 
find out if that's the case is turning it on and see what breaks.

Re: [RFC] add DMA setup FIS auto-activate feature

[Sergei Shtylyov]

Hello.

Shaohua Li wrote:

> hi,
> The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> transfers. I had an attempt to add it, though my test doesn't show obvious
> performance improvement (not worse too), I wonder why not add this feature?
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 2c6aeda..53cc355 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2303,6 +2303,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>  {
>  	struct ata_port *ap = dev->link->ap;
>  	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
> +	const u16 *id = dev->id;
>   

   Why introduce the variable if you're only using it once?

>  	if (!ata_id_has_ncq(dev->id)) {
>   

   Should be changed to just 'id'...

>  		desc[0] = '\0';
> @@ -2317,6 +2318,9 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>  		dev->flags |= ATA_DFLAG_NCQ;
>  	}
>  
> +	if (ata_id_has_daa(id))
> +		ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE, SATA_DAA);
> +
>   

MBR, Sergei