[PATCHSET #upstram-fixes] libata-acpi: improve ACPI corner case handling

[PATCHSET #upstram-fixes] libata-acpi: improve ACPI corner case handling

[Tejun Heo]

Hello,

This patchset contains the following patches to improve ACPI corner
case handling.

  0001-libata-drop-const-from-struct-ata_port-param-on-ata.patch
  0002-libata-update-ata_-_printk-macros-such-that-level.patch
  0003-libata-add-more-opcodes-to-ata.h.patch
  0004-libata-implement-ata_acpi_init_gtm.patch
  0005-libata-use-init_gtm-in-ata_acpi_cbl_80wire.patch
  0006-libata-acpi-improve-ACPI-disabling.patch
  0007-libata-acpi-improve-_GTF-execution-error-handling-a.patch
  0008-libata-acpi-ignore-_GTM-failure-during-suspend.patch
  0009-libata-acpi-implement-_GTF-command-filtering-and-fi.patch

The first three patches are misc prep patches.

0004-0005 backport init_gtm from #upstream to fix ACPI evaluation
failure on some boards and fix ACPI cable detection.

0006-0008 improve ATA ACPI corner case / error handling.  With these
changes, most ACPI errors don't cause slow down or other side effects
for probing and suspend/resuming.

0009 implements command filtering such that certain _GTF commands are
filtered.  Filtering can be controlled with kernel parameter.
Currently only lock / freeze lock commands are filtered out.

This fixes all reported ATA ACPI related regressions.  There can be
cases where _GTM evaluation explodes but it doesn't affect operation
at all and the correct solution seems to be suppress those messages.

Thanks.

--
tejun

[PATCH 1/9] libata: drop const from struct ata_port param on ata_acpi_gtm/stm()

[Tejun Heo]

No other libata interface function takes const ata_port.  Drop them
from ata_acpi_gtm/stm().

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |    4 ++--
 include/linux/libata.h    |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 545ea86..e55cb4b 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -200,7 +200,7 @@ void ata_acpi_associate(struct ata_host *host)
  * RETURNS:
  * 0 on success, -ENOENT if _GTM doesn't exist, -errno on failure.
  */
-int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *gtm)
+int ata_acpi_gtm(struct ata_port *ap, struct ata_acpi_gtm *gtm)
 {
 	struct acpi_buffer output = { .length = ACPI_ALLOCATE_BUFFER };
 	union acpi_object *out_obj;
@@ -259,7 +259,7 @@ EXPORT_SYMBOL_GPL(ata_acpi_gtm);
  * RETURNS:
  * 0 on success, -ENOENT if _STM doesn't exist, -errno on failure.
  */
-int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm)
+int ata_acpi_stm(struct ata_port *ap, struct ata_acpi_gtm *stm)
 {
 	acpi_status status;
 	struct acpi_object_list         input;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index ef52a07..6940344 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -940,8 +940,8 @@ enum {
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
 extern int ata_acpi_cbl_80wire(struct ata_port *ap);
-int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
-int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
+int ata_acpi_stm(struct ata_port *ap, struct ata_acpi_gtm *stm);
+int ata_acpi_gtm(struct ata_port *ap, struct ata_acpi_gtm *stm);
 #else
 static inline int ata_acpi_cbl_80wire(struct ata_port *ap) { return 0; }
 #endif
-- 
1.5.2.4

[PATCH 2/9] libata: update ata_*_printk() macros such that level can be a variable

[Tejun Heo]

Make prink helpers format @lv together rather than prepending to the
format string as constant.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 include/linux/libata.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6940344..d3a0caf 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1013,18 +1013,18 @@ extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
  * printk helpers
  */
 #define ata_port_printk(ap, lv, fmt, args...) \
-	printk(lv"ata%u: "fmt, (ap)->print_id , ##args)
+	printk("%sata%u: "fmt, lv, (ap)->print_id , ##args)
 
 #define ata_link_printk(link, lv, fmt, args...) do { \
 	if ((link)->ap->nr_pmp_links) \
-		printk(lv"ata%u.%02u: "fmt, (link)->ap->print_id, \
+		printk("%sata%u.%02u: "fmt, lv, (link)->ap->print_id,	\
 		       (link)->pmp , ##args); \
 	else \
-		printk(lv"ata%u: "fmt, (link)->ap->print_id , ##args); \
+		printk("%sata%u: "fmt, lv, (link)->ap->print_id , ##args); \
 	} while(0)
 
 #define ata_dev_printk(dev, lv, fmt, args...) \
-	printk(lv"ata%u.%02u: "fmt, (dev)->link->ap->print_id, \
+	printk("%sata%u.%02u: "fmt, lv, (dev)->link->ap->print_id,	\
 	       (dev)->link->pmp + (dev)->devno , ##args)
 
 /*
-- 
1.5.2.4

[PATCH 3/9] libata: add more opcodes to ata.h

[Tejun Heo]

Add constants for DEVICE CONFIGURATION OVERLAY and SET_MAX to
include/linux/ata.h.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 include/linux/ata.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/ata.h b/include/linux/ata.h
index 5c4e54a..72ab808 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -190,6 +190,8 @@ enum {
 	ATA_CMD_READ_LOG_EXT	= 0x2f,
 	ATA_CMD_PMP_READ	= 0xE4,
 	ATA_CMD_PMP_WRITE	= 0xE8,
+	ATA_CMD_CONF_OVERLAY	= 0xB1,
+	ATA_CMD_SEC_FREEZE_LOCK	= 0xF5,
 
 	/* READ_LOG_EXT pages */
 	ATA_LOG_SATA_NCQ	= 0x10,
@@ -239,6 +241,19 @@ enum {
 	SATA_AN			= 0x05,  /* Asynchronous Notification */
 	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
 
+	/* feature values for SET_MAX */
+	ATA_SET_MAX_ADDR	= 0x00,
+	ATA_SET_MAX_PASSWD	= 0x01,
+	ATA_SET_MAX_LOCK	= 0x02,
+	ATA_SET_MAX_UNLOCK	= 0x03,
+	ATA_SET_MAX_FREEZE_LOCK	= 0x04,
+
+	/* feature values for DEVICE CONFIGURATION OVERLAY */
+	ATA_DCO_RESTORE		= 0xC0,
+	ATA_DCO_FREEZE_LOCK	= 0xC1,
+	ATA_DCO_IDENTIFY	= 0xC2,
+	ATA_DCO_SET		= 0xC3,
+
 	/* ATAPI stuff */
 	ATAPI_PKT_DMA		= (1 << 0),
 	ATAPI_DMADIR		= (1 << 2),	/* ATAPI data dir:
-- 
1.5.2.4

[PATCH 4/9] libata: implement ata_acpi_init_gtm()

[Tejun Heo]

Add dev->__acpi_init_gtm and store initial GTM values on host
initialization.  If the field is valid, ata_acpi_init_gtm() returns
pointer to the saved GTM structure; otherwise, NULL.  This is to
remember BIOS/firmware programmed initial timing for later use before
reset and mode configuration modify it.  The accessor is there to make
building w/o ACPI easy dev->__acpi_init doesn't exist if ACPI is not
enabled.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Signed-off-by: Jeff Garzik <jeff@garzik.org>
---
 drivers/ata/libata-acpi.c |    3 +++
 include/linux/libata.h    |   12 ++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index e55cb4b..e4ec391 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -94,6 +94,9 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 
 		dev->acpi_handle = acpi_get_child(ap->acpi_handle, i);
 	}
+
+	if (ata_acpi_gtm(ap, &ap->__acpi_init_gtm) == 0)
+		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct kobject *kobj,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d3a0caf..bc8ef5b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -212,6 +212,7 @@ enum {
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 	ATA_PFLAG_GTM_VALID	= (1 << 19), /* acpi_gtm data valid */
+	ATA_PFLAG_INIT_GTM_VALID = (1 << 20), /* initial gtm data valid */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */
@@ -654,6 +655,7 @@ struct ata_port {
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
 	struct ata_acpi_gtm	acpi_gtm;
+	struct ata_acpi_gtm	__acpi_init_gtm; /* use ata_acpi_init_gtm() */
 #endif
 	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
 };
@@ -939,10 +941,20 @@ enum {
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
+static inline const struct ata_acpi_gtm *ata_acpi_init_gtm(struct ata_port *ap)
+{
+	if (ap->pflags & ATA_PFLAG_INIT_GTM_VALID)
+		return &ap->__acpi_init_gtm;
+	return NULL;
+}
 extern int ata_acpi_cbl_80wire(struct ata_port *ap);
 int ata_acpi_stm(struct ata_port *ap, struct ata_acpi_gtm *stm);
 int ata_acpi_gtm(struct ata_port *ap, struct ata_acpi_gtm *stm);
 #else
+static inline const struct ata_acpi_gtm *ata_acpi_init_gtm(struct ata_port *ap)
+{
+	return NULL;
+}
 static inline int ata_acpi_cbl_80wire(struct ata_port *ap) { return 0; }
 #endif
 
-- 
1.5.2.4

[PATCH 5/9] libata: use init_gtm in ata_acpi_cbl_80wire()

[Tejun Heo]

ata_acpi_cbl_80wire() is called from cable detection where transfer
spped is forced to PIO0 after resetting.  On some controllers, this
results in incorrect result or ACPI evaluation failure.  Use cached
initial _GTM value.

This is partial fix for 2.6.24.  More extensive update to ACPI mode
filtering is queued for 2.6.25.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index e4ec391..5785cac 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -396,22 +396,22 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf,
 
 int ata_acpi_cbl_80wire(struct ata_port *ap)
 {
-	struct ata_acpi_gtm gtm;
+	const struct ata_acpi_gtm *gtm;
 	int valid = 0;
 
-	/* No _GTM data, no information */
-	if (ata_acpi_gtm(ap, &gtm) < 0)
+	gtm = ata_acpi_init_gtm(ap);
+	if (!gtm)
 		return 0;
 
 	/* Split timing, DMA enabled */
-	if ((gtm.flags & 0x11) == 0x11 && gtm.drive[0].dma < 55)
+	if ((gtm->flags & 0x11) == 0x11 && gtm->drive[0].dma < 55)
 		valid |= 1;
-	if ((gtm.flags & 0x14) == 0x14 && gtm.drive[1].dma < 55)
+	if ((gtm->flags & 0x14) == 0x14 && gtm->drive[1].dma < 55)
 		valid |= 2;
 	/* Shared timing, DMA enabled */
-	if ((gtm.flags & 0x11) == 0x01 && gtm.drive[0].dma < 55)
+	if ((gtm->flags & 0x11) == 0x01 && gtm->drive[0].dma < 55)
 		valid |= 1;
-	if ((gtm.flags & 0x14) == 0x04 && gtm.drive[0].dma < 55)
+	if ((gtm->flags & 0x14) == 0x04 && gtm->drive[0].dma < 55)
 		valid |= 2;
 
 	/* Drive check */
-- 
1.5.2.4

[PATCH 6/9] libata-acpi: improve ACPI disabling

[Tejun Heo]

* If _GTF evalution fails, it's pointless to retry.  Just disable ATA
  ACPI.

* After disabling ACPI, return success iff the number of executed _GTF
  command equals zero.  Otherwise, tell EH to retry.  This change
  fixes bogus 1 return bug where ata_acpi_on_devcfg() expects the
  caller to reload IDENTIFY data and continue but the caller
  interprets it as an error.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |   60 +++++++++++++++++++++++++++++---------------
 1 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 5785cac..a118c3a 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -314,8 +314,8 @@ EXPORT_SYMBOL_GPL(ata_acpi_stm);
  * EH context.
  *
  * RETURNS:
- * Number of taskfiles on success, 0 if _GTF doesn't exist or doesn't
- * contain valid data.
+ * Number of taskfiles on success, 0 if _GTF doesn't exist.  -EINVAL
+ * if _GTF is invalid.
  */
 static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf,
 			   void **ptr_to_free)
@@ -342,6 +342,7 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf,
 			ata_dev_printk(dev, KERN_WARNING,
 				       "_GTF evaluation failed (AE 0x%x)\n",
 				       status);
+			rc = -EINVAL;
 		}
 		goto out_free;
 	}
@@ -353,6 +354,7 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf,
 				__FUNCTION__,
 				(unsigned long long)output.length,
 				output.pointer);
+		rc = -EINVAL;
 		goto out_free;
 	}
 
@@ -361,6 +363,7 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf,
 		ata_dev_printk(dev, KERN_WARNING,
 			       "_GTF unexpected object type 0x%x\n",
 			       out_obj->type);
+		rc = -EINVAL;
 		goto out_free;
 	}
 
@@ -368,6 +371,7 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf,
 		ata_dev_printk(dev, KERN_WARNING,
 			       "unexpected _GTF length (%d)\n",
 			       out_obj->buffer.length);
+		rc = -EINVAL;
 		goto out_free;
 	}
 
@@ -494,6 +498,7 @@ static int taskfile_load_raw(struct ata_device *dev,
 /**
  * ata_acpi_exec_tfs - get then write drive taskfile settings
  * @dev: target ATA device
+ * @nr_executed: out paramter for the number of executed commands
  *
  * Evaluate _GTF and excute returned taskfiles.
  *
@@ -501,17 +506,20 @@ static int taskfile_load_raw(struct ata_device *dev,
  * EH context.
  *
  * RETURNS:
- * Number of executed taskfiles on success, 0 if _GTF doesn't exist or
- * doesn't contain valid data.  -errno on other errors.
+ * Number of executed taskfiles on success, 0 if _GTF doesn't exist.
+ * -errno on other errors.
  */
-static int ata_acpi_exec_tfs(struct ata_device *dev)
+static int ata_acpi_exec_tfs(struct ata_device *dev, int *nr_executed)
 {
 	struct ata_acpi_gtf *gtf = NULL;
 	void *ptr_to_free = NULL;
 	int gtf_count, i, rc;
 
 	/* get taskfiles */
-	gtf_count = ata_dev_get_GTF(dev, &gtf, &ptr_to_free);
+	rc = ata_dev_get_GTF(dev, &gtf, &ptr_to_free);
+	if (rc < 0)
+		return rc;
+	gtf_count = rc;
 
 	/* execute them */
 	for (i = 0, rc = 0; i < gtf_count; i++) {
@@ -523,12 +531,12 @@ static int ata_acpi_exec_tfs(struct ata_device *dev)
 		tmp = taskfile_load_raw(dev, gtf++);
 		if (!rc)
 			rc = tmp;
+
+		(*nr_executed)++;
 	}
 
 	kfree(ptr_to_free);
 
-	if (rc == 0)
-		return gtf_count;
 	return rc;
 }
 
@@ -667,6 +675,8 @@ int ata_acpi_on_devcfg(struct ata_device *dev)
 	struct ata_port *ap = dev->link->ap;
 	struct ata_eh_context *ehc = &ap->link.eh_context;
 	int acpi_sata = ap->flags & ATA_FLAG_ACPI_SATA;
+	int nr_executed = 0;
+	const char *reason;
 	int rc;
 
 	if (!dev->acpi_handle)
@@ -685,14 +695,14 @@ int ata_acpi_on_devcfg(struct ata_device *dev)
 	}
 
 	/* do _GTF */
-	rc = ata_acpi_exec_tfs(dev);
-	if (rc < 0)
+	rc = ata_acpi_exec_tfs(dev, &nr_executed);
+	if (rc)
 		goto acpi_err;
 
 	dev->flags &= ~ATA_DFLAG_ACPI_PENDING;
 
 	/* refresh IDENTIFY page if any _GTF command has been executed */
-	if (rc > 0) {
+	if (nr_executed) {
 		rc = ata_dev_reread_id(dev, 0);
 		if (rc < 0) {
 			ata_dev_printk(dev, KERN_ERR, "failed to IDENTIFY "
@@ -704,17 +714,25 @@ int ata_acpi_on_devcfg(struct ata_device *dev)
 	return 0;
 
  acpi_err:
-	/* let EH retry on the first failure, disable ACPI on the second */
-	if (dev->flags & ATA_DFLAG_ACPI_FAILED) {
-		ata_dev_printk(dev, KERN_WARNING, "ACPI on devcfg failed the "
-			       "second time, disabling (errno=%d)\n", rc);
+	/* fail and let EH retry once more for unknown IO errors */
+	if (rc != -EINVAL && !(dev->flags & ATA_DFLAG_ACPI_FAILED)) {
+		dev->flags |= ATA_DFLAG_ACPI_FAILED;
+		return rc;
+	}
 
-		dev->acpi_handle = NULL;
+	if (rc == -EINVAL)
+		reason = "_GTF invalid";
+	else
+		reason = "failed the second time";
+
+	ata_dev_printk(dev, KERN_WARNING, "ACPI: %s, disabled\n", reason);
+	dev->acpi_handle = NULL;
+
+	/* We can safely continue if no _GTF command has been executed
+	 * and port is not frozen.
+	 */
+	if (!nr_executed && !(ap->pflags & ATA_PFLAG_FROZEN))
+		return 0;
 
-		/* if port is working, request IDENTIFY reload and continue */
-		if (!(ap->pflags & ATA_PFLAG_FROZEN))
-			rc = 1;
-	}
-	dev->flags |= ATA_DFLAG_ACPI_FAILED;
 	return rc;
 }
-- 
1.5.2.4

[PATCH 7/9] libata-acpi: improve _GTF execution error handling and reporting

[Tejun Heo]

As _GTF commands can't transfer data, device error never signals
transfer error.  It indicates that the device vetoed the operation, so
it's meaningless to retry.

This patch makes libata-acpi to report and continue on device errors
when executing _GTF commands.  Also commands rejected by device don't
contributed to the number of _GTF commands executed.

While at it, update _GTF execution reporting such that all successful
commands are logged at KERN_DEBUG and rename taskfile_load_raw() to
ata_acpi_run_tf() for consistency.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |   77 +++++++++++++++++++++++++++------------------
 1 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index a118c3a..e68e918 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -429,7 +429,7 @@ int ata_acpi_cbl_80wire(struct ata_port *ap)
 EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
 
 /**
- * taskfile_load_raw - send taskfile registers to host controller
+ * ata_acpi_run_tf - send taskfile registers to host controller
  * @dev: target ATA device
  * @gtf: raw ATA taskfile register set (0x1f1 - 0x1f7)
  *
@@ -448,14 +448,17 @@ EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
  * EH context.
  *
  * RETURNS:
- * 0 on success, -errno on failure.
+ * 1 if command is executed successfully.  0 if ignored or rejected,
+ * -errno on other errors.
  */
-static int taskfile_load_raw(struct ata_device *dev,
-			      const struct ata_acpi_gtf *gtf)
+static int ata_acpi_run_tf(struct ata_device *dev,
+			   const struct ata_acpi_gtf *gtf)
 {
-	struct ata_port *ap = dev->link->ap;
 	struct ata_taskfile tf, rtf;
 	unsigned int err_mask;
+	const char *level;
+	char msg[60];
+	int rc;
 
 	if ((gtf->tf[0] == 0) && (gtf->tf[1] == 0) && (gtf->tf[2] == 0)
 	    && (gtf->tf[3] == 0) && (gtf->tf[4] == 0) && (gtf->tf[5] == 0)
@@ -475,24 +478,39 @@ static int taskfile_load_raw(struct ata_device *dev,
 	tf.device  = gtf->tf[5];	/* 0x1f6 */
 	tf.command = gtf->tf[6];	/* 0x1f7 */
 
-	if (ata_msg_probe(ap))
-		ata_dev_printk(dev, KERN_DEBUG, "executing ACPI cmd "
-			       "%02x/%02x:%02x:%02x:%02x:%02x:%02x\n",
-			       tf.command, tf.feature, tf.nsect,
-			       tf.lbal, tf.lbam, tf.lbah, tf.device);
-
 	rtf = tf;
 	err_mask = ata_exec_internal(dev, &rtf, NULL, DMA_NONE, NULL, 0, 0);
-	if (err_mask) {
-		ata_dev_printk(dev, KERN_ERR,
-			"ACPI cmd %02x/%02x:%02x:%02x:%02x:%02x:%02x failed "
-			"(Emask=0x%x Stat=0x%02x Err=0x%02x)\n",
-			tf.command, tf.feature, tf.nsect, tf.lbal, tf.lbam,
-			tf.lbah, tf.device, err_mask, rtf.command, rtf.feature);
-		return -EIO;
+
+	switch (err_mask) {
+	case 0:
+		level = KERN_DEBUG;
+		snprintf(msg, sizeof(msg), "succeeded");
+		rc = 1;
+		break;
+
+	case AC_ERR_DEV:
+		level = KERN_INFO;
+		snprintf(msg, sizeof(msg),
+			 "rejected by device (Stat=0x%02x Err=0x%02x)",
+			 rtf.command, rtf.feature);
+		rc = 0;
+		break;
+
+	default:
+		level = KERN_ERR;
+		snprintf(msg, sizeof(msg),
+			 "failed (Emask=0x%x Stat=0x%02x Err=0x%02x)",
+			 err_mask, rtf.command, rtf.feature);
+		rc = -EIO;
+		break;
 	}
 
-	return 0;
+	ata_dev_printk(dev, level,
+		       "ACPI cmd %02x/%02x:%02x:%02x:%02x:%02x:%02x %s\n",
+		       tf.command, tf.feature, tf.nsect, tf.lbal,
+		       tf.lbam, tf.lbah, tf.device, msg);
+
+	return rc;
 }
 
 /**
@@ -522,22 +540,19 @@ static int ata_acpi_exec_tfs(struct ata_device *dev, int *nr_executed)
 	gtf_count = rc;
 
 	/* execute them */
-	for (i = 0, rc = 0; i < gtf_count; i++) {
-		int tmp;
-
-		/* ACPI errors are eventually ignored.  Run till the
-		 * end even after errors.
-		 */
-		tmp = taskfile_load_raw(dev, gtf++);
-		if (!rc)
-			rc = tmp;
-
-		(*nr_executed)++;
+	for (i = 0; i < gtf_count; i++) {
+		rc = ata_acpi_run_tf(dev, gtf++);
+		if (rc < 0)
+			break;
+		if (rc)
+			(*nr_executed)++;
 	}
 
 	kfree(ptr_to_free);
 
-	return rc;
+	if (rc < 0)
+		return rc;
+	return 0;
 }
 
 /**
-- 
1.5.2.4

Re: [PATCH 7/9] libata-acpi: improve _GTF execution error handling and reporting

[Mark Lord]

Tejun Heo wrote:
> As _GTF commands can't transfer data, device error never signals
> transfer error.  It indicates that the device vetoed the operation, so
> it's meaningless to retry.
..

The SECURITY commands may pass a data block to the drive,
containing the password required to unlock/freeze a drive.
I suspect ACPI on many machines will issue such a command,
and it does indeed transfer data (512 bytes).

????

Re: [PATCH 7/9] libata-acpi: improve _GTF execution error handling and reporting

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> As _GTF commands can't transfer data, device error never signals
>> transfer error.  It indicates that the device vetoed the operation, so
>> it's meaningless to retry.
> ..
> 
> The SECURITY commands may pass a data block to the drive,
> containing the password required to unlock/freeze a drive.
> I suspect ACPI on many machines will issue such a command,
> and it does indeed transfer data (512 bytes).

Surprise, ACPI _GTF can't do that.  Don't ask me why.  :-)

-- 
tejun

Re: [PATCH 7/9] libata-acpi: improve _GTF execution error handling and reporting

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> As _GTF commands can't transfer data, device error never signals
>>> transfer error.  It indicates that the device vetoed the operation, so
>>> it's meaningless to retry.
>> ..
>>
>> The SECURITY commands may pass a data block to the drive,
>> containing the password required to unlock/freeze a drive.
>> I suspect ACPI on many machines will issue such a command,
>> and it does indeed transfer data (512 bytes).
> 
> Surprise, ACPI _GTF can't do that.  Don't ask me why.  :-)
..

Then why are we (going to be) filtering those op's (one of your patches) ?

??
 

Re: [PATCH 7/9] libata-acpi: improve _GTF execution error handling and reporting

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
>>> Tejun Heo wrote:
>>>> As _GTF commands can't transfer data, device error never signals
>>>> transfer error.  It indicates that the device vetoed the operation, so
>>>> it's meaningless to retry.
>>> ..
>>>
>>> The SECURITY commands may pass a data block to the drive,
>>> containing the password required to unlock/freeze a drive.
>>> I suspect ACPI on many machines will issue such a command,
>>> and it does indeed transfer data (512 bytes).
>>
>> Surprise, ACPI _GTF can't do that.  Don't ask me why.  :-)
> ..
> 
> Then why are we (going to be) filtering those op's (one of your patches) ?

Filtered ones are not LOCK / FREEZE LOCK commands which prevent further
changes.  e.g. SET MAX FREEZE LOCK will prevent further size changes
using SET MAX [EXT].  It's bad for three reasons 1. it locks down drive
feature without user consent (e.g. can't unlock HPA) 2. it makes
debugging difficult by putting drives into different states 3. it just
doesn't serve any good.

-- 
tejun

Re: [PATCH 7/9] libata-acpi: improve _GTF execution error handling and reporting

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Mark Lord wrote:
>>>> Tejun Heo wrote:
>>>>> As _GTF commands can't transfer data, device error never signals
>>>>> transfer error.  It indicates that the device vetoed the operation, so
>>>>> it's meaningless to retry.
>>>> ..
>>>>
>>>> The SECURITY commands may pass a data block to the drive,
>>>> containing the password required to unlock/freeze a drive.
>>>> I suspect ACPI on many machines will issue such a command,
>>>> and it does indeed transfer data (512 bytes).
>>> Surprise, ACPI _GTF can't do that.  Don't ask me why.  :-)
>> ..
>>
>> Then why are we (going to be) filtering those op's (one of your patches) ?
> 
> Filtered ones are not LOCK / FREEZE LOCK commands which prevent further
> changes.  e.g. SET MAX FREEZE LOCK will prevent further size changes
> using SET MAX [EXT].  It's bad for three reasons 1. it locks down drive
> feature without user consent (e.g. can't unlock HPA) 2. it makes
> debugging difficult by putting drives into different states 3. it just
> doesn't serve any good.
..

Okay-dokey.  I think I mis-read which FREEZE command was in the patch.

Cheers!

[PATCH 8/9] libata-acpi: ignore _GTM failure during suspend

[Tejun Heo]

Don't fail suspend because of _GTM failure.  This might eventually
cause _GTF evaluation failure during resume but that's better than
failing suspend.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index e68e918..1981112 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -640,9 +640,12 @@ int ata_acpi_on_suspend(struct ata_port *ap)
 		ap->pflags &= ~ATA_PFLAG_GTM_VALID;
 	spin_unlock_irqrestore(ap->lock, flags);
 
-	if (rc == -ENOENT)
-		rc = 0;
-	return rc;
+	/* Don't fail suspend because of _GTM failure.  This will skip
+	 * _STM during resume and has the possibility of failing _GTF
+	 * but that's better than failing suspend/resume.  After all,
+	 * libata doesn't depend on ACPI to configure devices.
+	 */
+	return 0;
 }
 
 /**
-- 
1.5.2.4

[PATCH 9/9] libata-acpi: implement _GTF command filtering and filter out LOCK commands

[Tejun Heo]

Implement _GTF command filtering which can be controlled by
libata.acpi_filter kernel parameter and filter LOCK commands using it.
DCO FREEZE LOCK, SECURITY FREEZE LOCK and SET MAX [FREEZE] LOCK are
filtered by default.

Note that detecting SET MAX LOCK requires looking at the previous
command.  This adds a bit to code complexity.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |  148 +++++++++++++++++++++++++++++++-------------
 1 files changed, 104 insertions(+), 44 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 1981112..b487268 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2006 Randy Dunlap
  */
 
+#include <linux/module.h>
 #include <linux/ata.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -25,6 +26,16 @@
 #include <acpi/acmacros.h>
 #include <acpi/actypes.h>
 
+enum {
+	ATA_ACPI_FILTER_LOCK	= 1 << 0,
+
+	ATA_ACPI_FILTER_DEFAULT	= ATA_ACPI_FILTER_LOCK,
+};
+
+static unsigned int ata_acpi_gtf_filter = ATA_ACPI_FILTER_DEFAULT;
+module_param_named(acpi_gtf_filter, ata_acpi_gtf_filter, int, 0644);
+MODULE_PARM_DESC(acpi_gtf_filter, "filter mask for ACPI _GTF commands, set to filter out (0x1=lock/freeze lock)");
+
 #define NO_PORT_MULT		0xffff
 #define SATA_ADR(root, pmp)	(((root) << 16) | (pmp))
 
@@ -428,6 +439,51 @@ int ata_acpi_cbl_80wire(struct ata_port *ap)
 
 EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
 
+static void ata_acpi_gtf_to_tf(struct ata_device *dev,
+			       const struct ata_acpi_gtf *gtf,
+			       struct ata_taskfile *tf)
+{
+	ata_tf_init(dev, tf);
+
+	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf->protocol = ATA_PROT_NODATA;
+	tf->feature = gtf->tf[0];	/* 0x1f1 */
+	tf->nsect   = gtf->tf[1];	/* 0x1f2 */
+	tf->lbal    = gtf->tf[2];	/* 0x1f3 */
+	tf->lbam    = gtf->tf[3];	/* 0x1f4 */
+	tf->lbah    = gtf->tf[4];	/* 0x1f5 */
+	tf->device  = gtf->tf[5];	/* 0x1f6 */
+	tf->command = gtf->tf[6];	/* 0x1f7 */
+}
+
+static int ata_acpi_filter_tf(const struct ata_taskfile *tf,
+			      const struct ata_taskfile *ptf)
+{
+	if (ata_acpi_gtf_filter & ATA_ACPI_FILTER_LOCK) {
+		/* BIOS writers, sorry but we don't wanna lock
+		 * features unless the user explicitly said so.
+		 */
+
+		/* DEVICE CONFIGURATION FREEZE LOCK */
+		if (tf->command == ATA_CMD_CONF_OVERLAY &&
+		    tf->feature == ATA_DCO_FREEZE_LOCK)
+			return 1;
+
+		/* SECURITY FREEZE LOCK */
+		if (tf->command == ATA_CMD_SEC_FREEZE_LOCK)
+			return 1;
+
+		/* SET MAX LOCK and SET MAX FREEZE LOCK */
+		if ((!ptf || ptf->command != ATA_CMD_READ_NATIVE_MAX) &&
+		    tf->command == ATA_CMD_SET_MAX &&
+		    (tf->feature == ATA_SET_MAX_LOCK ||
+		     tf->feature == ATA_SET_MAX_FREEZE_LOCK))
+			return 1;
+	}
+
+	return 0;
+}
+
 /**
  * ata_acpi_run_tf - send taskfile registers to host controller
  * @dev: target ATA device
@@ -448,13 +504,15 @@ EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
  * EH context.
  *
  * RETURNS:
- * 1 if command is executed successfully.  0 if ignored or rejected,
- * -errno on other errors.
+ * 1 if command is executed successfully.  0 if ignored, rejected or
+ * filtered out, -errno on other errors.
  */
 static int ata_acpi_run_tf(struct ata_device *dev,
-			   const struct ata_acpi_gtf *gtf)
+			   const struct ata_acpi_gtf *gtf,
+			   const struct ata_acpi_gtf *prev_gtf)
 {
-	struct ata_taskfile tf, rtf;
+	struct ata_taskfile *pptf = NULL;
+	struct ata_taskfile tf, ptf, rtf;
 	unsigned int err_mask;
 	const char *level;
 	char msg[60];
@@ -465,44 +523,44 @@ static int ata_acpi_run_tf(struct ata_device *dev,
 	    && (gtf->tf[6] == 0))
 		return 0;
 
-	ata_tf_init(dev, &tf);
-
-	/* convert gtf to tf */
-	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; /* TBD */
-	tf.protocol = ATA_PROT_NODATA;
-	tf.feature = gtf->tf[0];	/* 0x1f1 */
-	tf.nsect   = gtf->tf[1];	/* 0x1f2 */
-	tf.lbal    = gtf->tf[2];	/* 0x1f3 */
-	tf.lbam    = gtf->tf[3];	/* 0x1f4 */
-	tf.lbah    = gtf->tf[4];	/* 0x1f5 */
-	tf.device  = gtf->tf[5];	/* 0x1f6 */
-	tf.command = gtf->tf[6];	/* 0x1f7 */
-
-	rtf = tf;
-	err_mask = ata_exec_internal(dev, &rtf, NULL, DMA_NONE, NULL, 0, 0);
-
-	switch (err_mask) {
-	case 0:
-		level = KERN_DEBUG;
-		snprintf(msg, sizeof(msg), "succeeded");
-		rc = 1;
-		break;
-
-	case AC_ERR_DEV:
+	ata_acpi_gtf_to_tf(dev, gtf, &tf);
+	if (prev_gtf) {
+		ata_acpi_gtf_to_tf(dev, prev_gtf, &ptf);
+		pptf = &ptf;
+	}
+
+	if (!ata_acpi_filter_tf(&tf, pptf)) {
+		rtf = tf;
+		err_mask = ata_exec_internal(dev, &rtf, NULL,
+					     DMA_NONE, NULL, 0, 0);
+
+		switch (err_mask) {
+		case 0:
+			level = KERN_DEBUG;
+			snprintf(msg, sizeof(msg), "succeeded");
+			rc = 1;
+			break;
+
+		case AC_ERR_DEV:
+			level = KERN_INFO;
+			snprintf(msg, sizeof(msg),
+				 "rejected by device (Stat=0x%02x Err=0x%02x)",
+				 rtf.command, rtf.feature);
+			rc = 0;
+			break;
+
+		default:
+			level = KERN_ERR;
+			snprintf(msg, sizeof(msg),
+				 "failed (Emask=0x%x Stat=0x%02x Err=0x%02x)",
+				 err_mask, rtf.command, rtf.feature);
+			rc = -EIO;
+			break;
+		}
+	} else {
 		level = KERN_INFO;
-		snprintf(msg, sizeof(msg),
-			 "rejected by device (Stat=0x%02x Err=0x%02x)",
-			 rtf.command, rtf.feature);
+		snprintf(msg, sizeof(msg), "filtered out");
 		rc = 0;
-		break;
-
-	default:
-		level = KERN_ERR;
-		snprintf(msg, sizeof(msg),
-			 "failed (Emask=0x%x Stat=0x%02x Err=0x%02x)",
-			 err_mask, rtf.command, rtf.feature);
-		rc = -EIO;
-		break;
 	}
 
 	ata_dev_printk(dev, level,
@@ -529,7 +587,7 @@ static int ata_acpi_run_tf(struct ata_device *dev,
  */
 static int ata_acpi_exec_tfs(struct ata_device *dev, int *nr_executed)
 {
-	struct ata_acpi_gtf *gtf = NULL;
+	struct ata_acpi_gtf *gtf = NULL, *pgtf = NULL;
 	void *ptr_to_free = NULL;
 	int gtf_count, i, rc;
 
@@ -540,12 +598,14 @@ static int ata_acpi_exec_tfs(struct ata_device *dev, int *nr_executed)
 	gtf_count = rc;
 
 	/* execute them */
-	for (i = 0; i < gtf_count; i++) {
-		rc = ata_acpi_run_tf(dev, gtf++);
+	for (i = 0; i < gtf_count; i++, gtf++) {
+		rc = ata_acpi_run_tf(dev, gtf, pgtf);
 		if (rc < 0)
 			break;
-		if (rc)
+		if (rc) {
 			(*nr_executed)++;
+			pgtf = gtf;
+		}
 	}
 
 	kfree(ptr_to_free);
-- 
1.5.2.4

Re: [PATCHSET #upstram-fixes] libata-acpi: improve ACPI corner case handling

[Tejun Heo]

Tejun Heo wrote:
> Hello,
> 
> This patchset contains the following patches to improve ACPI corner
> case handling.

I'm working on an improved version.  Please hold a bit.  Thanks.

-- 
tejun