Skip to contentSkip to conversation

libata-acpi: improve ACPI corner case handling

PATCH In review Tejun Heo 2 participants 16 messages Dec 14, 2007 libata

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata-acpi: improve ACPI corner case handling

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

author
#3 lore raw

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

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

Signed-off-by Tejun Heo <[email protected]>
---
 include/linux/ata.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
include/linux/ata.h +15 -0
index 5c4e54a..72ab808 100644
@@ -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:
Signature · 1 line
1.5.2.4
author
#4 lore raw

PATCH 4/9 libata: implement ata_acpi_init_gtm()

In reply to 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 <[email protected]>
Signed-off-by Jeff Garzik <[email protected]>
---
 drivers/ata/libata-acpi.c |    3 +++
 include/linux/libata.h    |   12 ++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)
drivers/ata/libata-acpi.c +3 -0
index e55cb4b..e4ec391 100644
@@ -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,
include/linux/libata.h +12 -0
index d3a0caf..bc8ef5b 100644
@@ -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
Signature · 1 line
1.5.2.4
author
#1 lore raw

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

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-acpi.c |    4 ++--
 include/linux/libata.h    |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
drivers/ata/libata-acpi.c +2 -2
index 545ea86..e55cb4b 100644
@@ -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;
include/linux/libata.h +2 -2
index ef52a07..6940344 100644
@@ -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
Signature · 1 line
1.5.2.4
author
#2 lore raw

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

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>
---
 include/linux/libata.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
include/linux/libata.h +4 -4
index 6940344..d3a0caf 100644
@@ -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)
 
 /*
Signature · 1 line
1.5.2.4
author
#6 lore raw

PATCH 6/9 libata-acpi: improve ACPI disabling

In reply to 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 <[email protected]>
---
 drivers/ata/libata-acpi.c |   60 +++++++++++++++++++++++++++++---------------
 1 files changed, 39 insertions(+), 21 deletions(-)
drivers/ata/libata-acpi.c +39 -21
index 5785cac..a118c3a 100644
@@ -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;
 }
Signature · 1 line
1.5.2.4
author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/ata/libata-acpi.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
drivers/ata/libata-acpi.c +7 -7
index e4ec391..5785cac 100644
@@ -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 */
Signature · 1 line
1.5.2.4
author
#7 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/ata/libata-acpi.c |   77 +++++++++++++++++++++++++++------------------
 1 files changed, 46 insertions(+), 31 deletions(-)
drivers/ata/libata-acpi.c +46 -31
index a118c3a..e68e918 100644
@@ -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;
 }
 
 /**
Signature · 1 line
1.5.2.4
author
#13 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/ata/libata-acpi.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
drivers/ata/libata-acpi.c +6 -3
index e68e918..1981112 100644
@@ -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;
 }
 
 /**
Signature · 1 line
1.5.2.4
author
#14 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/ata/libata-acpi.c |  148 +++++++++++++++++++++++++++++++-------------
 1 files changed, 104 insertions(+), 44 deletions(-)
drivers/ata/libata-acpi.c +104 -44
index 1981112..b487268 100644
@@ -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);
Signature · 1 line
1.5.2.4
author
#15 lore raw
In reply to Tejun Heo
Quoted 4 lines · Tejun Heo

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.

Signature · 1 line
tejun
#8 lore raw

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

In reply to Tejun Heo
Quoted 3 lines · 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.

..

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).

????

author
#9 lore raw

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

Quoted 10 lines · Mark Lord
Quoted 3 lines · 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.

..

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. :-)

Signature · 1 line
tejun
#10 lore raw

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

Quoted 5 lines · Tejun Heo
Quoted 4 lines · Mark Lord
Quoted 3 lines · 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.

Quoted 8 lines
Quoted 6 lines

..

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) ?

??

author
#11 lore raw

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

Quoted 13 lines · Mark Lord
Quoted 12 lines · Tejun Heo
Quoted 10 lines · Mark Lord
Quoted 3 lines · 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.

..

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).

Quoted 4 lines
Quoted 1 line

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.

Signature · 1 line
tejun
#12 lore raw

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

Quoted 16 lines · Tejun Heo
Quoted 15 lines · Mark Lord
Quoted 12 lines · Tejun Heo
Quoted 10 lines · Mark Lord
Quoted 3 lines · 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.

..

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. :-)

..

Quoted 8 lines
Quoted 1 line

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!