[PATCHSET] libata: improve ATA ACPI support

[PATCHSET] libata: improve ATA ACPI support

[Tejun Heo]

Hello, all.

This patchset improves ATA ACPI support as proposed in the following
message.

  http://article.gmane.org/gmane.linux.ide/17554

Improvements are...

* safe and simpler ACPI node association

* major code cleanup

* invoke ACPI methods only when ACPI spec says necessary

* proper ACPI error handling with a retry

* after successfully executing _GTF taskfiles, IDENTIFY page is
  reloaded

* _GTM/_STM support

This patchset is composed of 13 patches.

#01-02 : misc preparation
#03-04 : separate out ata_dev_reread_id() and make revalidation robust
	 against size change during device configuration
#05-06 : clean up libata-acpi.c
#07-08 : implement ata_acpi_associate()
#09-10 : more cleanups
#11-13 : reimplement ACPI invocation so that methods are called where
	 the spec specifies and errors are properly handled, and add
	 _GTM/_STM support.

Tested on three desktop boards (nf ultra, ich7 and ich8) and a
notebook.  This patchset is against...

  upstream (5365067b4bb17d1801fefe995d1342108b324471)
  + [1] pata_amd-remove-contamination
  + [2] libata-add-missing-call-to-cable_detect-in-new-EH
  + [3] libata-acpi-fix-GTF-command-protocol-for-ATAPI

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/18120
[2] http://article.gmane.org/gmane.linux.ide/18121
[3] http://article.gmane.org/gmane.linux.ide/18122

[PATCH 01/13] ahci: consolidate common port flags

[Tejun Heo]

