[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 <***@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) \


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

[Tejun Heo]

Post by 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?

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.

[Jeff Garzik]

Post by 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?

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


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

[Shaohua Li]

Post by Jeff Garzik

Post by 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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Tejun Heo]

Post by Shaohua Li

Post by Jeff Garzik
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.

[Jeff Garzik]

Post by Tejun Heo
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


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

[Tejun Heo]

Post by Jeff Garzik

Post by Tejun Heo
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.

[Jeff Garzik]

Post by Tejun Heo

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


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

[Shaohua Li]

Post by Jeff Garzik

Post by Tejun Heo

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 <***@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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Jeff Garzik]

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


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

[Tejun Heo]

Post by Jeff Garzik

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.

[Shaohua Li]

Post by Tejun Heo

Post by Jeff Garzik

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 <***@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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Tejun Heo]

Hello, Shaohua.

Post by Shaohua Li
+ 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.

[Shaohua Li]

Post by Tejun Heo
Hello, Shaohua.

Post by Shaohua Li
+ 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 <***@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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Tejun Heo]

Post by Shaohua Li

Post by Tejun Heo
Hello, Shaohua.

Post by Shaohua Li
+ 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.

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

Thanks.

[Jeff Garzik]

Post by Shaohua Li

Post by Tejun Heo
Hello, Shaohua.

Post by Shaohua Li
+ 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.

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


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

[Robert Hancock]

Post by Shaohua Li

Post by Jeff Garzik

Post by Tejun Heo

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.
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).
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Robert Hancock]

Post by Tejun Heo

Post by Shaohua Li

Post by Jeff Garzik
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sergei Shtylyov]

Hello.

Post by 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?
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?

Post by Shaohua Li
if (!ata_id_has_ncq(dev->id)) {

Should be changed to just 'id'...

Post by Shaohua Li
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


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