Consolidate common port flags into AHCI_FLAG_COMMON.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/ahci.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 7c61bc7..34c5534 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -170,6 +170,10 @@ enum {
 	AHCI_FLAG_IGN_IRQ_IF_ERR	= (1 << 25), /* ignore IRQ_IF_ERR */
 	AHCI_FLAG_HONOR_PI		= (1 << 26), /* honor PORTS_IMPL */
 	AHCI_FLAG_IGN_SERR_INTERNAL	= (1 << 27), /* ignore SERR_INTERNAL */
+
+	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+					  ATA_FLAG_SKIP_D2H_BSY,
 };
 
 struct ahci_cmd_hdr {
@@ -323,54 +327,41 @@ static const struct ata_port_operations ahci_vt8251_ops = {
 static const struct ata_port_info ahci_port_info[] = {
 	/* board_ahci */
 	{
-		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_SKIP_D2H_BSY,
+		.flags		= AHCI_FLAG_COMMON,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
 	/* board_ahci_pi */
 	{
-		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_SKIP_D2H_BSY | AHCI_FLAG_HONOR_PI,
+		.flags		= AHCI_FLAG_COMMON | AHCI_FLAG_HONOR_PI,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
 	/* board_ahci_vt8251 */
 	{
-		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_SKIP_D2H_BSY |
-				  ATA_FLAG_HRST_TO_RESUME | AHCI_FLAG_NO_NCQ,
+		.flags		= AHCI_FLAG_COMMON | ATA_FLAG_HRST_TO_RESUME |
+				  AHCI_FLAG_NO_NCQ,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_vt8251_ops,
 	},
 	/* board_ahci_ign_iferr */
 	{
-		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_SKIP_D2H_BSY |
-				  AHCI_FLAG_IGN_IRQ_IF_ERR,
+		.flags		= AHCI_FLAG_COMMON | AHCI_FLAG_IGN_IRQ_IF_ERR,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
 	/* board_ahci_sb600 */
 	{
-		.sht		= &ahci_sht,
-		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_SKIP_D2H_BSY |
+		.flags		= AHCI_FLAG_COMMON |
 				  AHCI_FLAG_IGN_SERR_INTERNAL,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
-
 };
 
 static const struct pci_device_id ahci_pci_tbl[] = {
-- 
1.5.0.3

Re: [PATCH 01/13] ahci: consolidate common port flags

[Alan Cox]

On Mon, 23 Apr 2007 02:41:05 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Consolidate common port flags into AHCI_FLAG_COMMON.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
>

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH 01/13] ahci: consolidate common port flags

[Jeff Garzik]

Tejun Heo wrote:
> Consolidate common port flags into AHCI_FLAG_COMMON.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/ahci.c |   29 ++++++++++-------------------
>  1 files changed, 10 insertions(+), 19 deletions(-)

applied patches 1-2

[PATCH 02/13] libata: separate ATA_EHI_DID_RESET into DID_SOFTRESET and DID_HARDRESET

[Tejun Heo]

Separate ATA_EHI_DID_RESET into ATA_EHI_DID_SOFTRESET and
ATA_EHI_DID_HARDRESET.  ATA_EHI_DID_RESET is redefined as OR of the
two flags.  This patch doesn't introduce any behavior change.  This
will be used later to determine whether _SDD is necessary or not.

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

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 67bf150..2bff9ad 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1671,7 +1671,10 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 				reset == softreset ? "soft" : "hard");
 
 	/* mark that this EH session started with reset */
-	ehc->i.flags |= ATA_EHI_DID_RESET;
+	if (reset == hardreset)
+		ehc->i.flags |= ATA_EHI_DID_HARDRESET;
+	else
+		ehc->i.flags |= ATA_EHI_DID_SOFTRESET;
 
 	rc = ata_do_reset(ap, reset, classes);
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 73b86dd..d8cfc72 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -282,11 +282,13 @@ enum {
 	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
 	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
 
-	ATA_EHI_DID_RESET	= (1 << 16), /* already reset this port */
-	ATA_EHI_PRINTINFO	= (1 << 17), /* print configuration info */
-	ATA_EHI_SETMODE		= (1 << 18), /* configure transfer mode */
-	ATA_EHI_POST_SETMODE	= (1 << 19), /* revaildating after setmode */
+	ATA_EHI_DID_SOFTRESET	= (1 << 16), /* already soft-reset this port */
+	ATA_EHI_DID_HARDRESET	= (1 << 17), /* already soft-reset this port */
+	ATA_EHI_PRINTINFO	= (1 << 18), /* print configuration info */
+	ATA_EHI_SETMODE		= (1 << 19), /* configure transfer mode */
+	ATA_EHI_POST_SETMODE	= (1 << 20), /* revaildating after setmode */
 
+	ATA_EHI_DID_RESET	= ATA_EHI_DID_SOFTRESET | ATA_EHI_DID_HARDRESET,
 	ATA_EHI_RESET_MODIFIER_MASK = ATA_EHI_RESUME_LINK,
 
 	/* max repeat if error condition is still set after ->error_handler */
-- 
1.5.0.3

Re: [PATCH 02/13] libata: separate ATA_EHI_DID_RESET into DID_SOFTRESET and DID_HARDRESET

[Alan Cox]

On Mon, 23 Apr 2007 02:41:05 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Separate ATA_EHI_DID_RESET into ATA_EHI_DID_SOFTRESET and
> ATA_EHI_DID_HARDRESET.  ATA_EHI_DID_RESET is redefined as OR of the
> two flags.  This patch doesn't introduce any behavior change.  This
> will be used later to determine whether _SDD is necessary or not.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Acked-by: Alan Cox <alan@redhat.com>

>

[PATCH 03/13] libata: separate out ata_dev_reread_id()

[Tejun Heo]

Separate out ata_dev_reread_id() from ata_dev_revalidate().
ata_dev_reread_id() reads IDENTIFY page and determines whether the
same device is still there.  ata_dev_revalidate() reconfigures after
reread completes.  This will be used by ACPI update.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   47 ++++++++++++++++++++++++++++++++------------
 drivers/ata/libata.h      |    3 +-
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6d0a946..7a6e8fd 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3562,8 +3562,8 @@ static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
 }
 
 /**
- *	ata_dev_revalidate - Revalidate ATA device
- *	@dev: device to revalidate
+ *	ata_dev_reread_id - Re-read IDENTIFY data
+ *	@adev: target ATA device
  *	@readid_flags: read ID flags
  *
  *	Re-read IDENTIFY page and make sure @dev is still attached to
@@ -3575,29 +3575,50 @@ static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
  *	RETURNS:
  *	0 on success, negative errno otherwise
  */
-int ata_dev_revalidate(struct ata_device *dev, unsigned int readid_flags)
+int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags)
 {
 	unsigned int class = dev->class;
 	u16 *id = (void *)dev->ap->sector_buf;
 	int rc;
 
-	if (!ata_dev_enabled(dev)) {
-		rc = -ENODEV;
-		goto fail;
-	}
-
 	/* read ID data */
 	rc = ata_dev_read_id(dev, &class, readid_flags, id);
 	if (rc)
-		goto fail;
+		return rc;
 
 	/* is the device still there? */
-	if (!ata_dev_same_device(dev, class, id)) {
-		rc = -ENODEV;
-		goto fail;
-	}
+	if (!ata_dev_same_device(dev, class, id))
+		return -ENODEV;
 
 	memcpy(dev->id, id, sizeof(id[0]) * ATA_ID_WORDS);
+	return 0;
+}
+
+/**
+ *	ata_dev_revalidate - Revalidate ATA device
+ *	@dev: device to revalidate
+ *	@readid_flags: read ID flags
+ *
+ *	Re-read IDENTIFY page, make sure @dev is still attached to the
+ *	port and reconfigure it according to the new IDENTIFY page.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, negative errno otherwise
+ */
+int ata_dev_revalidate(struct ata_device *dev, unsigned int readid_flags)
+{
+	int rc;
+
+	if (!ata_dev_enabled(dev))
+		return -ENODEV;
+
+	/* re-read ID */
+	rc = ata_dev_reread_id(dev, readid_flags);
+	if (rc)
+		goto fail;
 
 	/* configure device according to the new ID */
 	rc = ata_dev_configure(dev);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5f4d40c..c40665a 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -75,7 +75,8 @@ extern unsigned ata_exec_internal_sg(struct ata_device *dev,
 extern unsigned int ata_do_simple_cmd(struct ata_device *dev, u8 cmd);
 extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 			   unsigned int flags, u16 *id);
-extern int ata_dev_revalidate(struct ata_device *dev, unsigned int flags);
+extern int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags);
+extern int ata_dev_revalidate(struct ata_device *dev, unsigned int readid_flags);
 extern int ata_dev_configure(struct ata_device *dev);
 extern int sata_down_spd_limit(struct ata_port *ap);
 extern int sata_set_spd_needed(struct ata_port *ap);
-- 
1.5.0.3

Re: [PATCH 03/13] libata: separate out ata_dev_reread_id()

[Jeff Garzik]

Tejun Heo wrote:
> Separate out ata_dev_reread_id() from ata_dev_revalidate().
> ata_dev_reread_id() reads IDENTIFY page and determines whether the
> same device is still there.  ata_dev_revalidate() reconfigures after
> reread completes.  This will be used by ACPI update.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-core.c |   47 ++++++++++++++++++++++++++++++++------------
>  drivers/ata/libata.h      |    3 +-
>  2 files changed, 36 insertions(+), 14 deletions(-)

ACK, though I think you'll have to rediff due to a Mark Lord patch

Re: [PATCH 03/13] libata: separate out ata_dev_reread_id()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Separate out ata_dev_reread_id() from ata_dev_revalidate().
>> ata_dev_reread_id() reads IDENTIFY page and determines whether the
>> same device is still there.  ata_dev_revalidate() reconfigures after
>> reread completes.  This will be used by ACPI update.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>>  drivers/ata/libata-core.c |   47
>> ++++++++++++++++++++++++++++++++------------
>>  drivers/ata/libata.h      |    3 +-
>>  2 files changed, 36 insertions(+), 14 deletions(-)
> 
> ACK, though I think you'll have to rediff due to a Mark Lord patch

No problem.

-- 
tejun

[PATCH 06/13] libata-acpi: clean up parameters and misc stuff

[Tejun Heo]

This patch cleans up libata-acpi such that it looks similar to other
libata files.  This patch doesn't introuce any behavior changes.

* make libata-acpi functions take ata_device instead of ata_port +
  device index
* s/atadev/adev/
* de-indent local variable declarations

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index cb3eab6..f005730 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -270,8 +270,7 @@ out:
 
 /**
  * do_drive_get_GTF - get the drive bootup default taskfile settings
- * @ap: the ata_port for the drive
- * @ix: target ata_device (drive) index
+ * @adev: target ATA device
  * @gtf_length: number of bytes of _GTF data returned at @gtf_address
  * @gtf_address: buffer containing _GTF taskfile arrays
  *
@@ -286,20 +285,19 @@ out:
  * The returned @gtf_length and @gtf_address are only valid if the
  * function return value is 0.
  */
-static int do_drive_get_GTF(struct ata_port *ap, int ix,
-			unsigned int *gtf_length, unsigned long *gtf_address,
-			unsigned long *obj_loc)
+static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
+			    unsigned long *gtf_address, unsigned long *obj_loc)
 {
-	acpi_status			status;
-	acpi_handle			dev_handle = NULL;
-	acpi_handle			chan_handle, drive_handle;
-	acpi_integer			pcidevfn = 0;
-	u32				dev_adr;
-	struct acpi_buffer		output;
-	union acpi_object 		*out_obj;
-	struct device			*dev = ap->host->dev;
-	struct ata_device		*atadev = &ap->device[ix];
-	int				err = -ENODEV;
+	struct ata_port *ap = adev->ap;
+	acpi_status status;
+	acpi_handle dev_handle = NULL;
+	acpi_handle chan_handle, drive_handle;
+	acpi_integer pcidevfn = 0;
+	u32 dev_adr;
+	struct acpi_buffer output;
+	union acpi_object *out_obj;
+	struct device *dev = ap->host->dev;
+	int err = -ENODEV;
 
 	*gtf_length = 0;
 	*gtf_address = 0UL;
@@ -309,14 +307,14 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 		return 0;
 
 	if (ata_msg_probe(ap))
-		ata_dev_printk(atadev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
+		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
 			       __FUNCTION__, ap->port_no);
 
-	if (!ata_dev_enabled(atadev) || (ap->flags & ATA_FLAG_DISABLED)) {
+	if (!ata_dev_enabled(adev) || (ap->flags & ATA_FLAG_DISABLED)) {
 		if (ata_msg_probe(ap))
-			ata_dev_printk(atadev, KERN_DEBUG, "%s: ERR: "
+			ata_dev_printk(adev, KERN_DEBUG, "%s: ERR: "
 				"ata_dev_present: %d, PORT_DISABLED: %lu\n",
-				__FUNCTION__, ata_dev_enabled(atadev),
+				__FUNCTION__, ata_dev_enabled(adev),
 				ap->flags & ATA_FLAG_DISABLED);
 		goto out;
 	}
@@ -327,7 +325,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 		err = pata_get_dev_handle(dev, &dev_handle, &pcidevfn);
 		if (err < 0) {
 			if (ata_msg_probe(ap))
-				ata_dev_printk(atadev, KERN_DEBUG,
+				ata_dev_printk(adev, KERN_DEBUG,
 					"%s: pata_get_dev_handle failed (%d)\n",
 					__FUNCTION__, err);
 			goto out;
@@ -336,7 +334,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 		err = sata_get_dev_handle(dev, &dev_handle, &pcidevfn);
 		if (err < 0) {
 			if (ata_msg_probe(ap))
-				ata_dev_printk(atadev, KERN_DEBUG,
+				ata_dev_printk(adev, KERN_DEBUG,
 					"%s: sata_get_dev_handle failed (%d\n",
 					__FUNCTION__, err);
 			goto out;
@@ -344,7 +342,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 	}
 
 	/* Get this drive's _ADR info. if not already known. */
-	if (!atadev->obj_handle) {
+	if (!adev->obj_handle) {
 		if (!(ap->cbl == ATA_CBL_SATA)) {
 			/* get child objects of dev_handle == channel objects,
 	 		 * + _their_ children == drive objects */
@@ -352,7 +350,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 			chan_handle = acpi_get_child(dev_handle,
 						ap->port_no);
 			if (ata_msg_probe(ap))
-				ata_dev_printk(atadev, KERN_DEBUG,
+				ata_dev_printk(adev, KERN_DEBUG,
 					"%s: chan adr=%d: chan_handle=0x%p\n",
 					__FUNCTION__, ap->port_no,
 					chan_handle);
@@ -361,26 +359,26 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 				goto out;
 			}
 			/* TBD: could also check ACPI object VALID bits */
-			drive_handle = acpi_get_child(chan_handle, ix);
+			drive_handle = acpi_get_child(chan_handle, adev->devno);
 			if (!drive_handle) {
 				err = -ENODEV;
 				goto out;
 			}
-			dev_adr = ix;
-			atadev->obj_handle = drive_handle;
+			dev_adr = adev->devno;
+			adev->obj_handle = drive_handle;
 		} else {	/* for SATA mode */
 			dev_adr = SATA_ADR_RSVD;
 			err = get_sata_adr(dev, dev_handle, pcidevfn, 0,
-					ap, atadev, &dev_adr);
+					ap, adev, &dev_adr);
 		}
 		if (err < 0 || dev_adr == SATA_ADR_RSVD ||
-		    !atadev->obj_handle) {
+		    !adev->obj_handle) {
 			if (ata_msg_probe(ap))
-				ata_dev_printk(atadev, KERN_DEBUG,
+				ata_dev_printk(adev, KERN_DEBUG,
 					"%s: get_sata/pata_adr failed: "
 					"err=%d, dev_adr=%u, obj_handle=0x%p\n",
 					__FUNCTION__, err, dev_adr,
-					atadev->obj_handle);
+					adev->obj_handle);
 			goto out;
 		}
 	}
@@ -391,11 +389,11 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 
 	/* _GTF has no input parameters */
 	err = -EIO;
-	status = acpi_evaluate_object(atadev->obj_handle, "_GTF",
+	status = acpi_evaluate_object(adev->obj_handle, "_GTF",
 					NULL, &output);
 	if (ACPI_FAILURE(status)) {
 		if (ata_msg_probe(ap))
-			ata_dev_printk(atadev, KERN_DEBUG,
+			ata_dev_printk(adev, KERN_DEBUG,
 				"%s: Run _GTF error: status = 0x%x\n",
 				__FUNCTION__, status);
 		goto out;
@@ -403,7 +401,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 
 	if (!output.length || !output.pointer) {
 		if (ata_msg_probe(ap))
-			ata_dev_printk(atadev, KERN_DEBUG, "%s: Run _GTF: "
+			ata_dev_printk(adev, KERN_DEBUG, "%s: Run _GTF: "
 				"length or ptr is NULL (0x%llx, 0x%p)\n",
 				__FUNCTION__,
 				(unsigned long long)output.length,
@@ -416,7 +414,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 	if (out_obj->type != ACPI_TYPE_BUFFER) {
 		kfree(output.pointer);
 		if (ata_msg_probe(ap))
-			ata_dev_printk(atadev, KERN_DEBUG, "%s: Run _GTF: "
+			ata_dev_printk(adev, KERN_DEBUG, "%s: Run _GTF: "
 				"error: expected object type of "
 				" ACPI_TYPE_BUFFER, got 0x%x\n",
 				__FUNCTION__, out_obj->type);
@@ -427,7 +425,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 	if (!out_obj->buffer.length || !out_obj->buffer.pointer ||
 	    out_obj->buffer.length % REGS_PER_GTF) {
 		if (ata_msg_drv(ap))
-			ata_dev_printk(atadev, KERN_ERR,
+			ata_dev_printk(adev, KERN_ERR,
 				"%s: unexpected GTF length (%d) or addr (0x%p)\n",
 				__FUNCTION__, out_obj->buffer.length,
 				out_obj->buffer.pointer);
@@ -439,7 +437,7 @@ static int do_drive_get_GTF(struct ata_port *ap, int ix,
 	*gtf_address = (unsigned long)out_obj->buffer.pointer;
 	*obj_loc = (unsigned long)out_obj;
 	if (ata_msg_probe(ap))
-		ata_dev_printk(atadev, KERN_DEBUG, "%s: returning "
+		ata_dev_printk(adev, KERN_DEBUG, "%s: returning "
 			"gtf_length=%d, gtf_address=0x%lx, obj_loc=0x%lx\n",
 			__FUNCTION__, *gtf_length, *gtf_address, *obj_loc);
 	err = 0;
@@ -449,7 +447,7 @@ out:
 
 /**
  * taskfile_load_raw - send taskfile registers to host controller
- * @ap: Port to which output is sent
+ * @adev: target ATA device
  * @gtf: raw ATA taskfile register set (0x1f1 - 0x1f7)
  *
  * Outputs ATA taskfile to standard ATA host controller using MMIO
@@ -466,15 +464,15 @@ out:
  * LOCKING: TBD:
  * Inherited from caller.
  */
-static void taskfile_load_raw(struct ata_port *ap,
-				struct ata_device *atadev,
-				const struct taskfile_array *gtf)
+static void taskfile_load_raw(struct ata_device *adev,
+			      const struct taskfile_array *gtf)
 {
+	struct ata_port *ap = adev->ap;
 	struct ata_taskfile tf;
 	unsigned int err;
 
 	if (ata_msg_probe(ap))
-		ata_dev_printk(atadev, KERN_DEBUG, "%s: (0x1f1-1f7): hex: "
+		ata_dev_printk(adev, KERN_DEBUG, "%s: (0x1f1-1f7): hex: "
 			"%02x %02x %02x %02x %02x %02x %02x\n",
 			__FUNCTION__,
 			gtf->tfa[0], gtf->tfa[1], gtf->tfa[2],
@@ -485,7 +483,7 @@ static void taskfile_load_raw(struct ata_port *ap,
 	    && (gtf->tfa[6] == 0))
 		return;
 
-	ata_tf_init(atadev, &tf);
+	ata_tf_init(adev, &tf);
 
 	/* convert gtf to tf */
 	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; /* TBD */
@@ -498,17 +496,16 @@ static void taskfile_load_raw(struct ata_port *ap,
 	tf.device  = gtf->tfa[5];	/* 0x1f6 */
 	tf.command = gtf->tfa[6];	/* 0x1f7 */
 
-	err = ata_exec_internal(atadev, &tf, NULL, DMA_NONE, NULL, 0);
+	err = ata_exec_internal(adev, &tf, NULL, DMA_NONE, NULL, 0);
 	if (err && ata_msg_probe(ap))
-		ata_dev_printk(atadev, KERN_ERR,
+		ata_dev_printk(adev, KERN_ERR,
 			"%s: ata_exec_internal failed: %u\n",
 			__FUNCTION__, err);
 }
 
 /**
  * do_drive_set_taskfiles - write the drive taskfile settings from _GTF
- * @ap: the ata_port for the drive
- * @atadev: target ata_device
+ * @adev: target ATA device
  * @gtf_length: total number of bytes of _GTF taskfiles
  * @gtf_address: location of _GTF taskfile arrays
  *
@@ -517,30 +514,31 @@ static void taskfile_load_raw(struct ata_port *ap,
  * Write {gtf_address, length gtf_length} in groups of
  * REGS_PER_GTF bytes.
  */
-static int do_drive_set_taskfiles(struct ata_port *ap,
-		struct ata_device *atadev, unsigned int gtf_length,
-		unsigned long gtf_address)
+static int do_drive_set_taskfiles(struct ata_device *adev,
+				  unsigned int gtf_length,
+				  unsigned long gtf_address)
 {
-	int			err = -ENODEV;
-	int			gtf_count = gtf_length / REGS_PER_GTF;
-	int			ix;
+	struct ata_port *ap = adev->ap;
+	int err = -ENODEV;
+	int gtf_count = gtf_length / REGS_PER_GTF;
+	int ix;
 	struct taskfile_array	*gtf;
 
 	if (ata_msg_probe(ap))
-		ata_dev_printk(atadev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
+		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
 			       __FUNCTION__, ap->port_no);
 
 	if (libata_noacpi || !(ap->cbl == ATA_CBL_SATA))
 		return 0;
 
-	if (!ata_dev_enabled(atadev) || (ap->flags & ATA_FLAG_DISABLED))
+	if (!ata_dev_enabled(adev) || (ap->flags & ATA_FLAG_DISABLED))
 		goto out;
 	if (!gtf_count)		/* shouldn't be here */
 		goto out;
 
 	if (gtf_length % REGS_PER_GTF) {
 		if (ata_msg_drv(ap))
-			ata_dev_printk(atadev, KERN_ERR,
+			ata_dev_printk(adev, KERN_ERR,
 				"%s: unexpected GTF length (%d)\n",
 				__FUNCTION__, gtf_length);
 		goto out;
@@ -551,7 +549,7 @@ static int do_drive_set_taskfiles(struct ata_port *ap,
 			(gtf_address + ix * REGS_PER_GTF);
 
 		/* send all TaskFile registers (0x1f1-0x1f7) *in*that*order* */
-		taskfile_load_raw(ap, atadev, gtf);
+		taskfile_load_raw(adev, gtf);
 	}
 
 	err = 0;
@@ -567,11 +565,11 @@ out:
  */
 int ata_acpi_exec_tfs(struct ata_port *ap)
 {
-	int		ix;
-	int		ret =0;
-	unsigned int	gtf_length;
-	unsigned long	gtf_address;
-	unsigned long	obj_loc;
+	int ix;
+	int ret = 0;
+	unsigned int gtf_length;
+	unsigned long gtf_address;
+	unsigned long obj_loc;
 
 	if (libata_noacpi)
 		return 0;
@@ -584,11 +582,13 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 		return 0;
 
 	for (ix = 0; ix < ATA_MAX_DEVICES; ix++) {
-		if (!ata_dev_enabled(&ap->device[ix]))
+		struct ata_device *adev = &ap->device[ix];
+
+		if (!ata_dev_enabled(adev))
 			continue;
 
-		ret = do_drive_get_GTF(ap, ix,
-				&gtf_length, &gtf_address, &obj_loc);
+		ret = do_drive_get_GTF(adev, &gtf_length, &gtf_address,
+				       &obj_loc);
 		if (ret < 0) {
 			if (ata_msg_probe(ap))
 				ata_port_printk(ap, KERN_DEBUG,
@@ -597,8 +597,7 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 			break;
 		}
 
-		ret = do_drive_set_taskfiles(ap, &ap->device[ix],
-				gtf_length, gtf_address);
+		ret = do_drive_set_taskfiles(adev, gtf_length, gtf_address);
 		kfree((void *)obj_loc);
 		if (ret < 0) {
 			if (ata_msg_probe(ap))
@@ -614,8 +613,7 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 
 /**
  * ata_acpi_push_id - send Identify data to drive
- * @ap: the ata_port for the drive
- * @ix: drive index
+ * @adev: target ATA device
  *
  * _SDD ACPI object: for SATA mode only
  * Must be after Identify (Packet) Device -- uses its data
@@ -623,29 +621,29 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
  * method and if it fails for whatever reason, we should still
  * just keep going.
  */
-int ata_acpi_push_id(struct ata_port *ap, unsigned int ix)
+int ata_acpi_push_id(struct ata_device *adev)
 {
-	acpi_handle                     handle;
-	acpi_integer                    pcidevfn;
-	int                             err;
-	struct device                   *dev = ap->host->dev;
-	struct ata_device               *atadev = &ap->device[ix];
-	u32                             dev_adr;
-	acpi_status                     status;
-	struct acpi_object_list         input;
-	union acpi_object               in_params[1];
+	struct ata_port *ap = adev->ap;
+	acpi_handle handle;
+	acpi_integer pcidevfn;
+	int err;
+	struct device *dev = ap->host->dev;
+	u32 dev_adr;
+	acpi_status status;
+	struct acpi_object_list input;
+	union acpi_object in_params[1];
 
 	if (libata_noacpi)
 		return 0;
 
 	if (ata_msg_probe(ap))
-		ata_dev_printk(atadev, KERN_DEBUG, "%s: ix = %d, port#: %d\n",
-			       __FUNCTION__, ix, ap->port_no);
+		ata_dev_printk(adev, KERN_DEBUG, "%s: ix = %d, port#: %d\n",
+			       __FUNCTION__, adev->devno, ap->port_no);
 
 	/* Don't continue if not a SATA device. */
 	if (!(ap->cbl == ATA_CBL_SATA)) {
 		if (ata_msg_probe(ap))
-			ata_dev_printk(atadev, KERN_DEBUG,
+			ata_dev_printk(adev, KERN_DEBUG,
 				"%s: Not a SATA device\n", __FUNCTION__);
 		goto out;
 	}
@@ -655,25 +653,25 @@ int ata_acpi_push_id(struct ata_port *ap, unsigned int ix)
 	err = sata_get_dev_handle(dev, &handle, &pcidevfn);
 	if (err < 0) {
 		if (ata_msg_probe(ap))
-			ata_dev_printk(atadev, KERN_DEBUG,
+			ata_dev_printk(adev, KERN_DEBUG,
 				"%s: sata_get_dev_handle failed (%d\n",
 				__FUNCTION__, err);
 		goto out;
 	}
 
 	/* Get this drive's _ADR info, if not already known */
-	if (!atadev->obj_handle) {
+	if (!adev->obj_handle) {
 		dev_adr = SATA_ADR_RSVD;
-		err = get_sata_adr(dev, handle, pcidevfn, ix, ap, atadev,
+		err = get_sata_adr(dev, handle, pcidevfn, adev->devno, ap, adev,
 					&dev_adr);
 		if (err < 0 || dev_adr == SATA_ADR_RSVD ||
-			!atadev->obj_handle) {
+			!adev->obj_handle) {
 			if (ata_msg_probe(ap))
-				ata_dev_printk(atadev, KERN_DEBUG,
+				ata_dev_printk(adev, KERN_DEBUG,
 					"%s: get_sata_adr failed: "
 					"err=%d, dev_adr=%u, obj_handle=0x%p\n",
 					__FUNCTION__, err, dev_adr,
-					atadev->obj_handle);
+					adev->obj_handle);
 			goto out;
 		}
 	}
@@ -683,19 +681,19 @@ int ata_acpi_push_id(struct ata_port *ap, unsigned int ix)
 	input.count = 1;
 	input.pointer = in_params;
 	in_params[0].type = ACPI_TYPE_BUFFER;
-	in_params[0].buffer.length = sizeof(atadev->id[0]) * ATA_ID_WORDS;
-	in_params[0].buffer.pointer = (u8 *)atadev->id;
+	in_params[0].buffer.length = sizeof(adev->id[0]) * ATA_ID_WORDS;
+	in_params[0].buffer.pointer = (u8 *)adev->id;
 	/* Output buffer: _SDD has no output */
 
 	/* It's OK for _SDD to be missing too. */
-	swap_buf_le16(atadev->id, ATA_ID_WORDS);
-	status = acpi_evaluate_object(atadev->obj_handle, "_SDD", &input, NULL);
-	swap_buf_le16(atadev->id, ATA_ID_WORDS);
+	swap_buf_le16(adev->id, ATA_ID_WORDS);
+	status = acpi_evaluate_object(adev->obj_handle, "_SDD", &input, NULL);
+	swap_buf_le16(adev->id, ATA_ID_WORDS);
 
 	err = ACPI_FAILURE(status) ? -EIO : 0;
 	if (err < 0) {
 		if (ata_msg_probe(ap))
-			ata_dev_printk(atadev, KERN_DEBUG,
+			ata_dev_printk(adev, KERN_DEBUG,
 				       "%s _SDD error: status = 0x%x\n",
 				       __FUNCTION__, status);
 	}
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2171af6..d3d0f01 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1812,7 +1812,7 @@ int ata_dev_configure(struct ata_device *dev)
 		ata_dev_printk(dev, KERN_DEBUG, "%s: ENTER\n", __FUNCTION__);
 
 	/* set _SDD */
-	rc = ata_acpi_push_id(ap, dev->devno);
+	rc = ata_acpi_push_id(dev);
 	if (rc) {
 		ata_dev_printk(dev, KERN_WARNING, "failed to set _SDD(%d)\n",
 			rc);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 122bc16..6041b91 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -99,13 +99,13 @@ extern struct ata_port *ata_port_alloc(struct ata_host *host);
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
 extern int ata_acpi_exec_tfs(struct ata_port *ap);
-extern int ata_acpi_push_id(struct ata_port *ap, unsigned int ix);
+extern int ata_acpi_push_id(struct ata_device *adev);
 #else
 static inline int ata_acpi_exec_tfs(struct ata_port *ap)
 {
 	return 0;
 }
-static inline int ata_acpi_push_id(struct ata_port *ap, unsigned int ix)
+static inline int ata_acpi_push_id(struct ata_device *adev)
 {
 	return 0;
 }
-- 
1.5.0.3

Re: [PATCH 06/13] libata-acpi: clean up parameters and misc stuff

[Jeff Garzik]

Tejun Heo wrote:
> This patch cleans up libata-acpi such that it looks similar to other
> libata files.  This patch doesn't introuce any behavior changes.
> 
> * make libata-acpi functions take ata_device instead of ata_port +
>   device index
> * s/atadev/adev/
> * de-indent local variable declarations

I prefer 'dev' over 'adev', unless that makes the code in question more 
ambiguous.

Otherwise, ACK

Re: [PATCH 06/13] libata-acpi: clean up parameters and misc stuff

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> This patch cleans up libata-acpi such that it looks similar to other
>> libata files.  This patch doesn't introuce any behavior changes.
>>
>> * make libata-acpi functions take ata_device instead of ata_port +
>>   device index
>> * s/atadev/adev/
>> * de-indent local variable declarations
> 
> I prefer 'dev' over 'adev', unless that makes the code in question more
> ambiguous.

Alan is preferring adev over dev and I thought that might be better in
the spirit of 'ap'.  I don't really care about the naming tho.  Will
convert to dev.  It won't increase ambiguity.

-- 
tejun

Re: [PATCH 06/13] libata-acpi: clean up parameters and misc stuff

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> This patch cleans up libata-acpi such that it looks similar to other
>>> libata files.  This patch doesn't introuce any behavior changes.
>>>
>>> * make libata-acpi functions take ata_device instead of ata_port +
>>>   device index
>>> * s/atadev/adev/
>>> * de-indent local variable declarations
>> I prefer 'dev' over 'adev', unless that makes the code in question more
>> ambiguous.
> 
> Alan is preferring adev over dev and I thought that might be better in
> the spirit of 'ap'.  I don't really care about the naming tho.  Will
> convert to dev.  It won't increase ambiguity.

Cool.  You will see 'dev' used universally in the code I wrote.  It also 
matches well with "ata_dev_" prefixes, which are a bit better than 
"ata_adev_" prefix if one were to apply the alternate policy.

Yes, I sometimes spend way too much time pay attention to trivialities :)

	Jeff

[PATCH 08/13] libata-acpi: implement ata_acpi_associate()

[Tejun Heo]

* Add acpi_handle to ata_host and ata_port.  Rename
  ata_device->obj_handle to ->acpi_handle and move it above such that
  it doesn't get cleared on reconfiguration.

* Replace ACPI node association which ata_acpi_associate() which is
  called once during host initialization.  Unlike the previous
  implementation, ata_acpi_associate() uses ATA_FLAG_ACPI_SATA to
  choose between IDE or SATA ACPI hierarchy and uses simple child look
  up instead of recursive walk to match the nodes.  This is way safer
  and simpler.  Please read the following message for more info.

  http://article.gmane.org/gmane.linux.ide/17554

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 24c2198..a2745b5 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -24,10 +24,8 @@
 #include <acpi/acmacros.h>
 #include <acpi/actypes.h>
 
-#define SATA_ROOT_PORT(x)	(((x) >> 16) & 0xffff)
-#define SATA_PORT_NUMBER(x)	((x) & 0xffff)	/* or NO_PORT_MULT */
 #define NO_PORT_MULT		0xffff
-#define SATA_ADR_RSVD		0xffffffff
+#define SATA_ADR(root,pmp)	(((root) << 16) | (pmp))
 
 #define REGS_PER_GTF		7
 struct taskfile_array {
@@ -42,230 +40,64 @@ static int is_pci_dev(struct device *dev)
 	return (dev->bus == &pci_bus_type);
 }
 
-/**
- * sata_get_dev_handle - finds acpi_handle and PCI device.function
- * @dev: device to locate
- * @handle: returned acpi_handle for @dev
- * @pcidevfn: return PCI device.func for @dev
- *
- * This function is somewhat SATA-specific.  Or at least the
- * PATA & SATA versions of this function are different,
- * so it's not entirely generic code.
- *
- * Returns 0 on success, <0 on error.
- */
-static int sata_get_dev_handle(struct device *dev, acpi_handle *handle,
-					acpi_integer *pcidevfn)
+static void ata_acpi_associate_sata_port(struct ata_port *ap)
 {
-	struct pci_dev	*pci_dev;
-	acpi_integer	addr;
-
-	if (!is_pci_dev(dev))
-		return -ENODEV;
-
-	pci_dev = to_pci_dev(dev);	/* NOTE: PCI-specific */
-	/* Please refer to the ACPI spec for the syntax of _ADR. */
-	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
-	*pcidevfn = addr;
-	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr);
-	if (!*handle)
-		return -ENODEV;
-	return 0;
+	acpi_integer adr = SATA_ADR(ap->port_no, NO_PORT_MULT);
+
+	ap->device->acpi_handle = acpi_get_child(ap->host->acpi_handle, adr);
 }
 
-/**
- * pata_get_dev_handle - finds acpi_handle and PCI device.function
- * @dev: device to locate
- * @handle: returned acpi_handle for @dev
- * @pcidevfn: return PCI device.func for @dev
- *
- * The PATA and SATA versions of this function are different.
- *
- * Returns 0 on success, <0 on error.
- */
-static int pata_get_dev_handle(struct device *dev, acpi_handle *handle,
-				acpi_integer *pcidevfn)
+static void ata_acpi_associate_ide_port(struct ata_port *ap)
 {
-	unsigned int bus, devnum, func;
-	acpi_integer addr;
-	acpi_handle dev_handle, parent_handle;
-	struct acpi_buffer buffer = {.length = ACPI_ALLOCATE_BUFFER,
-					.pointer = NULL};
-	acpi_status status;
-	struct acpi_device_info	*dinfo = NULL;
-	int ret = -ENODEV;
-	struct pci_dev *pdev;
-
-	if (!is_pci_dev(dev))
-		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
-
-	bus = pdev->bus->number;
-	devnum = PCI_SLOT(pdev->devfn);
-	func = PCI_FUNC(pdev->devfn);
-
-	dev_handle = DEVICE_ACPI_HANDLE(dev);
-	parent_handle = DEVICE_ACPI_HANDLE(dev->parent);
-
-	status = acpi_get_object_info(parent_handle, &buffer);
-	if (ACPI_FAILURE(status))
-		goto err;
-
-	dinfo = buffer.pointer;
-	if (dinfo && (dinfo->valid & ACPI_VALID_ADR) &&
-	    dinfo->address == bus) {
-		/* ACPI spec for _ADR for PCI bus: */
-		addr = (acpi_integer)(devnum << 16 | func);
-		*pcidevfn = addr;
-		*handle = dev_handle;
-	} else {
-		goto err;
-	}
+	int max_devices, i;
 
-	if (!*handle)
-		goto err;
-	ret = 0;
-err:
-	kfree(dinfo);
-	return ret;
-}
-
-struct walk_info {		/* can be trimmed some */
-	struct device	*dev;
-	struct acpi_device *adev;
-	acpi_handle	handle;
-	acpi_integer	pcidevfn;
-	unsigned int	drivenum;
-	acpi_handle	obj_handle;
-	struct ata_port *ataport;
-	struct ata_device *atadev;
-	u32		sata_adr;
-	int		status;
-	char		basepath[ACPI_PATHNAME_MAX];
-	int		basepath_len;
-};
+	ap->acpi_handle = acpi_get_child(ap->host->acpi_handle, ap->port_no);
+	if (!ap->acpi_handle)
+		return;
 
-static acpi_status get_devices(acpi_handle handle,
-				u32 level, void *context, void **return_value)
-{
-	acpi_status		status;
-	struct walk_info	*winfo = context;
-	struct acpi_buffer	namebuf = {ACPI_ALLOCATE_BUFFER, NULL};
-	char			*pathname;
-	struct acpi_buffer	buffer;
-	struct acpi_device_info	*dinfo;
-
-	status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &namebuf);
-	if (status)
-		goto ret;
-	pathname = namebuf.pointer;
-
-	buffer.length = ACPI_ALLOCATE_BUFFER;
-	buffer.pointer = NULL;
-	status = acpi_get_object_info(handle, &buffer);
-	if (ACPI_FAILURE(status))
-		goto out2;
-
-	dinfo = buffer.pointer;
-
-	/* find full device path name for pcidevfn */
-	if (dinfo && (dinfo->valid & ACPI_VALID_ADR) &&
-	    dinfo->address == winfo->pcidevfn) {
-		if (ata_msg_probe(winfo->ataport))
-			ata_dev_printk(winfo->atadev, KERN_DEBUG,
-				":%s: matches pcidevfn (0x%llx)\n",
-				pathname, winfo->pcidevfn);
-		strlcpy(winfo->basepath, pathname,
-			sizeof(winfo->basepath));
-		winfo->basepath_len = strlen(pathname);
-		goto out;
-	}
+	max_devices = 1;
+	if (ap->flags & ATA_FLAG_SLAVE_POSS)
+		max_devices++;
 
-	/* if basepath is not yet known, ignore this object */
-	if (!winfo->basepath_len)
-		goto out;
+	for (i = 0; i < max_devices; i++) {
+		struct ata_device *adev = &ap->device[i];
 
-	/* if this object is in scope of basepath, maybe use it */
-	if (strncmp(pathname, winfo->basepath,
-	    winfo->basepath_len) == 0) {
-		if (!(dinfo->valid & ACPI_VALID_ADR))
-			goto out;
-		if (ata_msg_probe(winfo->ataport))
-			ata_dev_printk(winfo->atadev, KERN_DEBUG,
-				"GOT ONE: (%s) root_port = 0x%llx,"
-				" port_num = 0x%llx\n", pathname,
-				SATA_ROOT_PORT(dinfo->address),
-				SATA_PORT_NUMBER(dinfo->address));
-		/* heuristics: */
-		if (SATA_PORT_NUMBER(dinfo->address) != NO_PORT_MULT)
-			if (ata_msg_probe(winfo->ataport))
-				ata_dev_printk(winfo->atadev,
-					KERN_DEBUG, "warning: don't"
-					" know how to handle SATA port"
-					" multiplier\n");
-		if (SATA_ROOT_PORT(dinfo->address) ==
-			winfo->ataport->port_no &&
-		    SATA_PORT_NUMBER(dinfo->address) == NO_PORT_MULT) {
-			if (ata_msg_probe(winfo->ataport))
-				ata_dev_printk(winfo->atadev,
-					KERN_DEBUG,
-					"THIS ^^^^^ is the requested"
-					" SATA drive (handle = 0x%p)\n",
-					handle);
-			winfo->sata_adr = dinfo->address;
-			winfo->obj_handle = handle;
-		}
+		adev->acpi_handle = acpi_get_child(ap->acpi_handle, i);
 	}
-out:
-	kfree(dinfo);
-out2:
-	kfree(pathname);
-
-ret:
-	return status;
 }
 
-/* Get the SATA drive _ADR object. */
-static int get_sata_adr(struct device *dev, acpi_handle handle,
-			acpi_integer pcidevfn, unsigned int drive,
-			struct ata_port *ap,
-			struct ata_device *atadev, u32 *dev_adr)
+/**
+ * ata_acpi_associate - associate ATA host with ACPI objects
+ * @host: target ATA host
+ *
+ * Look up ACPI objects associated with @host and initialize
+ * acpi_handle fields of @host, its ports and devices accordingly.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
+ */
+void ata_acpi_associate(struct ata_host *host)
 {
-	acpi_status	status;
-	struct walk_info *winfo;
-	int		err = -ENOMEM;
+	int i;
 
-	winfo = kzalloc(sizeof(struct walk_info), GFP_KERNEL);
-	if (!winfo)
-		goto out;
+	if (!is_pci_dev(host->dev) || libata_noacpi)
+		return;
 
-	winfo->dev = dev;
-	winfo->atadev = atadev;
-	winfo->ataport = ap;
-	if (acpi_bus_get_device(handle, &winfo->adev) < 0)
-		if (ata_msg_probe(ap))
-			ata_dev_printk(winfo->atadev, KERN_DEBUG,
-				"acpi_bus_get_device failed\n");
-	winfo->handle = handle;
-	winfo->pcidevfn = pcidevfn;
-	winfo->drivenum = drive;
+	host->acpi_handle = DEVICE_ACPI_HANDLE(host->dev);
+	if (!host->acpi_handle)
+		return;
 
-	status = acpi_get_devices(NULL, get_devices, winfo, NULL);
-	if (ACPI_FAILURE(status)) {
-		if (ata_msg_probe(ap))
-			ata_dev_printk(winfo->atadev, KERN_DEBUG,
-				"%s: acpi_get_devices failed\n",
-				__FUNCTION__);
-		err = -ENODEV;
-	} else {
-		*dev_adr = winfo->sata_adr;
-		atadev->obj_handle = winfo->obj_handle;
-		err = 0;
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (host->ports[0]->flags & ATA_FLAG_ACPI_SATA)
+			ata_acpi_associate_sata_port(ap);
+		else
+			ata_acpi_associate_ide_port(ap);
 	}
-	kfree(winfo);
-out:
-	return err;
 }
 
 /**
@@ -290,20 +122,15 @@ static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
 {
 	struct ata_port *ap = adev->ap;
 	acpi_status status;
-	acpi_handle dev_handle = NULL;
-	acpi_handle chan_handle, drive_handle;
-	acpi_integer pcidevfn = 0;
-	u32 dev_adr;
 	struct acpi_buffer output;
 	union acpi_object *out_obj;
-	struct device *dev = ap->host->dev;
 	int err = -ENODEV;
 
 	*gtf_length = 0;
 	*gtf_address = 0UL;
 	*obj_loc = 0UL;
 
-	if (libata_noacpi)
+	if (!adev->acpi_handle)
 		return 0;
 
 	if (ata_msg_probe(ap))
@@ -319,78 +146,14 @@ static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
 		goto out;
 	}
 
-	/* Don't continue if device has no _ADR method.
-	 * _GTF is intended for known motherboard devices. */
-	if (!(ap->flags & ATA_FLAG_ACPI_SATA)) {
-		err = pata_get_dev_handle(dev, &dev_handle, &pcidevfn);
-		if (err < 0) {
-			if (ata_msg_probe(ap))
-				ata_dev_printk(adev, KERN_DEBUG,
-					"%s: pata_get_dev_handle failed (%d)\n",
-					__FUNCTION__, err);
-			goto out;
-		}
-	} else {
-		err = sata_get_dev_handle(dev, &dev_handle, &pcidevfn);
-		if (err < 0) {
-			if (ata_msg_probe(ap))
-				ata_dev_printk(adev, KERN_DEBUG,
-					"%s: sata_get_dev_handle failed (%d\n",
-					__FUNCTION__, err);
-			goto out;
-		}
-	}
-
-	/* Get this drive's _ADR info. if not already known. */
-	if (!adev->obj_handle) {
-		if (!(ap->flags & ATA_FLAG_ACPI_SATA)) {
-			/* get child objects of dev_handle == channel objects,
-	 		 * + _their_ children == drive objects */
-			/* channel is ap->port_no */
-			chan_handle = acpi_get_child(dev_handle,
-						ap->port_no);
-			if (ata_msg_probe(ap))
-				ata_dev_printk(adev, KERN_DEBUG,
-					"%s: chan adr=%d: chan_handle=0x%p\n",
-					__FUNCTION__, ap->port_no,
-					chan_handle);
-			if (!chan_handle) {
-				err = -ENODEV;
-				goto out;
-			}
-			/* TBD: could also check ACPI object VALID bits */
-			drive_handle = acpi_get_child(chan_handle, adev->devno);
-			if (!drive_handle) {
-				err = -ENODEV;
-				goto out;
-			}
-			dev_adr = adev->devno;
-			adev->obj_handle = drive_handle;
-		} else {	/* for SATA mode */
-			dev_adr = SATA_ADR_RSVD;
-			err = get_sata_adr(dev, dev_handle, pcidevfn, 0,
-					ap, adev, &dev_adr);
-		}
-		if (err < 0 || dev_adr == SATA_ADR_RSVD ||
-		    !adev->obj_handle) {
-			if (ata_msg_probe(ap))
-				ata_dev_printk(adev, KERN_DEBUG,
-					"%s: get_sata/pata_adr failed: "
-					"err=%d, dev_adr=%u, obj_handle=0x%p\n",
-					__FUNCTION__, err, dev_adr,
-					adev->obj_handle);
-			goto out;
-		}
-	}
-
 	/* Setting up output buffer */
 	output.length = ACPI_ALLOCATE_BUFFER;
 	output.pointer = NULL;	/* ACPI-CA sets this; save/free it later */
 
 	/* _GTF has no input parameters */
 	err = -EIO;
-	status = acpi_evaluate_object(adev->obj_handle, "_GTF",
-					NULL, &output);
+	status = acpi_evaluate_object(adev->acpi_handle, "_GTF",
+				      NULL, &output);
 	if (ACPI_FAILURE(status)) {
 		if (ata_msg_probe(ap))
 			ata_dev_printk(adev, KERN_DEBUG,
@@ -528,7 +291,7 @@ static int do_drive_set_taskfiles(struct ata_device *adev,
 		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
 			       __FUNCTION__, ap->port_no);
 
-	if (libata_noacpi || !(ap->flags & ATA_FLAG_ACPI_SATA))
+	if (!(ap->flags & ATA_FLAG_ACPI_SATA))
 		return 0;
 
 	if (!ata_dev_enabled(adev) || (ap->flags & ATA_FLAG_DISABLED))
@@ -571,8 +334,6 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 	unsigned long gtf_address;
 	unsigned long obj_loc;
 
-	if (libata_noacpi)
-		return 0;
 	/*
 	 * TBD - implement PATA support.  For now,
 	 * we should not run GTF on PATA devices since some
@@ -624,16 +385,12 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 int ata_acpi_push_id(struct ata_device *adev)
 {
 	struct ata_port *ap = adev->ap;
-	acpi_handle handle;
-	acpi_integer pcidevfn;
 	int err;
-	struct device *dev = ap->host->dev;
-	u32 dev_adr;
 	acpi_status status;
 	struct acpi_object_list input;
 	union acpi_object in_params[1];
 
-	if (libata_noacpi)
+	if (!adev->acpi_handle)
 		return 0;
 
 	if (ata_msg_probe(ap))
@@ -648,34 +405,6 @@ int ata_acpi_push_id(struct ata_device *adev)
 		goto out;
 	}
 
-	/* Don't continue if device has no _ADR method.
-	 * _SDD is intended for known motherboard devices. */
-	err = sata_get_dev_handle(dev, &handle, &pcidevfn);
-	if (err < 0) {
-		if (ata_msg_probe(ap))
-			ata_dev_printk(adev, KERN_DEBUG,
-				"%s: sata_get_dev_handle failed (%d\n",
-				__FUNCTION__, err);
-		goto out;
-	}
-
-	/* Get this drive's _ADR info, if not already known */
-	if (!adev->obj_handle) {
-		dev_adr = SATA_ADR_RSVD;
-		err = get_sata_adr(dev, handle, pcidevfn, adev->devno, ap, adev,
-					&dev_adr);
-		if (err < 0 || dev_adr == SATA_ADR_RSVD ||
-			!adev->obj_handle) {
-			if (ata_msg_probe(ap))
-				ata_dev_printk(adev, KERN_DEBUG,
-					"%s: get_sata_adr failed: "
-					"err=%d, dev_adr=%u, obj_handle=0x%p\n",
-					__FUNCTION__, err, dev_adr,
-					adev->obj_handle);
-			goto out;
-		}
-	}
-
 	/* Give the drive Identify data to the drive via the _SDD method */
 	/* _SDD: set up input parameters */
 	input.count = 1;
@@ -687,7 +416,7 @@ int ata_acpi_push_id(struct ata_device *adev)
 
 	/* It's OK for _SDD to be missing too. */
 	swap_buf_le16(adev->id, ATA_ID_WORDS);
-	status = acpi_evaluate_object(adev->obj_handle, "_SDD", &input, NULL);
+	status = acpi_evaluate_object(adev->acpi_handle, "_SDD", &input, NULL);
 	swap_buf_le16(adev->id, ATA_ID_WORDS);
 
 	err = ACPI_FAILURE(status) ? -EIO : 0;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d3d0f01..5ff8a38 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6223,6 +6223,9 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	if (rc)
 		return rc;
 
+	/* associate with ACPI nodes */
+	ata_acpi_associate(host);
+
 	/* set cable, sata_spd_limit and report */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 6041b91..b91317e 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -98,9 +98,11 @@ extern struct ata_port *ata_port_alloc(struct ata_host *host);
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
+extern void ata_acpi_associate(struct ata_host *host);
 extern int ata_acpi_exec_tfs(struct ata_port *ap);
 extern int ata_acpi_push_id(struct ata_device *adev);
 #else
+static inline void ata_acpi_associate(struct ata_host *host) { }
 static inline int ata_acpi_exec_tfs(struct ata_port *ap)
 {
 	return 0;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a26bb7f..9a517e6 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -381,6 +381,9 @@ struct ata_host {
 	void			*private_data;
 	const struct ata_port_operations *ops;
 	unsigned long		flags;
+#ifdef CONFIG_ATA_ACPI
+	acpi_handle		acpi_handle;
+#endif
 	struct ata_port		*simplex_claimed;	/* channel owning the DMA */
 	struct ata_port		*ports[0];
 };
@@ -447,6 +450,9 @@ struct ata_device {
 	unsigned int		devno;		/* 0 or 1 */
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	struct scsi_device	*sdev;		/* attached SCSI device */
+#ifdef CONFIG_ATA_ACPI
+	acpi_handle		acpi_handle;
+#endif
 	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
 	u64			n_sectors;	/* size of device, if ATA */
 	unsigned int		class;		/* ATA_DEV_xxx */
@@ -475,10 +481,6 @@ struct ata_device {
 	struct ata_ering	ering;
 	int			spdn_cnt;
 	unsigned int		horkage;	/* List of broken features */
-#ifdef CONFIG_ATA_ACPI
-	/* ACPI objects info */
-	acpi_handle obj_handle;
-#endif
 };
 
 /* Offset into struct ata_device.  Fields above it are maintained
@@ -568,6 +570,9 @@ struct ata_port {
 
 	void			*private_data;
 
+#ifdef CONFIG_ATA_ACPI
+	acpi_handle		acpi_handle;
+#endif
 	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
 };
 
-- 
1.5.0.3

Re: [PATCH 08/13] libata-acpi: implement ata_acpi_associate()

[Jeff Garzik]

Tejun Heo wrote:
> * Add acpi_handle to ata_host and ata_port.  Rename
>   ata_device->obj_handle to ->acpi_handle and move it above such that
>   it doesn't get cleared on reconfiguration.
> 
> * Replace ACPI node association which ata_acpi_associate() which is
>   called once during host initialization.  Unlike the previous
>   implementation, ata_acpi_associate() uses ATA_FLAG_ACPI_SATA to
>   choose between IDE or SATA ACPI hierarchy and uses simple child look
>   up instead of recursive walk to match the nodes.  This is way safer
>   and simpler.  Please read the following message for more info.
> 
>   http://article.gmane.org/gmane.linux.ide/17554
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-acpi.c |  369 ++++++---------------------------------------
>  drivers/ata/libata-core.c |    3 +
>  drivers/ata/libata.h      |    2 +
>  include/linux/libata.h    |   13 +-
>  4 files changed, 63 insertions(+), 324 deletions(-)

largely dependent on previous patch's comments

[PATCH 10/13] libata-acpi: miscellaneous cleanups

[Tejun Heo]

* Add missing LOCKING: and RETURNS: to function comment.

* Don't conditionalize warning messages with ata_msg_probe().  Print
  directly with KERN_WARNING.

* Drop duplicate debug messages.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 21aad07..2750c36 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -174,21 +174,17 @@ static int do_drive_get_GTF(struct ata_device *adev, struct ata_acpi_gtf **gtf,
 
 	out_obj = output.pointer;
 	if (out_obj->type != ACPI_TYPE_BUFFER) {
-		if (ata_msg_probe(ap))
-			ata_dev_printk(adev, KERN_DEBUG, "%s: Run _GTF: "
-				"error: expected object type of "
-				" ACPI_TYPE_BUFFER, got 0x%x\n",
-				__FUNCTION__, out_obj->type);
+		ata_dev_printk(adev, KERN_WARNING,
+			       "_GTF unexpected object type 0x%x\n",
+			       out_obj->type);
 		rc = -EINVAL;
 		goto out_free;
 	}
 
 	if (out_obj->buffer.length % REGS_PER_GTF) {
-		if (ata_msg_drv(ap))
-			ata_dev_printk(adev, KERN_ERR,
-				"%s: unexpected GTF length (%d) or addr (0x%p)\n",
-				__FUNCTION__, out_obj->buffer.length,
-				out_obj->buffer.pointer);
+		ata_dev_printk(adev, KERN_WARNING,
+			       "unexpected _GTF length (%d)\n",
+			       out_obj->buffer.length);
 		rc = -EINVAL;
 		goto out_free;
 	}
@@ -319,6 +315,12 @@ static int do_drive_set_taskfiles(struct ata_device *adev,
  * @ap: the ata_port for the drive
  *
  * This applies to both PATA and SATA drives.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
  */
 int ata_acpi_exec_tfs(struct ata_port *ap)
 {
@@ -344,24 +346,14 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 		ret = do_drive_get_GTF(adev, &gtf, &ptr_to_free);
 		if (ret == 0)
 			continue;
-		if (ret < 0) {
-			if (ata_msg_probe(ap))
-				ata_port_printk(ap, KERN_DEBUG,
-					"%s: get_GTF error (%d)\n",
-					__FUNCTION__, ret);
+		if (ret < 0)
 			break;
-		}
 		gtf_count = ret;
 
 		ret = do_drive_set_taskfiles(adev, gtf, gtf_count);
 		kfree(ptr_to_free);
-		if (ret < 0) {
-			if (ata_msg_probe(ap))
-				ata_port_printk(ap, KERN_DEBUG,
-					"%s: set_taskfiles error (%d)\n",
-					__FUNCTION__, ret);
+		if (ret < 0)
 			break;
-		}
 	}
 
 	return ret;
@@ -376,6 +368,12 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
  * ATM this function never returns a failure.  It is an optional
  * method and if it fails for whatever reason, we should still
  * just keep going.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
  */
 int ata_acpi_push_id(struct ata_device *adev)
 {
@@ -415,12 +413,9 @@ int ata_acpi_push_id(struct ata_device *adev)
 	swap_buf_le16(adev->id, ATA_ID_WORDS);
 
 	err = ACPI_FAILURE(status) ? -EIO : 0;
-	if (err < 0) {
-		if (ata_msg_probe(ap))
-			ata_dev_printk(adev, KERN_DEBUG,
-				       "%s _SDD error: status = 0x%x\n",
-				       __FUNCTION__, status);
-	}
+	if (err < 0)
+		ata_dev_printk(adev, KERN_WARNING,
+			       "ACPI _SDD failed (AE 0x%x)\n", status);
 
 	/* always return success */
 out:
-- 
1.5.0.3

Re: [PATCH 10/13] libata-acpi: miscellaneous cleanups

[Jeff Garzik]

Tejun Heo wrote:
> * Add missing LOCKING: and RETURNS: to function comment.
> 
> * Don't conditionalize warning messages with ata_msg_probe().  Print
>   directly with KERN_WARNING.
> 
> * Drop duplicate debug messages.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-acpi.c |   51 ++++++++++++++++++++------------------------
>  1 files changed, 23 insertions(+), 28 deletions(-)

ACK

[PATCH 04/13] libata: during revalidation, check n_sectors after device is configured

[Tejun Heo]

Device might be resized during ata_dev_configure() due to HPA or
(later) ACPI _GTF.  Currently it's worked around by caching n_sectors
before turning off HPA.  The cached original size is overwritten if
the device is reconfigured without being hardreset - which always
happens after configuring trasnfer mode.  If the device gets hardreset
for some reason after that, revalidation fails with -ENODEV.

This patch makes size checking more robust by moving n_sectors check
from ata_dev_reread_id() to ata_dev_revalidate() after the device is
fully configured.  No matter what happens during configuration, a
device must have the same n_sectors after fully configured to be
treated as the same device.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 7a6e8fd..2171af6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1862,7 +1862,6 @@ int ata_dev_configure(struct ata_device *dev)
 			snprintf(revbuf, 7, "ATA-%d",  ata_id_major_version(id));
 
 		dev->n_sectors = ata_id_n_sectors(id);
-		dev->n_sectors_boot = dev->n_sectors;
 
 		/* SCSI only uses 4-char revisions, dump full 8 chars from ATA */
 		ata_id_c_string(dev->id, fwrevbuf, ATA_ID_FW_REV,
@@ -3519,7 +3518,6 @@ static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
 	const u16 *old_id = dev->id;
 	unsigned char model[2][ATA_ID_PROD_LEN + 1];
 	unsigned char serial[2][ATA_ID_SERNO_LEN + 1];
-	u64 new_n_sectors;
 
 	if (dev->class != new_class) {
 		ata_dev_printk(dev, KERN_INFO, "class mismatch %d != %d\n",
@@ -3531,7 +3529,6 @@ static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
 	ata_id_c_string(new_id, model[1], ATA_ID_PROD, sizeof(model[1]));
 	ata_id_c_string(old_id, serial[0], ATA_ID_SERNO, sizeof(serial[0]));
 	ata_id_c_string(new_id, serial[1], ATA_ID_SERNO, sizeof(serial[1]));
-	new_n_sectors = ata_id_n_sectors(new_id);
 
 	if (strcmp(model[0], model[1])) {
 		ata_dev_printk(dev, KERN_INFO, "model number mismatch "
@@ -3545,19 +3542,6 @@ static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
 		return 0;
 	}
 
-	if (dev->class == ATA_DEV_ATA && dev->n_sectors != new_n_sectors) {
-		ata_dev_printk(dev, KERN_INFO, "n_sectors mismatch "
-			       "%llu != %llu\n",
-			       (unsigned long long)dev->n_sectors,
-			       (unsigned long long)new_n_sectors);
-		/* Are we the boot time size - if so we appear to be the
-		   same disk at this point and our HPA got reapplied */
-		if (ata_ignore_hpa && dev->n_sectors_boot == new_n_sectors 
-		    && ata_id_hpa_enabled(new_id))
-			return 1;
-		return 0;
-	}
-
 	return 1;
 }
 
@@ -3610,6 +3594,7 @@ int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags)
  */
 int ata_dev_revalidate(struct ata_device *dev, unsigned int readid_flags)
 {
+	u64 n_sectors = dev->n_sectors;
 	int rc;
 
 	if (!ata_dev_enabled(dev))
@@ -3622,8 +3607,20 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int readid_flags)
 
 	/* configure device according to the new ID */
 	rc = ata_dev_configure(dev);
-	if (rc == 0)
-		return 0;
+	if (rc)
+		goto fail;
+
+	/* verify n_sectors hasn't changed */
+	if (dev->class == ATA_DEV_ATA && dev->n_sectors != n_sectors) {
+		ata_dev_printk(dev, KERN_INFO, "n_sectors mismatch "
+			       "%llu != %llu\n",
+			       (unsigned long long)n_sectors,
+			       (unsigned long long)dev->n_sectors);
+		rc = -ENODEV;
+		goto fail;
+	}
+
+	return 0;
 
  fail:
 	ata_dev_printk(dev, KERN_ERR, "revalidation failed (errno=%d)\n", rc);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d8cfc72..3cffbf6 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -448,7 +448,6 @@ struct ata_device {
 	struct scsi_device	*sdev;		/* attached SCSI device */
 	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
 	u64			n_sectors;	/* size of device, if ATA */
-	u64			n_sectors_boot;	/* size of ATA device at startup */
 	unsigned int		class;		/* ATA_DEV_xxx */
 	u16			id[ATA_ID_WORDS]; /* IDENTIFY xxx DEVICE data */
 	u8			pio_mode;
-- 
1.5.0.3

[PATCH 09/13] libata-acpi: clean up ata_acpi_exec_tfs()

[Tejun Heo]

This patch cleans up ata_acpi_exec_tfs() and its friends.

* Rename taskfile_array to ata_acpi_gtf and make it __packed as it's
  used as argument to ACPI method, and use pointer to ata_acpi_gtf and
  number of taskfiles to represent _GTF taskfiles instead of a pointer
  casted into unsigned long and byte count.  This makes argument
  re-checking in do_drive_set_taskfiles() unnecessary.

* Pointer in void * not in unsigned long.

* Clean up do_drive_get_GTF() error handling and make
  do_drive_get_GTF() return number of taskfiles on success, 0 if _GTF
  doesn't exist or doesn't contain valid ata.  -errno on other errors.

* Remove superflous check for acpi->buffer.pointer.

* Update taskfile_load_raw() such that printed messages look similar
  to the messages printed by ata_eh_report().

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index a2745b5..21aad07 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -28,9 +28,9 @@
 #define SATA_ADR(root,pmp)	(((root) << 16) | (pmp))
 
 #define REGS_PER_GTF		7
-struct taskfile_array {
-	u8	tfa[REGS_PER_GTF];	/* regs. 0x1f1 - 0x1f7 */
-};
+struct ata_acpi_gtf {
+	u8	tf[REGS_PER_GTF];	/* regs. 0x1f1 - 0x1f7 */
+} __packed;
 
 /*
  *	Helper - belongs in the PCI layer somewhere eventually
@@ -103,8 +103,8 @@ void ata_acpi_associate(struct ata_host *host)
 /**
  * do_drive_get_GTF - get the drive bootup default taskfile settings
  * @adev: target ATA device
- * @gtf_length: number of bytes of _GTF data returned at @gtf_address
- * @gtf_address: buffer containing _GTF taskfile arrays
+ * @gtf: output parameter for buffer containing _GTF taskfile arrays
+ * @ptr_to_free: pointer which should be freed
  *
  * This applies to both PATA and SATA drives.
  *
@@ -114,24 +114,28 @@ void ata_acpi_associate(struct ata_host *host)
  * The <variable number> is not known in advance, so have ACPI-CA
  * allocate the buffer as needed and return it, then free it later.
  *
- * The returned @gtf_length and @gtf_address are only valid if the
- * function return value is 0.
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * Number of taskfiles on success, 0 if _GTF doesn't exist or doesn't
+ * contain valid data.  -errno on other errors.
  */
-static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
-			    unsigned long *gtf_address, unsigned long *obj_loc)
+static int do_drive_get_GTF(struct ata_device *adev, struct ata_acpi_gtf **gtf,
+			    void **ptr_to_free)
 {
 	struct ata_port *ap = adev->ap;
 	acpi_status status;
 	struct acpi_buffer output;
 	union acpi_object *out_obj;
-	int err = -ENODEV;
+	int rc = 0;
 
-	*gtf_length = 0;
-	*gtf_address = 0UL;
-	*obj_loc = 0UL;
+	/* set up output buffer */
+	output.length = ACPI_ALLOCATE_BUFFER;
+	output.pointer = NULL;	/* ACPI-CA sets this; save/free it later */
 
 	if (!adev->acpi_handle)
-		return 0;
+		goto out_free;
 
 	if (ata_msg_probe(ap))
 		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
@@ -143,23 +147,19 @@ static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
 				"ata_dev_present: %d, PORT_DISABLED: %lu\n",
 				__FUNCTION__, ata_dev_enabled(adev),
 				ap->flags & ATA_FLAG_DISABLED);
-		goto out;
+		goto out_free;
 	}
 
-	/* Setting up output buffer */
-	output.length = ACPI_ALLOCATE_BUFFER;
-	output.pointer = NULL;	/* ACPI-CA sets this; save/free it later */
-
 	/* _GTF has no input parameters */
-	err = -EIO;
-	status = acpi_evaluate_object(adev->acpi_handle, "_GTF",
-				      NULL, &output);
+	status = acpi_evaluate_object(adev->acpi_handle, "_GTF", NULL, &output);
 	if (ACPI_FAILURE(status)) {
-		if (ata_msg_probe(ap))
-			ata_dev_printk(adev, KERN_DEBUG,
-				"%s: Run _GTF error: status = 0x%x\n",
-				__FUNCTION__, status);
-		goto out;
+		if (status != AE_NOT_FOUND) {
+			ata_dev_printk(adev, KERN_WARNING,
+				       "_GTF evaluation failed (AE 0x%x)\n",
+				       status);
+			rc = -EIO;
+		}
+		goto out_free;
 	}
 
 	if (!output.length || !output.pointer) {
@@ -169,43 +169,43 @@ static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
 				__FUNCTION__,
 				(unsigned long long)output.length,
 				output.pointer);
-		kfree(output.pointer);
-		goto out;
+		goto out_free;
 	}
 
 	out_obj = output.pointer;
 	if (out_obj->type != ACPI_TYPE_BUFFER) {
-		kfree(output.pointer);
 		if (ata_msg_probe(ap))
 			ata_dev_printk(adev, KERN_DEBUG, "%s: Run _GTF: "
 				"error: expected object type of "
 				" ACPI_TYPE_BUFFER, got 0x%x\n",
 				__FUNCTION__, out_obj->type);
-		err = -ENOENT;
-		goto out;
+		rc = -EINVAL;
+		goto out_free;
 	}
 
-	if (!out_obj->buffer.length || !out_obj->buffer.pointer ||
-	    out_obj->buffer.length % REGS_PER_GTF) {
+	if (out_obj->buffer.length % REGS_PER_GTF) {
 		if (ata_msg_drv(ap))
 			ata_dev_printk(adev, KERN_ERR,
 				"%s: unexpected GTF length (%d) or addr (0x%p)\n",
 				__FUNCTION__, out_obj->buffer.length,
 				out_obj->buffer.pointer);
-		err = -ENOENT;
-		goto out;
+		rc = -EINVAL;
+		goto out_free;
 	}
 
-	*gtf_length = out_obj->buffer.length;
-	*gtf_address = (unsigned long)out_obj->buffer.pointer;
-	*obj_loc = (unsigned long)out_obj;
+	*ptr_to_free = out_obj;
+	*gtf = (void *)out_obj->buffer.pointer;
+	rc = out_obj->buffer.length / REGS_PER_GTF;
+
 	if (ata_msg_probe(ap))
 		ata_dev_printk(adev, KERN_DEBUG, "%s: returning "
-			"gtf_length=%d, gtf_address=0x%lx, obj_loc=0x%lx\n",
-			__FUNCTION__, *gtf_length, *gtf_address, *obj_loc);
-	err = 0;
-out:
-	return err;
+			"gtf=%p, gtf_count=%d, ptr_to_free=%p\n",
+			__FUNCTION__, *gtf, rc, *ptr_to_free);
+	return rc;
+
+ out_free:
+	kfree(output.pointer);
+	return rc;
 }
 
 /**
@@ -224,68 +224,78 @@ out:
  * function also waits for idle after writing control and before
  * writing the remaining registers.
  *
- * LOCKING: TBD:
- * Inherited from caller.
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
  */
-static void taskfile_load_raw(struct ata_device *adev,
-			      const struct taskfile_array *gtf)
+static int taskfile_load_raw(struct ata_device *adev,
+			      const struct ata_acpi_gtf *gtf)
 {
 	struct ata_port *ap = adev->ap;
-	struct ata_taskfile tf;
-	unsigned int err;
+	struct ata_taskfile tf, rtf;
+	unsigned int err_mask;
 
-	if (ata_msg_probe(ap))
-		ata_dev_printk(adev, KERN_DEBUG, "%s: (0x1f1-1f7): hex: "
-			"%02x %02x %02x %02x %02x %02x %02x\n",
-			__FUNCTION__,
-			gtf->tfa[0], gtf->tfa[1], gtf->tfa[2],
-			gtf->tfa[3], gtf->tfa[4], gtf->tfa[5], gtf->tfa[6]);
-
-	if ((gtf->tfa[0] == 0) && (gtf->tfa[1] == 0) && (gtf->tfa[2] == 0)
-	    && (gtf->tfa[3] == 0) && (gtf->tfa[4] == 0) && (gtf->tfa[5] == 0)
-	    && (gtf->tfa[6] == 0))
-		return;
+	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)
+	    && (gtf->tf[6] == 0))
+		return 0;
 
 	ata_tf_init(adev, &tf);
 
 	/* convert gtf to tf */
 	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; /* TBD */
 	tf.protocol = ATA_PROT_NODATA;
-	tf.feature = gtf->tfa[0];	/* 0x1f1 */
-	tf.nsect   = gtf->tfa[1];	/* 0x1f2 */
-	tf.lbal    = gtf->tfa[2];	/* 0x1f3 */
-	tf.lbam    = gtf->tfa[3];	/* 0x1f4 */
-	tf.lbah    = gtf->tfa[4];	/* 0x1f5 */
-	tf.device  = gtf->tfa[5];	/* 0x1f6 */
-	tf.command = gtf->tfa[6];	/* 0x1f7 */
-
-	err = ata_exec_internal(adev, &tf, NULL, DMA_NONE, NULL, 0);
-	if (err && ata_msg_probe(ap))
+	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 */
+
+	if (ata_msg_probe(ap))
+		ata_dev_printk(adev, 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(adev, &rtf, NULL, DMA_NONE, NULL, 0);
+	if (err_mask) {
 		ata_dev_printk(adev, KERN_ERR,
-			"%s: ata_exec_internal failed: %u\n",
-			__FUNCTION__, 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;
+	}
+
+	return 0;
 }
 
 /**
  * do_drive_set_taskfiles - write the drive taskfile settings from _GTF
  * @adev: target ATA device
- * @gtf_length: total number of bytes of _GTF taskfiles
- * @gtf_address: location of _GTF taskfile arrays
+ * @gtf: pointer to array of _GTF taskfiles to execute
+ * @gtf_count: number of taskfiles
  *
  * This applies to both PATA and SATA drives.
  *
- * Write {gtf_address, length gtf_length} in groups of
- * REGS_PER_GTF bytes.
+ * Execute taskfiles in @gtf.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
  */
 static int do_drive_set_taskfiles(struct ata_device *adev,
-				  unsigned int gtf_length,
-				  unsigned long gtf_address)
+				  struct ata_acpi_gtf *gtf, int gtf_count)
 {
 	struct ata_port *ap = adev->ap;
-	int err = -ENODEV;
-	int gtf_count = gtf_length / REGS_PER_GTF;
 	int ix;
-	struct taskfile_array	*gtf;
 
 	if (ata_msg_probe(ap))
 		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
@@ -295,29 +305,13 @@ static int do_drive_set_taskfiles(struct ata_device *adev,
 		return 0;
 
 	if (!ata_dev_enabled(adev) || (ap->flags & ATA_FLAG_DISABLED))
-		goto out;
-	if (!gtf_count)		/* shouldn't be here */
-		goto out;
-
-	if (gtf_length % REGS_PER_GTF) {
-		if (ata_msg_drv(ap))
-			ata_dev_printk(adev, KERN_ERR,
-				"%s: unexpected GTF length (%d)\n",
-				__FUNCTION__, gtf_length);
-		goto out;
-	}
+		return -ENODEV;
 
-	for (ix = 0; ix < gtf_count; ix++) {
-		gtf = (struct taskfile_array *)
-			(gtf_address + ix * REGS_PER_GTF);
+	/* send all TaskFile registers (0x1f1-0x1f7) *in*that*order* */
+	for (ix = 0; ix < gtf_count; ix++)
+		 taskfile_load_raw(adev, gtf++);
 
-		/* send all TaskFile registers (0x1f1-0x1f7) *in*that*order* */
-		taskfile_load_raw(adev, gtf);
-	}
-
-	err = 0;
-out:
-	return err;
+	return 0;
 }
 
 /**
@@ -328,11 +322,7 @@ out:
  */
 int ata_acpi_exec_tfs(struct ata_port *ap)
 {
-	int ix;
-	int ret = 0;
-	unsigned int gtf_length;
-	unsigned long gtf_address;
-	unsigned long obj_loc;
+	int ix, ret = 0;
 
 	/*
 	 * TBD - implement PATA support.  For now,
@@ -344,12 +334,16 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 
 	for (ix = 0; ix < ATA_MAX_DEVICES; ix++) {
 		struct ata_device *adev = &ap->device[ix];
+		struct ata_acpi_gtf *gtf = NULL;
+		int gtf_count;
+		void *ptr_to_free = NULL;
 
 		if (!ata_dev_enabled(adev))
 			continue;
 
-		ret = do_drive_get_GTF(adev, &gtf_length, &gtf_address,
-				       &obj_loc);
+		ret = do_drive_get_GTF(adev, &gtf, &ptr_to_free);
+		if (ret == 0)
+			continue;
 		if (ret < 0) {
 			if (ata_msg_probe(ap))
 				ata_port_printk(ap, KERN_DEBUG,
@@ -357,9 +351,10 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 					__FUNCTION__, ret);
 			break;
 		}
+		gtf_count = ret;
 
-		ret = do_drive_set_taskfiles(adev, gtf_length, gtf_address);
-		kfree((void *)obj_loc);
+		ret = do_drive_set_taskfiles(adev, gtf, gtf_count);
+		kfree(ptr_to_free);
 		if (ret < 0) {
 			if (ata_msg_probe(ap))
 				ata_port_printk(ap, KERN_DEBUG,
-- 
1.5.0.3

Re: [PATCH 09/13] libata-acpi: clean up ata_acpi_exec_tfs()

[Jeff Garzik]

Tejun Heo wrote:
> This patch cleans up ata_acpi_exec_tfs() and its friends.
> 
> * Rename taskfile_array to ata_acpi_gtf and make it __packed as it's
>   used as argument to ACPI method, and use pointer to ata_acpi_gtf and
>   number of taskfiles to represent _GTF taskfiles instead of a pointer
>   casted into unsigned long and byte count.  This makes argument
>   re-checking in do_drive_set_taskfiles() unnecessary.
> 
> * Pointer in void * not in unsigned long.
> 
> * Clean up do_drive_get_GTF() error handling and make
>   do_drive_get_GTF() return number of taskfiles on success, 0 if _GTF
>   doesn't exist or doesn't contain valid ata.  -errno on other errors.
> 
> * Remove superflous check for acpi->buffer.pointer.
> 
> * Update taskfile_load_raw() such that printed messages look similar
>   to the messages printed by ata_eh_report().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-acpi.c |  219 ++++++++++++++++++++++-----------------------
>  1 files changed, 107 insertions(+), 112 deletions(-)

ACK

As an aside, I hate the "do_" prefix on functions.  It is utterly redundant.

Re: [PATCH 09/13] libata-acpi: clean up ata_acpi_exec_tfs()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> This patch cleans up ata_acpi_exec_tfs() and its friends.
>>
>> * Rename taskfile_array to ata_acpi_gtf and make it __packed as it's
>>   used as argument to ACPI method, and use pointer to ata_acpi_gtf and
>>   number of taskfiles to represent _GTF taskfiles instead of a pointer
>>   casted into unsigned long and byte count.  This makes argument
>>   re-checking in do_drive_set_taskfiles() unnecessary.
>>
>> * Pointer in void * not in unsigned long.
>>
>> * Clean up do_drive_get_GTF() error handling and make
>>   do_drive_get_GTF() return number of taskfiles on success, 0 if _GTF
>>   doesn't exist or doesn't contain valid ata.  -errno on other errors.
>>
>> * Remove superflous check for acpi->buffer.pointer.
>>
>> * Update taskfile_load_raw() such that printed messages look similar
>>   to the messages printed by ata_eh_report().
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>>  drivers/ata/libata-acpi.c |  219
>> ++++++++++++++++++++++-----------------------
>>  1 files changed, 107 insertions(+), 112 deletions(-)
> 
> ACK
> 
> As an aside, I hate the "do_" prefix on functions.  It is utterly
> redundant.

Okay, will drop.

-- 
tejun

[PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Tejun Heo]

Whether a controller needs IDE or SATA ACPI hierarchy is determined by
the programming interface of the controller not by whether the
controller is SATA or PATA, or it supports slave device or not.  This
patch adds ATA_FLAG_ACPI_SATA port flags which tells libata-acpi that
the port needs SATA ACPI nodes, and sets the flag for ahci and
sata_sil24.

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

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 34c5534..7f739e8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -173,7 +173,8 @@ enum {
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-					  ATA_FLAG_SKIP_D2H_BSY,
+					  ATA_FLAG_SKIP_D2H_BSY |
+					  ATA_FLAG_ACPI_SATA,
 };
 
 struct ahci_cmd_hdr {
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index f005730..24c2198 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -321,7 +321,7 @@ static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
 
 	/* Don't continue if device has no _ADR method.
 	 * _GTF is intended for known motherboard devices. */
-	if (!(ap->cbl == ATA_CBL_SATA)) {
+	if (!(ap->flags & ATA_FLAG_ACPI_SATA)) {
 		err = pata_get_dev_handle(dev, &dev_handle, &pcidevfn);
 		if (err < 0) {
 			if (ata_msg_probe(ap))
@@ -343,7 +343,7 @@ static int do_drive_get_GTF(struct ata_device *adev, unsigned int *gtf_length,
 
 	/* Get this drive's _ADR info. if not already known. */
 	if (!adev->obj_handle) {
-		if (!(ap->cbl == ATA_CBL_SATA)) {
+		if (!(ap->flags & ATA_FLAG_ACPI_SATA)) {
 			/* get child objects of dev_handle == channel objects,
 	 		 * + _their_ children == drive objects */
 			/* channel is ap->port_no */
@@ -528,7 +528,7 @@ static int do_drive_set_taskfiles(struct ata_device *adev,
 		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
 			       __FUNCTION__, ap->port_no);
 
-	if (libata_noacpi || !(ap->cbl == ATA_CBL_SATA))
+	if (libata_noacpi || !(ap->flags & ATA_FLAG_ACPI_SATA))
 		return 0;
 
 	if (!ata_dev_enabled(adev) || (ap->flags & ATA_FLAG_DISABLED))
@@ -578,7 +578,7 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
 	 * we should not run GTF on PATA devices since some
 	 * PATA require execution of GTM/STM before GTF.
 	 */
-	if (!(ap->cbl == ATA_CBL_SATA))
+	if (!(ap->flags & ATA_FLAG_ACPI_SATA))
 		return 0;
 
 	for (ix = 0; ix < ATA_MAX_DEVICES; ix++) {
@@ -641,7 +641,7 @@ int ata_acpi_push_id(struct ata_device *adev)
 			       __FUNCTION__, adev->devno, ap->port_no);
 
 	/* Don't continue if not a SATA device. */
-	if (!(ap->cbl == ATA_CBL_SATA)) {
+	if (!(ap->flags & ATA_FLAG_ACPI_SATA)) {
 		if (ata_msg_probe(ap))
 			ata_dev_printk(adev, KERN_DEBUG,
 				"%s: Not a SATA device\n", __FUNCTION__);
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index e6223ba..a499369 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -237,7 +237,8 @@ enum {
 	/* host flags */
 	SIL24_COMMON_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_NCQ | ATA_FLAG_SKIP_D2H_BSY,
+				  ATA_FLAG_NCQ | ATA_FLAG_SKIP_D2H_BSY |
+				  ATA_FLAG_ACPI_SATA,
 	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
 
 	IRQ_STAT_4PORTS		= 0xf,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 8a577c2..a26bb7f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -174,6 +174,7 @@ enum {
 	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	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 */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be
-- 
1.5.0.3

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Alan Cox]

On Mon, 23 Apr 2007 02:41:06 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Whether a controller needs IDE or SATA ACPI hierarchy is determined by
> the programming interface of the controller not by whether the
> controller is SATA or PATA

NAK

I keep trying to point out that this is not true.

The ACPI interface to use can only be safely determined one way - and
that is to see what methods the BIOS has attached to the device and use
those.

Take the ACPI handle, go look for _GTF, _SDD etc and believe the
firmware. Nothing else works.

Alan

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Tejun Heo]

Alan Cox wrote:
> On Mon, 23 Apr 2007 02:41:06 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> Whether a controller needs IDE or SATA ACPI hierarchy is determined by
>> the programming interface of the controller not by whether the
>> controller is SATA or PATA
> 
> NAK
> 
> I keep trying to point out that this is not true.
> 
> The ACPI interface to use can only be safely determined one way - and
> that is to see what methods the BIOS has attached to the device and use
> those.
> 
> Take the ACPI handle, go look for _GTF, _SDD etc and believe the
> firmware. Nothing else works.

Actually, that's dangerous.  For example, you must not do _STM/_GTM on
ahci becuase _STM/_GTM access PCI config registers which must not be
accessed in achi modes and some BIOSen supply the same _STM/_GTM nodes
whether the controller is in ata_piix mode or ahci mode.  Also, on ICH8,
the association gets quite weird due to PCI device splitting.

The ACPI spec says the layout is dependent on controller interface and I
 can see reasons why we need to follow that but not the other way
around.  Do you have counter-examples?

-- 
tejun

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Alan Cox]

> > Take the ACPI handle, go look for _GTF, _SDD etc and believe the
> > firmware. Nothing else works.
> 
> Actually, that's dangerous.  For example, you must not do _STM/_GTM on
> ahci becuase _STM/_GTM access PCI config registers which must not be
> accessed in achi modes and some BIOSen supply the same _STM/_GTM nodes
> whether the controller is in ata_piix mode or ahci mode.  Also, on ICH8,
> the association gets quite weird due to PCI device splitting.

You can end up with _SDD methods for SFF format controllers (or what we
think of as SFF format controllers) when we don't have the knowledge to
drive them in their full super-whizzo method (eg Marvell)

> The ACPI spec says the layout is dependent on controller interface and I
>  can see reasons why we need to follow that but not the other way
> around.  Do you have counter-examples?

The first problem is the words "the spec". 

Agreed that STM/GTM are not safe except when expected (and not always safe
when they are) but that ought to be ok as we then set all the modes
properly ourselves afterwards.

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Tejun Heo]

Alan Cox wrote:
> You can end up with _SDD methods for SFF format controllers (or what we
> think of as SFF format controllers) when we don't have the knowledge to
> drive them in their full super-whizzo method (eg Marvell)
> 
>> The ACPI spec says the layout is dependent on controller interface and I
>>  can see reasons why we need to follow that but not the other way
>> around.  Do you have counter-examples?
> 
> The first problem is the words "the spec". 
> 
> Agreed that STM/GTM are not safe except when expected (and not always safe
> when they are) but that ought to be ok as we then set all the modes
> properly ourselves afterwards.

I agree that there are cases where we drive a controller in different
way than how the BIOS configured it and as a result we can end up with
unmatching ACPI nodes, but I think that it's safer to ignore unmatched
ACPI nodes than to do what seems likely without really knowing what's
going on.

There just isn't a generic way to determine which node matches which
device.  The first two ports might map to the first IDE channel or port
1 and 3 (ata_piix).  Or each port might map to a separate IDE channel
with slave spot disabled.  Even if we support 'best effort' matching, it
is something that only the LLD knows how it should be done and whether
it's safe or not.  IMHO, we're better off just ignoring ATA ACPI
hierarchy if it doesn't match the expectation of the driver.

-- 
tejun

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Alan Cox]

> The ACPI interface to use can only be safely determined one way - and
> that is to see what methods the BIOS has attached to the device and use
> those.
> 
> Take the ACPI handle, go look for _GTF, _SDD etc and believe the
> firmware. Nothing else works.

Actually its even worse a mess than I thought 8(

We have to look at the BIOS method offered and we have to know if we
changed the mode of the controller.

If the BIOS mode of an IDE controller is SFF and we flip it into AHCI
mode then we can no longer use the BIOS provided methods because they are
the wrong ones.

For the sane cases (where we don't frob the device) we can ask the BIOS
what methods it has to guess what to use. If we change the mode of the
controller we need to look for methods that match our change, and be
aware we may have totally hosed the ACPI method support by changing the
chip config.

Bletch, ACPI, its like openprom crossed with european law, a million
times larger than needed, and full of holes.

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Tejun Heo]

Alan Cox wrote:
>> The ACPI interface to use can only be safely determined one way - and
>> that is to see what methods the BIOS has attached to the device and use
>> those.
>>
>> Take the ACPI handle, go look for _GTF, _SDD etc and believe the
>> firmware. Nothing else works.
> 
> Actually its even worse a mess than I thought 8(
> 
> We have to look at the BIOS method offered and we have to know if we
> changed the mode of the controller.
> 
> If the BIOS mode of an IDE controller is SFF and we flip it into AHCI
> mode then we can no longer use the BIOS provided methods because they are
> the wrong ones.
> 
> For the sane cases (where we don't frob the device) we can ask the BIOS
> what methods it has to guess what to use. If we change the mode of the
> controller we need to look for methods that match our change, and be
> aware we may have totally hosed the ACPI method support by changing the
> chip config.
> 
> Bletch, ACPI, its like openprom crossed with european law, a million
> times larger than needed, and full of holes.

Here are what I've seen till now.

* Many older boards only supply IDE style ACPI nodes and don't bother to
switch them off when the controller is put into ahci mode (whether it's
done by the BIOS itself or by the OS).

* Newer ones have both in their SSDTs.  Probably selects one or the
other according to BIOS setting.  I haven't seen them in action and not
too familiar with asl so dunno.

* Laptops which make heavy use of ATA ACPI features usually locks the
controller into single programming mode and supply the matching ACPI
hierarchy.

So, I think we'll be okay if we follow the spec here.

-- 
tejun

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Jeff Garzik]

Tejun Heo wrote:
> Whether a controller needs IDE or SATA ACPI hierarchy is determined by
> the programming interface of the controller not by whether the
> controller is SATA or PATA, or it supports slave device or not.  This
> patch adds ATA_FLAG_ACPI_SATA port flags which tells libata-acpi that
> the port needs SATA ACPI nodes, and sets the flag for ahci and
> sata_sil24.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/ahci.c        |    3 ++-
>  drivers/ata/libata-acpi.c |   10 +++++-----
>  drivers/ata/sata_sil24.c  |    3 ++-
>  include/linux/libata.h    |    1 +
>  4 files changed, 10 insertions(+), 7 deletions(-)

I don't think the situation is as static as a compiled-in driver flag 
implies.  And I'm not really convinced a driver flag is needed, or wanted.

If anything, the only flag we /may/ need could be a 
ATA_FLAG_NEVER_EVER_DO_ACPI_FOR_THIS_CONTROLLER.

	Jeff

Re: [PATCH 07/13] libata-acpi: add ATA_FLAG_ACPI_SATA port flag

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Whether a controller needs IDE or SATA ACPI hierarchy is determined by
>> the programming interface of the controller not by whether the
>> controller is SATA or PATA, or it supports slave device or not.  This
>> patch adds ATA_FLAG_ACPI_SATA port flags which tells libata-acpi that
>> the port needs SATA ACPI nodes, and sets the flag for ahci and
>> sata_sil24.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>>  drivers/ata/ahci.c        |    3 ++-
>>  drivers/ata/libata-acpi.c |   10 +++++-----
>>  drivers/ata/sata_sil24.c  |    3 ++-
>>  include/linux/libata.h    |    1 +
>>  4 files changed, 10 insertions(+), 7 deletions(-)
> 
> I don't think the situation is as static as a compiled-in driver flag
> implies.  And I'm not really convinced a driver flag is needed, or wanted.
> 
> If anything, the only flag we /may/ need could be a
> ATA_FLAG_NEVER_EVER_DO_ACPI_FOR_THIS_CONTROLLER.

Can you please elaborate a bit?  As I wrote while talking with Alan, I
really don't know how to do auto-matching.  Personally, I don't think
there is a way to do that safely but will be happy to implement it if
someone can enlighten me.  :-)

-- 
tejun

[PATCH 05/13] libata-acpi: s/CONFIG_SATA_ACPI/CONFIG_ATA_ACPI/

[Tejun Heo]

ACPI applies to both SATA and PATA.  Drop the 'S' from the config
variable.

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

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 365c306..6ee8f42 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -23,6 +23,19 @@ config ATA_NONSTANDARD
        bool
        default n
 
+config ATA_ACPI
+	bool
+	depends on ACPI && PCI
+	default y
+	help
+	  This option adds support for ATA-related ACPI objects.
+	  These ACPI objects add the ability to retrieve taskfiles
+	  from the ACPI BIOS and write them to the disk controller.
+	  These objects may be related to performance, security,
+	  power management, or other areas.
+	  You can disable this at kernel boot time by using the
+	  option libata.noacpi=1
+
 config SATA_AHCI
 	tristate "AHCI SATA support"
 	depends on PCI
@@ -156,19 +169,6 @@ config SATA_INIC162X
 	help
 	  This option enables support for Initio 162x Serial ATA.
 
-config SATA_ACPI
-	bool
-	depends on ACPI && PCI
-	default y
-	help
-	  This option adds support for SATA-related ACPI objects.
-	  These ACPI objects add the ability to retrieve taskfiles
-	  from the ACPI BIOS and write them to the disk controller.
-	  These objects may be related to performance, security,
-	  power management, or other areas.
-	  You can disable this at kernel boot time by using the
-	  option libata.noacpi=1
-
 config PATA_ALI
 	tristate "ALi PATA support (Experimental)"
 	depends on PCI && EXPERIMENTAL
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b7055e3..f9e53c1 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -68,4 +68,4 @@ obj-$(CONFIG_ATA_GENERIC)	+= ata_generic.o
 obj-$(CONFIG_PATA_LEGACY)	+= pata_legacy.o
 
 libata-objs	:= libata-core.o libata-scsi.o libata-sff.o libata-eh.o
-libata-$(CONFIG_SATA_ACPI) += libata-acpi.o
+libata-$(CONFIG_ATA_ACPI)	+= libata-acpi.o
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index c40665a..122bc16 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -97,7 +97,7 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern struct ata_port *ata_port_alloc(struct ata_host *host);
 
 /* libata-acpi.c */
-#ifdef CONFIG_SATA_ACPI
+#ifdef CONFIG_ATA_ACPI
 extern int ata_acpi_exec_tfs(struct ata_port *ap);
 extern int ata_acpi_push_id(struct ata_port *ap, unsigned int ix);
 #else
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3cffbf6..8a577c2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -474,7 +474,7 @@ struct ata_device {
 	struct ata_ering	ering;
 	int			spdn_cnt;
 	unsigned int		horkage;	/* List of broken features */
-#ifdef CONFIG_SATA_ACPI
+#ifdef CONFIG_ATA_ACPI
 	/* ACPI objects info */
 	acpi_handle obj_handle;
 #endif
-- 
1.5.0.3

Re: [PATCH 05/13] libata-acpi: s/CONFIG_SATA_ACPI/CONFIG_ATA_ACPI/

[Jeff Garzik]

Tejun Heo wrote:
> ACPI applies to both SATA and PATA.  Drop the 'S' from the config
> variable.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/Kconfig    |   26 +++++++++++++-------------
>  drivers/ata/Makefile   |    2 +-
>  drivers/ata/libata.h   |    2 +-
>  include/linux/libata.h |    2 +-
>  4 files changed, 16 insertions(+), 16 deletions(-)

ACK patches 4-5

[PATCH 11/13] libata: reimplement ACPI invocation

[Tejun Heo]

This patch reimplements ACPI invocation such that, instead of
exporting ACPI details to the rest of libata, ACPI event handlers -
ata_acpi_on_resume() and ata_acpi_on_devcfg() - are used.  These two
functions are responsible for determining whether specific ACPI method
is used and when.

On resume, _GTF is scheduled by setting ATA_DFLAG_ACPI_PENDING device
flag.  This is done this way to avoid performing the action on wrong
device device (device swapping while suspended).

On every ata_dev_configure(), ata_acpi_on_devcfg() is called, which
performs _SDD and _GTF.  _GTF is performed only after resuming and, if
SATA, hardreset as the ACPI spec specifies.  As _GTF may contain
arbitrary commands, IDENTIFY page is re-read after _GTF taskfiles are
executed.

If one of ACPI methods fails, ata_acpi_on_devcfg() retries on the
first failure.  If it fails again on the second try, ACPI is disabled
on the device.  Note that successful configuration clears ACPI failed
status.

With all feature checks moved to the above two functions,
do_drive_set_taskfiles() is trivial and thus collapsed into
ata_acpi_exec_tfs(), which is now static and converted to return the
number of executed taskfiles to be used by ata_acpi_on_resume().  As
failures are handled properly, ata_acpi_push_id() now returns -errno
on errors instead of unconditional zero.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |  204 +++++++++++++++++++++++++++-----------------
 drivers/ata/libata-core.c |   16 ++--
 drivers/ata/libata-eh.c   |    3 +
 drivers/ata/libata.h      |   14 +--
 include/linux/libata.h    |    2 +
 5 files changed, 140 insertions(+), 99 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 2750c36..63b77b9 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -272,91 +272,47 @@ static int taskfile_load_raw(struct ata_device *adev,
 }
 
 /**
- * do_drive_set_taskfiles - write the drive taskfile settings from _GTF
- * @adev: target ATA device
- * @gtf: pointer to array of _GTF taskfiles to execute
- * @gtf_count: number of taskfiles
- *
- * This applies to both PATA and SATA drives.
- *
- * Execute taskfiles in @gtf.
- *
- * LOCKING:
- * EH context.
- *
- * RETURNS:
- * 0 on success, -errno on failure.
- */
-static int do_drive_set_taskfiles(struct ata_device *adev,
-				  struct ata_acpi_gtf *gtf, int gtf_count)
-{
-	struct ata_port *ap = adev->ap;
-	int ix;
-
-	if (ata_msg_probe(ap))
-		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
-			       __FUNCTION__, ap->port_no);
-
-	if (!(ap->flags & ATA_FLAG_ACPI_SATA))
-		return 0;
-
-	if (!ata_dev_enabled(adev) || (ap->flags & ATA_FLAG_DISABLED))
-		return -ENODEV;
-
-	/* send all TaskFile registers (0x1f1-0x1f7) *in*that*order* */
-	for (ix = 0; ix < gtf_count; ix++)
-		 taskfile_load_raw(adev, gtf++);
-
-	return 0;
-}
-
-/**
  * ata_acpi_exec_tfs - get then write drive taskfile settings
- * @ap: the ata_port for the drive
+ * @adev: target ATA device
  *
- * This applies to both PATA and SATA drives.
+ * Evaluate _GTF and excute returned taskfiles.
  *
  * LOCKING:
  * EH context.
  *
  * RETURNS:
- * 0 on success, -errno on failure.
+ * Number of executed taskfiles on success, 0 if _GTF doesn't exist or
+ * doesn't contain valid data.  -errno on other errors.
  */
-int ata_acpi_exec_tfs(struct ata_port *ap)
+static int ata_acpi_exec_tfs(struct ata_device *adev)
 {
-	int ix, ret = 0;
-
-	/*
-	 * TBD - implement PATA support.  For now,
-	 * we should not run GTF on PATA devices since some
-	 * PATA require execution of GTM/STM before GTF.
-	 */
-	if (!(ap->flags & ATA_FLAG_ACPI_SATA))
-		return 0;
-
-	for (ix = 0; ix < ATA_MAX_DEVICES; ix++) {
-		struct ata_device *adev = &ap->device[ix];
-		struct ata_acpi_gtf *gtf = NULL;
-		int gtf_count;
-		void *ptr_to_free = NULL;
-
-		if (!ata_dev_enabled(adev))
-			continue;
-
-		ret = do_drive_get_GTF(adev, &gtf, &ptr_to_free);
-		if (ret == 0)
-			continue;
-		if (ret < 0)
-			break;
-		gtf_count = ret;
-
-		ret = do_drive_set_taskfiles(adev, gtf, gtf_count);
-		kfree(ptr_to_free);
-		if (ret < 0)
-			break;
+	struct ata_acpi_gtf *gtf = NULL;
+	void *ptr_to_free = NULL;
+	int gtf_count, i, rc;
+
+	/* get taskfiles */
+	rc = do_drive_get_GTF(adev, &gtf, &ptr_to_free);
+	if (rc < 0)
+		return rc;
+	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(adev, gtf++);
+		if (!rc)
+			rc = tmp;
 	}
 
-	return ret;
+	kfree(ptr_to_free);
+
+	if (rc == 0)
+		return gtf_count;
+	return rc;
 }
 
 /**
@@ -375,7 +331,7 @@ int ata_acpi_exec_tfs(struct ata_port *ap)
  * RETURNS:
  * 0 on success, -errno on failure.
  */
-int ata_acpi_push_id(struct ata_device *adev)
+static int ata_acpi_push_id(struct ata_device *adev)
 {
 	struct ata_port *ap = adev->ap;
 	int err;
@@ -395,7 +351,7 @@ int ata_acpi_push_id(struct ata_device *adev)
 		if (ata_msg_probe(ap))
 			ata_dev_printk(adev, KERN_DEBUG,
 				"%s: Not a SATA device\n", __FUNCTION__);
-		goto out;
+		return 0;
 	}
 
 	/* Give the drive Identify data to the drive via the _SDD method */
@@ -417,9 +373,99 @@ int ata_acpi_push_id(struct ata_device *adev)
 		ata_dev_printk(adev, KERN_WARNING,
 			       "ACPI _SDD failed (AE 0x%x)\n", status);
 
-	/* always return success */
-out:
-	return 0;
+	return err;
+}
+
+/**
+ * ata_acpi_on_resume - ATA ACPI hook called on resume
+ * @ap: target ATA port
+ *
+ * This function is called when @ap is resumed - right after port
+ * itself is resumed but before any EH action is taken.
+ *
+ * LOCKING:
+ * EH context.
+ */
+void ata_acpi_on_resume(struct ata_port *ap)
+{
+	int i;
+
+	/* schedule _GTF */
+	for (i = 0; i < ATA_MAX_DEVICES; i++)
+		ap->device[i].flags |= ATA_DFLAG_ACPI_PENDING;
 }
 
+/**
+ * ata_acpi_on_devcfg - ATA ACPI hook called on device donfiguration
+ * @adev: target ATA device
+ *
+ * This function is called when @adev is about to be configured.
+ * IDENTIFY data might have been modified after this hook is run.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * Positive number if IDENTIFY data needs to be refreshed, 0 if not,
+ * -errno on failure.
+ */
+int ata_acpi_on_devcfg(struct ata_device *adev)
+{
+	struct ata_port *ap = adev->ap;
+	struct ata_eh_context *ehc = &ap->eh_context;
+	int acpi_sata = ap->flags & ATA_FLAG_ACPI_SATA;
+	int rc;
+
+	/* XXX: _STM isn't implemented yet, skip if IDE for now */
+	if (!acpi_sata)
+		return 0;
 
+	if (!adev->acpi_handle)
+		return 0;
+
+	/* do we need to do _GTF? */
+	if (!(adev->flags & ATA_DFLAG_ACPI_PENDING) &&
+	    !(acpi_sata && (ehc->i.flags & ATA_EHI_DID_HARDRESET)))
+		return 0;
+
+	/* do _SDD if SATA */
+	if (acpi_sata) {
+		rc = ata_acpi_push_id(adev);
+		if (rc)
+			goto acpi_err;
+	}
+
+	/* do _GTF */
+	rc = ata_acpi_exec_tfs(adev);
+	if (rc < 0)
+		goto acpi_err;
+
+	adev->flags &= ~ATA_DFLAG_ACPI_PENDING;
+
+	/* refresh IDENTIFY page if any _GTF command has been executed */
+	if (rc > 0) {
+		rc = ata_dev_reread_id(adev, 0);
+		if (rc < 0) {
+			ata_dev_printk(adev, KERN_ERR, "failed to IDENTIFY "
+				       "after ACPI commands\n");
+			return rc;
+		}
+	}
+
+	return 0;
+
+ acpi_err:
+	/* let EH retry on the first failure, disable ACPI on the second */
+	if (adev->flags & ATA_DFLAG_ACPI_FAILED) {
+		ata_dev_printk(adev, KERN_WARNING, "ACPI on devcfg failed the "
+			       "second time, disabling (errno=%d)\n", rc);
+
+		adev->acpi_handle = NULL;
+
+		/* if port is working, request IDENTIFY reload and continue */
+		if (!(ap->pflags & ATA_PFLAG_FROZEN))
+			rc = 1;
+	}
+	adev->flags |= ATA_DFLAG_ACPI_FAILED;
+	return rc;
+}
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5ff8a38..eed9f69 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1794,7 +1794,8 @@ static void ata_dev_config_ncq(struct ata_device *dev,
 int ata_dev_configure(struct ata_device *dev)
 {
 	struct ata_port *ap = dev->ap;
-	int print_info = ap->eh_context.i.flags & ATA_EHI_PRINTINFO;
+	struct ata_eh_context *ehc = &ap->eh_context;
+	int print_info = ehc->i.flags & ATA_EHI_PRINTINFO;
 	const u16 *id = dev->id;
 	unsigned int xfer_mask;
 	char revbuf[7];		/* XYZ-99\0 */
@@ -1811,15 +1812,10 @@ int ata_dev_configure(struct ata_device *dev)
 	if (ata_msg_probe(ap))
 		ata_dev_printk(dev, KERN_DEBUG, "%s: ENTER\n", __FUNCTION__);
 
-	/* set _SDD */
-	rc = ata_acpi_push_id(dev);
-	if (rc) {
-		ata_dev_printk(dev, KERN_WARNING, "failed to set _SDD(%d)\n",
-			rc);
-	}
-
-	/* retrieve and execute the ATA task file of _GTF */
-	ata_acpi_exec_tfs(ap);
+	/* let ACPI work its magic */
+	rc = ata_acpi_on_devcfg(dev);
+	if (rc)
+		return rc;
 
 	/* print device capabilities */
 	if (ata_msg_probe(ap))
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 2bff9ad..b68673b 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2399,6 +2399,9 @@ static void ata_eh_handle_port_resume(struct ata_port *ap)
 	if (ap->ops->port_resume)
 		rc = ap->ops->port_resume(ap);
 
+	/* tell ACPI that we're resuming */
+	ata_acpi_on_resume(ap);
+
 	/* give devices time to request EH */
 	timeout = jiffies + HZ; /* 1s max */
 	while (1) {
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index b91317e..bee7cbc 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -99,18 +99,12 @@ extern struct ata_port *ata_port_alloc(struct ata_host *host);
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
 extern void ata_acpi_associate(struct ata_host *host);
-extern int ata_acpi_exec_tfs(struct ata_port *ap);
-extern int ata_acpi_push_id(struct ata_device *adev);
+extern void ata_acpi_on_resume(struct ata_port *ap);
+extern int ata_acpi_on_devcfg(struct ata_device *adev);
 #else
 static inline void ata_acpi_associate(struct ata_host *host) { }
-static inline int ata_acpi_exec_tfs(struct ata_port *ap)
-{
-	return 0;
-}
-static inline int ata_acpi_push_id(struct ata_device *adev)
-{
-	return 0;
-}
+static inline void ata_acpi_on_resume(struct ata_port *ap) { }
+static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
 #endif
 
 /* libata-scsi.c */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9a517e6..e43edc9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -136,6 +136,8 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_ACPI_PENDING	= (1 << 5), /* ACPI resume action pending */
+	ATA_DFLAG_ACPI_FAILED	= (1 << 6), /* ACPI on devcfg has failed */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
-- 
1.5.0.3

Re: [PATCH 11/13] libata: reimplement ACPI invocation

[Jeff Garzik]

patches 11-13 seem sane

My general feeling is
* Alan's comments to most of the patches seemed to be on target
* I hate ACPI, and am terribly pleased you are working on this.  Thanks. 
  You will find you have much latitude in this area.

[PATCH 13/13] libata-acpi: implement _GTM/_STM support

[Tejun Heo]

Implement _GTM/_STM support.  acpi_gtm is added to ata_port which
stores _GTM parameters over suspend/resume cycle.  A new hook
ata_acpi_on_suspend() is responsible for storing _GTM parameters
during suspend.  _STM is executed in ata_acpi_on_resume().  With this
change, invoking _GTF is safe on IDE hierarchy and acpi_sata check
before _GTF is removed.

ata_acpi_gtm() and ata_acpi_stm() implementation is taken from Alan
Cox's pata_acpi implementation.  ata_acpi_gtm() is fixed such that the
result parameter is not shifted by sizeof(union acpi_object).

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/libata-acpi.c |  153 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/ata/libata-eh.c   |    8 ++-
 drivers/ata/libata.h      |    2 +
 include/linux/libata.h    |   13 ++++
 4 files changed, 171 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 0ba0e79..8403c50 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -101,6 +101,108 @@ void ata_acpi_associate(struct ata_host *host)
 }
 
 /**
+ * ata_acpi_gtm - execute _GTM
+ * @ap: target ATA port
+ * @gtm: out parameter for _GTM result
+ *
+ * Evaluate _GTM and store the result in @gtm.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -ENOENT if _GTM doesn't exist, -errno on failure.
+ */
+static int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *gtm)
+{
+	struct acpi_buffer output = { .length = ACPI_ALLOCATE_BUFFER };
+	union acpi_object *out_obj;
+	acpi_status status;
+	int rc = 0;
+
+	status = acpi_evaluate_object(ap->acpi_handle, "_GTM", NULL, &output);
+
+	rc = -ENOENT;
+	if (status == AE_NOT_FOUND)
+		goto out_free;
+
+	rc = -EINVAL;
+	if (ACPI_FAILURE(status)) {
+		ata_port_printk(ap, KERN_ERR,
+				"ACPI get timing mode failed (AE 0x%x)\n",
+				status);
+		goto out_free;
+	}
+
+	out_obj = output.pointer;
+	if (out_obj->type != ACPI_TYPE_BUFFER) {
+		ata_port_printk(ap, KERN_WARNING,
+				"_GTM returned unexpected object type 0x%x\n",
+				out_obj->type);
+
+		goto out_free;
+	}
+
+	if (out_obj->buffer.length != sizeof(struct ata_acpi_gtm)) {
+		ata_port_printk(ap, KERN_ERR,
+				"_GTM returned invalid length %d\n",
+				out_obj->buffer.length);
+		goto out_free;
+	}
+
+	memcpy(gtm, out_obj->buffer.pointer, sizeof(struct ata_acpi_gtm));
+	rc = 0;
+ out_free:
+	kfree(output.pointer);
+	return rc;
+}
+
+/**
+ * ata_acpi_stm - execute _STM
+ * @ap: target ATA port
+ * @stm: timing parameter to _STM
+ *
+ * Evaluate _STM with timing parameter @stm.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -ENOENT if _STM doesn't exist, -errno on failure.
+ */
+static int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm)
+{
+	acpi_status status;
+	struct acpi_object_list         input;
+	union acpi_object               in_params[3];
+
+	in_params[0].type = ACPI_TYPE_BUFFER;
+	in_params[0].buffer.length = sizeof(struct ata_acpi_gtm);
+	in_params[0].buffer.pointer = (u8 *)stm;
+	/* Buffers for id may need byteswapping ? */
+	in_params[1].type = ACPI_TYPE_BUFFER;
+	in_params[1].buffer.length = 512;
+	in_params[1].buffer.pointer = (u8 *)ap->device[0].id;
+	in_params[2].type = ACPI_TYPE_BUFFER;
+	in_params[2].buffer.length = 512;
+	in_params[2].buffer.pointer = (u8 *)ap->device[1].id;
+
+	input.count = 3;
+	input.pointer = in_params;
+
+	status = acpi_evaluate_object(ap->acpi_handle, "_STM", &input, NULL);
+
+	if (status == AE_NOT_FOUND)
+		return -ENOENT;
+	if (ACPI_FAILURE(status)) {
+		ata_port_printk(ap, KERN_ERR,
+			"ACPI set timing mode failed (status=0x%x)\n", status);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/**
  * do_drive_get_GTF - get the drive bootup default taskfile settings
  * @adev: target ATA device
  * @gtf: output parameter for buffer containing _GTF taskfile arrays
@@ -354,6 +456,46 @@ static int ata_acpi_push_id(struct ata_device *adev)
 }
 
 /**
+ * ata_acpi_on_suspend - ATA ACPI hook called on suspend
+ * @ap: target ATA port
+ *
+ * This function is called when @ap is about to be suspended.  All
+ * devices are already put to sleep but the port_suspend() callback
+ * hasn't been executed yet.  Error return from this function aborts
+ * suspend.
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * 0 on success, -errno on failure.
+ */
+int ata_acpi_on_suspend(struct ata_port *ap)
+{
+	unsigned long flags;
+	int rc;
+
+	/* proceed iff per-port acpi_handle is valid */
+	if (!ap->acpi_handle)
+		return 0;
+	BUG_ON(ap->flags & ATA_FLAG_ACPI_SATA);
+
+	/* store timing parameters */
+	rc = ata_acpi_gtm(ap, &ap->acpi_gtm);
+
+	spin_lock_irqsave(ap->lock, flags);
+	if (rc == 0)
+		ap->pflags |= ATA_PFLAG_GTM_VALID;
+	else
+		ap->pflags &= ~ATA_PFLAG_GTM_VALID;
+	spin_unlock_irqrestore(ap->lock, flags);
+
+	if (rc == -ENOENT)
+		rc = 0;
+	return rc;
+}
+
+/**
  * ata_acpi_on_resume - ATA ACPI hook called on resume
  * @ap: target ATA port
  *
@@ -367,6 +509,13 @@ void ata_acpi_on_resume(struct ata_port *ap)
 {
 	int i;
 
+	if (ap->acpi_handle && (ap->pflags & ATA_PFLAG_GTM_VALID)) {
+		BUG_ON(ap->flags & ATA_FLAG_ACPI_SATA);
+
+		/* restore timing parameters */
+		ata_acpi_stm(ap, &ap->acpi_gtm);
+	}
+
 	/* schedule _GTF */
 	for (i = 0; i < ATA_MAX_DEVICES; i++)
 		ap->device[i].flags |= ATA_DFLAG_ACPI_PENDING;
@@ -393,10 +542,6 @@ int ata_acpi_on_devcfg(struct ata_device *adev)
 	int acpi_sata = ap->flags & ATA_FLAG_ACPI_SATA;
 	int rc;
 
-	/* XXX: _STM isn't implemented yet, skip if IDE for now */
-	if (!acpi_sata)
-		return 0;
-
 	if (!adev->acpi_handle)
 		return 0;
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b68673b..25ee022 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2335,19 +2335,25 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
 
 	WARN_ON(ap->pflags & ATA_PFLAG_SUSPENDED);
 
+	/* tell ACPI we're suspending */
+	rc = ata_acpi_on_suspend(ap);
+	if (rc)
+		goto out;
+
 	/* suspend */
 	ata_eh_freeze_port(ap);
 
 	if (ap->ops->port_suspend)
 		rc = ap->ops->port_suspend(ap, ap->pm_mesg);
 
+ out:
 	/* report result */
 	spin_lock_irqsave(ap->lock, flags);
 
 	ap->pflags &= ~ATA_PFLAG_PM_PENDING;
 	if (rc == 0)
 		ap->pflags |= ATA_PFLAG_SUSPENDED;
-	else
+	else if (ap->pflags & ATA_PFLAG_FROZEN)
 		ata_port_schedule_eh(ap);
 
 	if (ap->pm_result) {
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index bee7cbc..ba17fc5 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -99,10 +99,12 @@ extern struct ata_port *ata_port_alloc(struct ata_host *host);
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
 extern void ata_acpi_associate(struct ata_host *host);
+extern int ata_acpi_on_suspend(struct ata_port *ap);
 extern void ata_acpi_on_resume(struct ata_port *ap);
 extern int ata_acpi_on_devcfg(struct ata_device *adev);
 #else
 static inline void ata_acpi_associate(struct ata_host *host) { }
+static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; }
 static inline void ata_acpi_on_resume(struct ata_port *ap) { }
 static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
 #endif
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e43edc9..806108b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -198,6 +198,7 @@ enum {
 	ATA_PFLAG_FLUSH_PORT_TASK = (1 << 16), /* flush port task */
 	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 */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */
@@ -512,6 +513,17 @@ struct ata_eh_context {
 	unsigned int		did_probe_mask;
 };
 
+struct ata_acpi_drive
+{
+	u32 pio;
+	u32 dma;
+} __packed;
+
+struct ata_acpi_gtm {
+	struct ata_acpi_drive drive[2];
+	u32 flags;
+} __packed;
+
 struct ata_port {
 	struct Scsi_Host	*scsi_host; /* our co-allocated scsi host */
 	const struct ata_port_operations *ops;
@@ -574,6 +586,7 @@ struct ata_port {
 
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
+	struct ata_acpi_gtm	acpi_gtm;
 #endif
 	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
 };
-- 
1.5.0.3

[PATCH 12/13] libata-acpi: remove redundant checks

[Tejun Heo]

Remove remaining unnecessary feature and status checks.

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

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 63b77b9..0ba0e79 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -134,22 +134,10 @@ static int do_drive_get_GTF(struct ata_device *adev, struct ata_acpi_gtf **gtf,
 	output.length = ACPI_ALLOCATE_BUFFER;
 	output.pointer = NULL;	/* ACPI-CA sets this; save/free it later */
 
-	if (!adev->acpi_handle)
-		goto out_free;
-
 	if (ata_msg_probe(ap))
 		ata_dev_printk(adev, KERN_DEBUG, "%s: ENTER: port#: %d\n",
 			       __FUNCTION__, ap->port_no);
 
-	if (!ata_dev_enabled(adev) || (ap->flags & ATA_FLAG_DISABLED)) {
-		if (ata_msg_probe(ap))
-			ata_dev_printk(adev, KERN_DEBUG, "%s: ERR: "
-				"ata_dev_present: %d, PORT_DISABLED: %lu\n",
-				__FUNCTION__, ata_dev_enabled(adev),
-				ap->flags & ATA_FLAG_DISABLED);
-		goto out_free;
-	}
-
 	/* _GTF has no input parameters */
 	status = acpi_evaluate_object(adev->acpi_handle, "_GTF", NULL, &output);
 	if (ACPI_FAILURE(status)) {
@@ -339,21 +327,10 @@ static int ata_acpi_push_id(struct ata_device *adev)
 	struct acpi_object_list input;
 	union acpi_object in_params[1];
 
-	if (!adev->acpi_handle)
-		return 0;
-
 	if (ata_msg_probe(ap))
 		ata_dev_printk(adev, KERN_DEBUG, "%s: ix = %d, port#: %d\n",
 			       __FUNCTION__, adev->devno, ap->port_no);
 
-	/* Don't continue if not a SATA device. */
-	if (!(ap->flags & ATA_FLAG_ACPI_SATA)) {
-		if (ata_msg_probe(ap))
-			ata_dev_printk(adev, KERN_DEBUG,
-				"%s: Not a SATA device\n", __FUNCTION__);
-		return 0;
-	}
-
 	/* Give the drive Identify data to the drive via the _SDD method */
 	/* _SDD: set up input parameters */
 	input.count = 1;
-- 
1.5.0.3

Re: [PATCHSET] libata: improve ATA ACPI support

[Alan Cox]

> * after successfully executing _GTF taskfiles, IDENTIFY page is
>   reloaded

Interesting question we should sort out: What is our identify page as
supplied to the user meant to be ?

The old IDE one started off as the "identify data at boot" (which is
useful) and mutated through a million "kind of boot but mangled" and "not
boot" versions of the identify data, all of which are useless to
userspace.

Having the "boot" data (for some definition of boot) is important in
order to know things like the BIOS view of the disk geometry and HPA (eg
for partitioning), having the current data is useless as its already
available via SG_IO.

Alan

Re: [PATCHSET] libata: improve ATA ACPI support

[Tejun Heo]

Alan Cox wrote:
>> * after successfully executing _GTF taskfiles, IDENTIFY page is
>>   reloaded
> 
> Interesting question we should sort out: What is our identify page as
> supplied to the user meant to be ?
> 
> The old IDE one started off as the "identify data at boot" (which is
> useful) and mutated through a million "kind of boot but mangled" and "not
> boot" versions of the identify data, all of which are useless to
> userspace.
> 
> Having the "boot" data (for some definition of boot) is important in
> order to know things like the BIOS view of the disk geometry and HPA (eg
> for partitioning), having the current data is useless as its already
> available via SG_IO.

AFAIK, IDENTIFY using SG_IO is the way to obtain ID data.
HDIO_GET_IDENTITY is only added for backward compatibility and as a
sideway to obtain ATAPI IDENTIFY page while IDENTIFY_PACKET_DEVICE via
SG_IO is broken.  libata tries pretty hard to keep the cached IDENTIFY
page up-to-date and I'm pretty sure it stays in sync with the drive most
of the time.  I think HDIO_GET_IDENTITY's meaning is something like
"give me IDENTIFY page of the device as seen by the driver" and doesn't
really matter as long as it can be used to get general idea about the
device.

-- 
tejun

Re: [PATCHSET] libata: improve ATA ACPI support

[Mark Lord]

Tejun Heo wrote:
> 
> of the time.  I think HDIO_GET_IDENTITY's meaning is something like
> "give me IDENTIFY page of the device as seen by the driver" and doesn't
> really matter as long as it can be used to get general idea about the
> device.

The correct/original meaning of that ioctl was "boot time IDENTIFY" data.
As Alan said, it later got bastardized by various people at some point.

Cheers

Re: [PATCHSET] libata: improve ATA ACPI support

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday 24 April 2007, Mark Lord wrote:
> Tejun Heo wrote:
> > 
> > of the time.  I think HDIO_GET_IDENTITY's meaning is something like
> > "give me IDENTIFY page of the device as seen by the driver" and doesn't
> > really matter as long as it can be used to get general idea about the
> > device.
> 
> The correct/original meaning of that ioctl was "boot time IDENTIFY" data.
> As Alan said, it later got bastardized by various people at some point.

It got de-bastardized later :) and currently HDIO_GET_IDENTITY seems to be
returning "boot time IDENTIFY" with only two exceptions:

* if ID can't be read et all - fake geometry with user supplied values
  to make things work - this is OK (was added by Alan IIRC)

* id->dma_{1word,mword,ultra} (currently selected DMA transfer modes)
  are updated when the current transfer mode changes - this is of least
  importance than capacity/geometry data (since nowadays all speed tuning
  should be done by kernel anyway) and easy to fix once DMA rewrite patches
  from my tree get merged

Thanks,
Bart

Re: [PATCHSET] libata: improve ATA ACPI support

[Mark Lord]

Alan Cox wrote:
>> * after successfully executing _GTF taskfiles, IDENTIFY page is
>>   reloaded
> 
> Interesting question we should sort out: What is our identify page as
> supplied to the user meant to be ?
> 
> The old IDE one started off as the "identify data at boot" (which is
> useful) and mutated through a million "kind of boot but mangled" and "not
> boot" versions of the identify data, all of which are useless to
> userspace.
> 
> Having the "boot" data (for some definition of boot) is important in
> order to know things like the BIOS view of the disk geometry and HPA (eg
> for partitioning), having the current data is useless as its already
> available via SG_IO.

Agreed.  Userspace can already fetch the real, current IDENTIFY data
just by issuing the appropriate drive command (except for ATAPI,
since Jeff has gone mute on the months old patch to fix it).

Boot-time IDENTIFY data was extremely useful for IDE development.
It showed how the BIOS had set things up (or not).

Cheers