linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] ACPI / bind: Simplify child devices lookup
@ 2013-11-25  0:09 Rafael J. Wysocki
  2013-11-25  0:10 ` [PATCH 1/4] ACPI / bind: Simplify child device lookups Rafael J. Wysocki
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-25  0:09 UTC (permalink / raw)
  To: ACPI Devel Maling List; +Cc: LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

Hi,

The following series of four patches (on top of current linux-pm.git/bleeding-edge)
rework child device lookup in drivers/acpi/glue.c and related things:

[1/4] ACPI / bind: Simplify child device lookup
[2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
[3/4] ACPI / bind: Redefine acpi_get_child()
[4/4] ACPI / bind: Redefine acpi_preset_companion()

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] ACPI / bind: Simplify child device lookups
  2013-11-25  0:09 [PATCH 0/4] ACPI / bind: Simplify child devices lookup Rafael J. Wysocki
@ 2013-11-25  0:10 ` Rafael J. Wysocki
  2013-11-25  0:12 ` [PATCH 2/4] PCI / ACPI: Use acpi_find_child_device() for child devices lookup Rafael J. Wysocki
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-25  0:10 UTC (permalink / raw)
  To: ACPI Devel Maling List; +Cc: LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Now that we create a struct acpi_device object for every ACPI
namespace node representing a device, it is not necessary to
use acpi_walk_namespace() for child device lookup in
acpi_find_child() any more.  Instead, we can simply walk the
list of children of the given struct acpi_device object and
return the matching one (or the one which is the best match if
there are more of them).  The checks done during the matching
loop can be simplified too so that the secondary namespace walks
in find_child_checks() are not necessary any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c     |  134 ++++++++++++++++++------------------------------
 include/acpi/acpi_bus.h |    3 +
 2 files changed, 56 insertions(+), 81 deletions(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -82,107 +82,79 @@ static struct acpi_bus_type *acpi_get_bu
 #define FIND_CHILD_MIN_SCORE	1
 #define FIND_CHILD_MAX_SCORE	2
 
-static acpi_status acpi_dev_present(acpi_handle handle, u32 lvl_not_used,
-				  void *not_used, void **ret_p)
-{
-	struct acpi_device *adev = NULL;
-
-	acpi_bus_get_device(handle, &adev);
-	if (adev) {
-		*ret_p = handle;
-		return AE_CTRL_TERMINATE;
-	}
-	return AE_OK;
-}
-
-static int do_find_child_checks(acpi_handle handle, bool is_bridge)
+static int find_child_checks(struct acpi_device *adev, bool check_children)
 {
 	bool sta_present = true;
 	unsigned long long sta;
 	acpi_status status;
 
-	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+	status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);
 	if (status == AE_NOT_FOUND)
 		sta_present = false;
 	else if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_ENABLED))
 		return -ENODEV;
 
-	if (is_bridge) {
-		void *test = NULL;
+	if (check_children && list_empty(&adev->children))
+		return -ENODEV;
 
-		/* Check if this object has at least one child device. */
-		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
-				    acpi_dev_present, NULL, NULL, &test);
-		if (!test)
-			return -ENODEV;
-	}
 	return sta_present ? FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
 }
 
-struct find_child_context {
-	u64 addr;
-	bool is_bridge;
-	acpi_handle ret;
-	int ret_score;
-};
-
-static acpi_status do_find_child(acpi_handle handle, u32 lvl_not_used,
-				 void *data, void **not_used)
+struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
+					   u64 address, bool check_children)
 {
-	struct find_child_context *context = data;
-	unsigned long long addr;
-	acpi_status status;
-	int score;
+	struct acpi_device *adev, *ret = NULL;
+	int ret_score = 0;
 
-	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &addr);
-	if (ACPI_FAILURE(status) || addr != context->addr)
-		return AE_OK;
-
-	if (!context->ret) {
-		/* This is the first matching object.  Save its handle. */
-		context->ret = handle;
-		return AE_OK;
-	}
-	/*
-	 * There is more than one matching object with the same _ADR value.
-	 * That really is unexpected, so we are kind of beyond the scope of the
-	 * spec here.  We have to choose which one to return, though.
-	 *
-	 * First, check if the previously found object is good enough and return
-	 * its handle if so.  Second, check the same for the object that we've
-	 * just found.
-	 */
-	if (!context->ret_score) {
-		score = do_find_child_checks(context->ret, context->is_bridge);
-		if (score == FIND_CHILD_MAX_SCORE)
-			return AE_CTRL_TERMINATE;
-		else
-			context->ret_score = score;
-	}
-	score = do_find_child_checks(handle, context->is_bridge);
-	if (score == FIND_CHILD_MAX_SCORE) {
-		context->ret = handle;
-		return AE_CTRL_TERMINATE;
-	} else if (score > context->ret_score) {
-		context->ret = handle;
-		context->ret_score = score;
+	list_for_each_entry(adev, &parent->children, node) {
+		unsigned long long addr;
+		acpi_status status;
+		int score;
+
+		status = acpi_evaluate_integer(adev->handle, METHOD_NAME__ADR,
+					       NULL, &addr);
+		if (ACPI_FAILURE(status) || addr != address)
+			continue;
+
+		if (!ret) {
+			/* This is the first matching object.  Save it. */
+			ret = adev;
+			continue;
+		}
+		/*
+		 * There is more than one matching device object with the same
+		 * _ADR value.  That really is unexpected, so we are kind of
+		 * beyond the scope of the spec here.  We have to choose which
+		 * one to return, though.
+		 *
+		 * First, check if the previously found object is good enough
+		 * and return it if so.  Second, do the same for the object that
+		 * we've just found.
+		 */
+		if (!ret_score) {
+			ret_score = find_child_checks(ret, check_children);
+			if (ret_score == FIND_CHILD_MAX_SCORE)
+				return ret;
+		}
+		score = find_child_checks(adev, check_children);
+		if (score == FIND_CHILD_MAX_SCORE) {
+			return adev;
+		} else if (score > ret_score) {
+			ret = adev;
+			ret_score = score;
+		}
 	}
-	return AE_OK;
+	return ret;
 }
 
-acpi_handle acpi_find_child(acpi_handle parent, u64 addr, bool is_bridge)
+acpi_handle acpi_find_child(acpi_handle handle, u64 addr, bool is_bridge)
 {
-	if (parent) {
-		struct find_child_context context = {
-			.addr = addr,
-			.is_bridge = is_bridge,
-		};
-
-		acpi_walk_namespace(ACPI_TYPE_DEVICE, parent, 1, do_find_child,
-				    NULL, &context, NULL);
-		return context.ret;
-	}
-	return NULL;
+	struct acpi_device *parent;
+
+	if (!handle || acpi_bus_get_device(handle, &parent))
+		return NULL;
+
+	return acpi_find_child_device(parent, addr, is_bridge);
 }
 EXPORT_SYMBOL_GPL(acpi_find_child);
 
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -433,6 +433,9 @@ struct acpi_pci_root {
 };
 
 /* helper */
+
+struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
+					   u64 address, bool check_children);
 acpi_handle acpi_find_child(acpi_handle, u64, bool);
 static inline acpi_handle acpi_get_child(acpi_handle handle, u64 addr)
 {


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/4] PCI / ACPI: Use acpi_find_child_device() for child devices lookup
  2013-11-25  0:09 [PATCH 0/4] ACPI / bind: Simplify child devices lookup Rafael J. Wysocki
  2013-11-25  0:10 ` [PATCH 1/4] ACPI / bind: Simplify child device lookups Rafael J. Wysocki
@ 2013-11-25  0:12 ` Rafael J. Wysocki
  2013-11-25  0:12 ` [PATCH 3/4] ACPI / bind: Redefine acpi_get_child() Rafael J. Wysocki
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-25  0:12 UTC (permalink / raw)
  To: ACPI Devel Maling List; +Cc: LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is much more efficient to use acpi_find_child_device()
for child devices lookup in acpi_pci_find_device() and pass
ACPI_COMPANION(dev->parent) to it directly instead of obtaining
ACPI_HANDLE() of ACPI_COMPANION(dev->parent) and passing it to
acpi_find_child() which has to run acpi_bus_get_device() to
obtain ACPI_COMPANION(dev->parent) from that again.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-acpi.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -309,7 +309,8 @@ void acpi_pci_remove_bus(struct pci_bus
 static int acpi_pci_find_device(struct device *dev, acpi_handle *handle)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	bool is_bridge;
+	struct acpi_device *adev;
+	bool check_children;
 	u64 addr;
 
 	/*
@@ -317,14 +318,17 @@ static int acpi_pci_find_device(struct d
 	 * is set only after acpi_pci_find_device() has been called for the
 	 * given device.
 	 */
-	is_bridge = pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE
+	check_children = pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE
 			|| pci_dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
 	/* Please ref to ACPI spec for the syntax of _ADR */
 	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
-	*handle = acpi_find_child(ACPI_HANDLE(dev->parent), addr, is_bridge);
-	if (!*handle)
-		return -ENODEV;
-	return 0;
+	adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,
+				      check_children);
+	if (adev) {
+		*handle = adev->handle;
+		return 0;
+	}
+	return -ENODEV;
 }
 
 static void pci_acpi_setup(struct device *dev)


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/4] ACPI / bind: Redefine acpi_get_child()
  2013-11-25  0:09 [PATCH 0/4] ACPI / bind: Simplify child devices lookup Rafael J. Wysocki
  2013-11-25  0:10 ` [PATCH 1/4] ACPI / bind: Simplify child device lookups Rafael J. Wysocki
  2013-11-25  0:12 ` [PATCH 2/4] PCI / ACPI: Use acpi_find_child_device() for child devices lookup Rafael J. Wysocki
@ 2013-11-25  0:12 ` Rafael J. Wysocki
  2013-11-25  0:14 ` [PATCH 4/4] ACPI / bind: Redefine acpi_preset_companion() Rafael J. Wysocki
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-25  0:12 UTC (permalink / raw)
  To: ACPI Devel Maling List; +Cc: LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since acpi_get_child() is the only user of acpi_find_child() now,
drop the static inline definition of the former and redefine the
latter as new acpi_get_child().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c     |    6 +++---
 include/acpi/acpi_bus.h |    6 +-----
 2 files changed, 4 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -147,16 +147,16 @@ struct acpi_device *acpi_find_child_devi
 	return ret;
 }
 
-acpi_handle acpi_find_child(acpi_handle handle, u64 addr, bool is_bridge)
+acpi_handle acpi_get_child(acpi_handle handle, u64 addr)
 {
 	struct acpi_device *parent;
 
 	if (!handle || acpi_bus_get_device(handle, &parent))
 		return NULL;
 
-	return acpi_find_child_device(parent, addr, is_bridge);
+	return acpi_find_child_device(parent, addr, false);
 }
-EXPORT_SYMBOL_GPL(acpi_find_child);
+EXPORT_SYMBOL_GPL(acpi_get_child);
 
 static void acpi_physnode_link_name(char *buf, unsigned int node_id)
 {
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -436,11 +436,7 @@ struct acpi_pci_root {
 
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
 					   u64 address, bool check_children);
-acpi_handle acpi_find_child(acpi_handle, u64, bool);
-static inline acpi_handle acpi_get_child(acpi_handle handle, u64 addr)
-{
-	return acpi_find_child(handle, addr, false);
-}
+acpi_handle acpi_get_child(acpi_handle handle, u64 addr);
 void acpi_preset_companion(struct device *dev, acpi_handle parent, u64 addr);
 int acpi_is_root_bridge(acpi_handle);
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/4] ACPI / bind: Redefine acpi_preset_companion()
  2013-11-25  0:09 [PATCH 0/4] ACPI / bind: Simplify child devices lookup Rafael J. Wysocki
                   ` (2 preceding siblings ...)
  2013-11-25  0:12 ` [PATCH 3/4] ACPI / bind: Redefine acpi_get_child() Rafael J. Wysocki
@ 2013-11-25  0:14 ` Rafael J. Wysocki
  2013-11-25  3:17 ` [PATCH 0/4] ACPI / bind: Simplify child devices lookup Aaron Lu
  2013-11-27  0:00 ` Toshi Kani
  5 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-25  0:14 UTC (permalink / raw)
  To: ACPI Devel Maling List; +Cc: LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify acpi_preset_companion() to take a struct acpi_device pointer
instead of an ACPI handle as its second argument and redefine it as
a static inline wrapper around ACPI_COMPANION_SET() passing the
return value of acpi_find_child_device() directly as the second
argument to it.  Update its users to pass struct acpi_device
pointers instead of ACPI handles to it.

This allows some unnecessary acpi_bus_get_device() calls to be
avoided.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c         |   10 +---------
 drivers/ata/libata-acpi.c   |   26 +++++++++++++-------------
 drivers/mmc/core/sdio_bus.c |    2 +-
 include/acpi/acpi_bus.h     |    1 -
 include/linux/acpi.h        |    6 ++++++
 5 files changed, 21 insertions(+), 24 deletions(-)

Index: linux-pm/drivers/ata/libata-acpi.c
===================================================================
--- linux-pm.orig/drivers/ata/libata-acpi.c
+++ linux-pm/drivers/ata/libata-acpi.c
@@ -180,12 +180,12 @@ static const struct acpi_dock_ops ata_ac
 /* bind acpi handle to pata port */
 void ata_acpi_bind_port(struct ata_port *ap)
 {
-	acpi_handle host_handle = ACPI_HANDLE(ap->host->dev);
+	struct acpi_device *host_companion = ACPI_COMPANION(ap->host->dev);
 
-	if (libata_noacpi || ap->flags & ATA_FLAG_ACPI_SATA || !host_handle)
+	if (libata_noacpi || ap->flags & ATA_FLAG_ACPI_SATA || !host_companion)
 		return;
 
-	acpi_preset_companion(&ap->tdev, host_handle, ap->port_no);
+	acpi_preset_companion(&ap->tdev, host_companion, ap->port_no);
 
 	if (ata_acpi_gtm(ap, &ap->__acpi_init_gtm) == 0)
 		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
@@ -198,17 +198,17 @@ void ata_acpi_bind_port(struct ata_port
 void ata_acpi_bind_dev(struct ata_device *dev)
 {
 	struct ata_port *ap = dev->link->ap;
-	acpi_handle port_handle = ACPI_HANDLE(&ap->tdev);
-	acpi_handle host_handle = ACPI_HANDLE(ap->host->dev);
-	acpi_handle parent_handle;
+	struct acpi_device *port_companion = ACPI_COMPANION(&ap->tdev);
+	struct acpi_device *host_companion = ACPI_COMPANION(ap->host->dev);
+	struct acpi_device *parent;
 	u64 adr;
 
 	/*
-	 * For both sata/pata devices, host handle is required.
-	 * For pata device, port handle is also required.
+	 * For both sata/pata devices, host companion device is required.
+	 * For pata device, port companion device is also required.
 	 */
-	if (libata_noacpi || !host_handle ||
-			(!(ap->flags & ATA_FLAG_ACPI_SATA) && !port_handle))
+	if (libata_noacpi || !host_companion ||
+			(!(ap->flags & ATA_FLAG_ACPI_SATA) && !port_companion))
 		return;
 
 	if (ap->flags & ATA_FLAG_ACPI_SATA) {
@@ -216,13 +216,13 @@ void ata_acpi_bind_dev(struct ata_device
 			adr = SATA_ADR(ap->port_no, NO_PORT_MULT);
 		else
 			adr = SATA_ADR(ap->port_no, dev->link->pmp);
-		parent_handle = host_handle;
+		parent = host_companion;
 	} else {
 		adr = dev->devno;
-		parent_handle = port_handle;
+		parent = port_companion;
 	}
 
-	acpi_preset_companion(&dev->tdev, parent_handle, adr);
+	acpi_preset_companion(&dev->tdev, parent, adr);
 
 	register_hotplug_dock_device(ata_dev_acpi_handle(dev),
 				     &ata_acpi_dev_dock_ops, dev, NULL, NULL);
Index: linux-pm/drivers/mmc/core/sdio_bus.c
===================================================================
--- linux-pm.orig/drivers/mmc/core/sdio_bus.c
+++ linux-pm/drivers/mmc/core/sdio_bus.c
@@ -308,7 +308,7 @@ static void sdio_acpi_set_handle(struct
 	struct mmc_host *host = func->card->host;
 	u64 addr = (host->slotno << 16) | func->num;
 
-	acpi_preset_companion(&func->dev, ACPI_HANDLE(host->parent), addr);
+	acpi_preset_companion(&func->dev, ACPI_COMPANION(host->parent), addr);
 }
 #else
 static inline void sdio_acpi_set_handle(struct sdio_func *func) {}
Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -146,6 +146,7 @@ struct acpi_device *acpi_find_child_devi
 	}
 	return ret;
 }
+EXPORT_SYMBOL_GPL(acpi_find_child_device);
 
 acpi_handle acpi_get_child(acpi_handle handle, u64 addr)
 {
@@ -294,15 +295,6 @@ int acpi_unbind_one(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(acpi_unbind_one);
 
-void acpi_preset_companion(struct device *dev, acpi_handle parent, u64 addr)
-{
-	struct acpi_device *adev;
-
-	if (!acpi_bus_get_device(acpi_get_child(parent, addr), &adev))
-		ACPI_COMPANION_SET(dev, adev);
-}
-EXPORT_SYMBOL_GPL(acpi_preset_companion);
-
 static int acpi_platform_notify(struct device *dev)
 {
 	struct acpi_bus_type *type = acpi_get_bus_type(dev);
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -437,7 +437,6 @@ struct acpi_pci_root {
 struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
 					   u64 address, bool check_children);
 acpi_handle acpi_get_child(acpi_handle handle, u64 addr);
-void acpi_preset_companion(struct device *dev, acpi_handle parent, u64 addr);
 int acpi_is_root_bridge(acpi_handle);
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
 
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -53,6 +53,12 @@ static inline acpi_handle acpi_device_ha
 #define ACPI_COMPANION_SET(dev, adev)	ACPI_COMPANION(dev) = (adev)
 #define ACPI_HANDLE(dev)		acpi_device_handle(ACPI_COMPANION(dev))
 
+static inline void acpi_preset_companion(struct device *dev,
+					 struct acpi_device *parent, u64 addr)
+{
+	ACPI_COMPANION_SET(dev, acpi_find_child_device(parent, addr, NULL));
+}
+
 static inline const char *acpi_dev_name(struct acpi_device *adev)
 {
 	return dev_name(&adev->dev);


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-25  0:09 [PATCH 0/4] ACPI / bind: Simplify child devices lookup Rafael J. Wysocki
                   ` (3 preceding siblings ...)
  2013-11-25  0:14 ` [PATCH 4/4] ACPI / bind: Redefine acpi_preset_companion() Rafael J. Wysocki
@ 2013-11-25  3:17 ` Aaron Lu
  2013-11-27  0:00 ` Toshi Kani
  5 siblings, 0 replies; 15+ messages in thread
From: Aaron Lu @ 2013-11-25  3:17 UTC (permalink / raw)
  To: Rafael J. Wysocki, ACPI Devel Maling List; +Cc: LKML, Linux PCI, Bjorn Helgaas

On 11/25/2013 08:09 AM, Rafael J. Wysocki wrote:
> Hi,
> 
> The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> rework child device lookup in drivers/acpi/glue.c and related things:
> 
> [1/4] ACPI / bind: Simplify child device lookup
> [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> [3/4] ACPI / bind: Redefine acpi_get_child()
> [4/4] ACPI / bind: Redefine acpi_preset_companion()
> 
> Thanks!
> 

Reviewed-by: Aaron Lu <aaron.lu@intel.com>
Tested-by: Aaron Lu <aaron.lu@intel.com> #for ATA binding

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-25  0:09 [PATCH 0/4] ACPI / bind: Simplify child devices lookup Rafael J. Wysocki
                   ` (4 preceding siblings ...)
  2013-11-25  3:17 ` [PATCH 0/4] ACPI / bind: Simplify child devices lookup Aaron Lu
@ 2013-11-27  0:00 ` Toshi Kani
  2013-11-27  0:27   ` Rafael J. Wysocki
  5 siblings, 1 reply; 15+ messages in thread
From: Toshi Kani @ 2013-11-27  0:00 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> Hi,
> 
> The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> rework child device lookup in drivers/acpi/glue.c and related things:
> 
> [1/4] ACPI / bind: Simplify child device lookup
> [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> [3/4] ACPI / bind: Redefine acpi_get_child()
> [4/4] ACPI / bind: Redefine acpi_preset_companion()

This patchset caused the attached panic during boot on a system.
acpi_pci_find_device() called acpi_find_child_device() with
ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.

This bus 0xf seems to be a chipset internal bus, which is not intended
for the OS to use.  Therefore, ACPI does not list its PCI bridge device.

# lspci -tv
   :
 +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
 |           +-08.2  Intel Corporation Ivytown QPI Link 0
   :

However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
finds this bus as it scans all the buses from 0 to pcibios_last_bus.
Hence, this dev->parent does not have an associated ACPI device object. 

Thanks,
-Toshi


[-- Attachment #2: panic.log --]
[-- Type: text/x-log, Size: 3039 bytes --]

 :
PCI: Discovered peer bus 0f
PCI host bridge to bus 0000:0f
pci_bus 0000:0f: root bus resource [io  0x0000-0xffff]
pci_bus 0000:0f: root bus resource [mem 0x00000000-0x3fffffffffff]
pci_bus 0000:0f: No busn resource found for root bus, will use [bus 0f-ff]
BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: [<ffffffff812da078>] acpi_find_child_device+0x23/0xc3
PGD 0 
Oops: 0000 [#1] SMP 
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc1+ #504
Hardware name: HP CB920s x1, BIOS Bundle: 005.028.018 SFW: 012.124.000 10/28/2013
task: ffff88085fa5d110 ti: ffff88085fa5e000 task.ti: ffff88085fa5e000
RIP: 0010:[<ffffffff812da078>]  [<ffffffff812da078>] acpi_find_child_device+0x23/0xc3
RSP: 0000:ffff88085fa5fbc0  EFLAGS: 00010286
RAX: ffff8a07de1c1800 RBX: ffff88085fa5fc28 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000080000 RDI: 0000000000000000
RBP: ffff88085fa5fbf8 R08: ffff88085ebb2578 R09: 0000000000000000
R10: 0000000000003c0a R11: 0000000000000006 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000080000
FS:  0000000000000000(0000) GS:ffff88087fa00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000018 CR3: 0000000001a0b000 CR4: 00000000001407f0
Stack:
 ffff88085fa5fc10 ffffffff812da3cb ffff88085fa5fc28 ffff88085ebb0098
 ffffffff81a7c860 ffff88085ebb0098 00000000ffffffff ffff88085fa5fc10
 ffffffff812affde ffff88085ebb00a8 ffff88085fa5fc40 ffffffff812da620
Call Trace:
 [<ffffffff812da3cb>] ? acpi_bind_one+0x65/0x280
 [<ffffffff812affde>] acpi_pci_find_device+0x3e/0x60
 [<ffffffff812da620>] acpi_platform_notify+0x3a/0x6c
 [<ffffffff8135eb4d>] device_add+0x13d/0x640
 [<ffffffff81292286>] ? pci_find_capability+0x26/0x50
 [<ffffffff8128eea4>] pci_device_add+0x114/0x150
 [<ffffffff8153ce41>] pci_scan_single_device+0x91/0xc0
 [<ffffffff8128ef2e>] pci_scan_slot+0x4e/0x150
 [<ffffffff8128fd7d>] pci_scan_child_bus+0x3d/0x150
 [<ffffffff81290090>] pci_scan_root_bus+0xa0/0xb0
 [<ffffffff8146551c>] pci_scan_bus_on_node+0x7c/0xd0
 [<ffffffff81463ea7>] pcibios_scan_specific_bus+0x97/0xa0
 [<ffffffff81b32e62>] ? pci_legacy_init+0x37/0x37
 [<ffffffff81b32e98>] pci_subsys_init+0x36/0x48
 [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0
 [<ffffffff81067fcd>] ? parse_args+0x21d/0x3f0
 [<ffffffff81aee047>] kernel_init_freeable+0x18f/0x21a
 [<ffffffff81aed833>] ? do_early_param+0x88/0x88
 [<ffffffff81539fe0>] ? rest_init+0x80/0x80
 [<ffffffff81539fee>] kernel_init+0xe/0x130
 [<ffffffff815563ec>] ret_from_fork+0x7c/0xb0
 [<ffffffff81539fe0>] ? rest_init+0x80/0x80
Code: ff ff 5a 5b 41 5c 5d c3 0f 1f 44 00 00 55 48 89 e5 41 57 49 89 f7 41 56 45 31 f6 41 55 44 0f b6 ea 41 54 45 31 e4 53 48 83 ec 10 <48> 8b 47 18 48 8d 58 d8 48 8d 47 18 48 89 45 c8 48 8d 43 28 48 
RIP  [<ffffffff812da078>] acpi_find_child_device+0x23/0xc3
 RSP <ffff88085fa5fbc0>
CR2: 0000000000000018
---[ end trace 752051e71cfc0265 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  0:00 ` Toshi Kani
@ 2013-11-27  0:27   ` Rafael J. Wysocki
  2013-11-27  0:33     ` Toshi Kani
  0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-27  0:27 UTC (permalink / raw)
  To: Toshi Kani
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Tuesday, November 26, 2013 05:00:42 PM Toshi Kani wrote:
> On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> > rework child device lookup in drivers/acpi/glue.c and related things:
> > 
> > [1/4] ACPI / bind: Simplify child device lookup
> > [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> > [3/4] ACPI / bind: Redefine acpi_get_child()
> > [4/4] ACPI / bind: Redefine acpi_preset_companion()
> 
> This patchset caused the attached panic during boot on a system.
> acpi_pci_find_device() called acpi_find_child_device() with
> ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.
> 
> This bus 0xf seems to be a chipset internal bus, which is not intended
> for the OS to use.  Therefore, ACPI does not list its PCI bridge device.
> 
> # lspci -tv
>    :
>  +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
>  |           +-08.2  Intel Corporation Ivytown QPI Link 0
>    :
> 
> However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
> finds this bus as it scans all the buses from 0 to pcibios_last_bus.
> Hence, this dev->parent does not have an associated ACPI device object. 

Thanks for the report!

I've dropped the patches from bleeding-edge for now.

Does "[1/4] ACPI / bind: Simplify child device lookup" alone work on that
system?

Rafael


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  0:27   ` Rafael J. Wysocki
@ 2013-11-27  0:33     ` Toshi Kani
  2013-11-27  1:02       ` Rafael J. Wysocki
  0 siblings, 1 reply; 15+ messages in thread
From: Toshi Kani @ 2013-11-27  0:33 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Wed, 2013-11-27 at 01:27 +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 26, 2013 05:00:42 PM Toshi Kani wrote:
> > On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> > > Hi,
> > > 
> > > The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> > > rework child device lookup in drivers/acpi/glue.c and related things:
> > > 
> > > [1/4] ACPI / bind: Simplify child device lookup
> > > [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> > > [3/4] ACPI / bind: Redefine acpi_get_child()
> > > [4/4] ACPI / bind: Redefine acpi_preset_companion()
> > 
> > This patchset caused the attached panic during boot on a system.
> > acpi_pci_find_device() called acpi_find_child_device() with
> > ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.
> > 
> > This bus 0xf seems to be a chipset internal bus, which is not intended
> > for the OS to use.  Therefore, ACPI does not list its PCI bridge device.
> > 
> > # lspci -tv
> >    :
> >  +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
> >  |           +-08.2  Intel Corporation Ivytown QPI Link 0
> >    :
> > 
> > However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
> > finds this bus as it scans all the buses from 0 to pcibios_last_bus.
> > Hence, this dev->parent does not have an associated ACPI device object. 
> 
> Thanks for the report!
> 
> I've dropped the patches from bleeding-edge for now.
> 
> Does "[1/4] ACPI / bind: Simplify child device lookup" alone work on that
> system?

Yes, the system boots fine with 1/4 alone.

Thanks,
-Toshi


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  0:33     ` Toshi Kani
@ 2013-11-27  1:02       ` Rafael J. Wysocki
  2013-11-27  1:11         ` Toshi Kani
  0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-27  1:02 UTC (permalink / raw)
  To: Toshi Kani
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Tuesday, November 26, 2013 05:33:28 PM Toshi Kani wrote:
> On Wed, 2013-11-27 at 01:27 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, November 26, 2013 05:00:42 PM Toshi Kani wrote:
> > > On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> > > > Hi,
> > > > 
> > > > The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> > > > rework child device lookup in drivers/acpi/glue.c and related things:
> > > > 
> > > > [1/4] ACPI / bind: Simplify child device lookup
> > > > [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> > > > [3/4] ACPI / bind: Redefine acpi_get_child()
> > > > [4/4] ACPI / bind: Redefine acpi_preset_companion()
> > > 
> > > This patchset caused the attached panic during boot on a system.
> > > acpi_pci_find_device() called acpi_find_child_device() with
> > > ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.
> > > 
> > > This bus 0xf seems to be a chipset internal bus, which is not intended
> > > for the OS to use.  Therefore, ACPI does not list its PCI bridge device.
> > > 
> > > # lspci -tv
> > >    :
> > >  +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
> > >  |           +-08.2  Intel Corporation Ivytown QPI Link 0
> > >    :
> > > 
> > > However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
> > > finds this bus as it scans all the buses from 0 to pcibios_last_bus.
> > > Hence, this dev->parent does not have an associated ACPI device object. 
> > 
> > Thanks for the report!
> > 
> > I've dropped the patches from bleeding-edge for now.
> > 
> > Does "[1/4] ACPI / bind: Simplify child device lookup" alone work on that
> > system?
> 
> Yes, the system boots fine with 1/4 alone.

Thanks!

Can you please check if the following modified [2/4] also works on top of it?

Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: PCI / ACPI: Use acpi_find_child_device() for child devices lookup

It is much more efficient to use acpi_find_child_device()
for child devices lookup in acpi_pci_find_device() and pass
ACPI_COMPANION(dev->parent) to it directly instead of obtaining
ACPI_HANDLE() of ACPI_COMPANION(dev->parent) and passing it to
acpi_find_child() which has to run acpi_bus_get_device() to
obtain ACPI_COMPANION(dev->parent) from that again.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/glue.c    |    3 +++
 drivers/pci/pci-acpi.c |   16 ++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -309,7 +309,8 @@ void acpi_pci_remove_bus(struct pci_bus
 static int acpi_pci_find_device(struct device *dev, acpi_handle *handle)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	bool is_bridge;
+	struct acpi_device *adev;
+	bool check_children;
 	u64 addr;
 
 	/*
@@ -317,14 +318,17 @@ static int acpi_pci_find_device(struct d
 	 * is set only after acpi_pci_find_device() has been called for the
 	 * given device.
 	 */
-	is_bridge = pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE
+	check_children = pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE
 			|| pci_dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
 	/* Please ref to ACPI spec for the syntax of _ADR */
 	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
-	*handle = acpi_find_child(ACPI_HANDLE(dev->parent), addr, is_bridge);
-	if (!*handle)
-		return -ENODEV;
-	return 0;
+	adev = acpi_find_child_device(ACPI_COMPANION(dev->parent), addr,
+				      check_children);
+	if (adev) {
+		*handle = adev->handle;
+		return 0;
+	}
+	return -ENODEV;
 }
 
 static void pci_acpi_setup(struct device *dev)
Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -106,6 +106,9 @@ struct acpi_device *acpi_find_child_devi
 	struct acpi_device *adev, *ret = NULL;
 	int ret_score = 0;
 
+	if (!parent)
+		return NULL;
+
 	list_for_each_entry(adev, &parent->children, node) {
 		unsigned long long addr;
 		acpi_status status;


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  1:02       ` Rafael J. Wysocki
@ 2013-11-27  1:11         ` Toshi Kani
  2013-11-27  1:32           ` Rafael J. Wysocki
  0 siblings, 1 reply; 15+ messages in thread
From: Toshi Kani @ 2013-11-27  1:11 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Wed, 2013-11-27 at 02:02 +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 26, 2013 05:33:28 PM Toshi Kani wrote:
> > On Wed, 2013-11-27 at 01:27 +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, November 26, 2013 05:00:42 PM Toshi Kani wrote:
> > > > On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> > > > > Hi,
> > > > > 
> > > > > The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> > > > > rework child device lookup in drivers/acpi/glue.c and related things:
> > > > > 
> > > > > [1/4] ACPI / bind: Simplify child device lookup
> > > > > [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> > > > > [3/4] ACPI / bind: Redefine acpi_get_child()
> > > > > [4/4] ACPI / bind: Redefine acpi_preset_companion()
> > > > 
> > > > This patchset caused the attached panic during boot on a system.
> > > > acpi_pci_find_device() called acpi_find_child_device() with
> > > > ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.
> > > > 
> > > > This bus 0xf seems to be a chipset internal bus, which is not intended
> > > > for the OS to use.  Therefore, ACPI does not list its PCI bridge device.
> > > > 
> > > > # lspci -tv
> > > >    :
> > > >  +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
> > > >  |           +-08.2  Intel Corporation Ivytown QPI Link 0
> > > >    :
> > > > 
> > > > However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
> > > > finds this bus as it scans all the buses from 0 to pcibios_last_bus.
> > > > Hence, this dev->parent does not have an associated ACPI device object. 
> > > 
> > > Thanks for the report!
> > > 
> > > I've dropped the patches from bleeding-edge for now.
> > > 
> > > Does "[1/4] ACPI / bind: Simplify child device lookup" alone work on that
> > > system?
> > 
> > Yes, the system boots fine with 1/4 alone.
> 
> Thanks!
> 
> Can you please check if the following modified [2/4] also works on top of it?

Yes, it works fine.

Thanks for the quick fix!
-Toshi



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  1:11         ` Toshi Kani
@ 2013-11-27  1:32           ` Rafael J. Wysocki
  2013-11-27  1:40             ` Toshi Kani
  0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-27  1:32 UTC (permalink / raw)
  To: Toshi Kani
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Tuesday, November 26, 2013 06:11:57 PM Toshi Kani wrote:
> On Wed, 2013-11-27 at 02:02 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, November 26, 2013 05:33:28 PM Toshi Kani wrote:
> > > On Wed, 2013-11-27 at 01:27 +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, November 26, 2013 05:00:42 PM Toshi Kani wrote:
> > > > > On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> > > > > > rework child device lookup in drivers/acpi/glue.c and related things:
> > > > > > 
> > > > > > [1/4] ACPI / bind: Simplify child device lookup
> > > > > > [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> > > > > > [3/4] ACPI / bind: Redefine acpi_get_child()
> > > > > > [4/4] ACPI / bind: Redefine acpi_preset_companion()
> > > > > 
> > > > > This patchset caused the attached panic during boot on a system.
> > > > > acpi_pci_find_device() called acpi_find_child_device() with
> > > > > ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.
> > > > > 
> > > > > This bus 0xf seems to be a chipset internal bus, which is not intended
> > > > > for the OS to use.  Therefore, ACPI does not list its PCI bridge device.
> > > > > 
> > > > > # lspci -tv
> > > > >    :
> > > > >  +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
> > > > >  |           +-08.2  Intel Corporation Ivytown QPI Link 0
> > > > >    :
> > > > > 
> > > > > However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
> > > > > finds this bus as it scans all the buses from 0 to pcibios_last_bus.
> > > > > Hence, this dev->parent does not have an associated ACPI device object. 
> > > > 
> > > > Thanks for the report!
> > > > 
> > > > I've dropped the patches from bleeding-edge for now.
> > > > 
> > > > Does "[1/4] ACPI / bind: Simplify child device lookup" alone work on that
> > > > system?
> > > 
> > > Yes, the system boots fine with 1/4 alone.
> > 
> > Thanks!
> > 
> > Can you please check if the following modified [2/4] also works on top of it?
> 
> Yes, it works fine.
> 
> Thanks for the quick fix!

No problem. :-)

[3/4] and [4/4] on top of this one should work too.

Thanks,
Rafael


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  1:32           ` Rafael J. Wysocki
@ 2013-11-27  1:40             ` Toshi Kani
  2013-11-27  1:58               ` Rafael J. Wysocki
  0 siblings, 1 reply; 15+ messages in thread
From: Toshi Kani @ 2013-11-27  1:40 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Wed, 2013-11-27 at 02:32 +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 26, 2013 06:11:57 PM Toshi Kani wrote:
> > On Wed, 2013-11-27 at 02:02 +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, November 26, 2013 05:33:28 PM Toshi Kani wrote:
> > > > On Wed, 2013-11-27 at 01:27 +0100, Rafael J. Wysocki wrote:
> > > > > On Tuesday, November 26, 2013 05:00:42 PM Toshi Kani wrote:
> > > > > > On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> > > > > > > rework child device lookup in drivers/acpi/glue.c and related things:
> > > > > > > 
> > > > > > > [1/4] ACPI / bind: Simplify child device lookup
> > > > > > > [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> > > > > > > [3/4] ACPI / bind: Redefine acpi_get_child()
> > > > > > > [4/4] ACPI / bind: Redefine acpi_preset_companion()
> > > > > > 
> > > > > > This patchset caused the attached panic during boot on a system.
> > > > > > acpi_pci_find_device() called acpi_find_child_device() with
> > > > > > ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.
> > > > > > 
> > > > > > This bus 0xf seems to be a chipset internal bus, which is not intended
> > > > > > for the OS to use.  Therefore, ACPI does not list its PCI bridge device.
> > > > > > 
> > > > > > # lspci -tv
> > > > > >    :
> > > > > >  +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
> > > > > >  |           +-08.2  Intel Corporation Ivytown QPI Link 0
> > > > > >    :
> > > > > > 
> > > > > > However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
> > > > > > finds this bus as it scans all the buses from 0 to pcibios_last_bus.
> > > > > > Hence, this dev->parent does not have an associated ACPI device object. 
> > > > > 
> > > > > Thanks for the report!
> > > > > 
> > > > > I've dropped the patches from bleeding-edge for now.
> > > > > 
> > > > > Does "[1/4] ACPI / bind: Simplify child device lookup" alone work on that
> > > > > system?
> > > > 
> > > > Yes, the system boots fine with 1/4 alone.
> > > 
> > > Thanks!
> > > 
> > > Can you please check if the following modified [2/4] also works on top of it?
> > 
> > Yes, it works fine.
> > 
> > Thanks for the quick fix!
> 
> No problem. :-)
> 
> [3/4] and [4/4] on top of this one should work too.

Yes, the system booted fine with 3/4 & 4/4. :-)

Thanks,
-Toshi


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  1:40             ` Toshi Kani
@ 2013-11-27  1:58               ` Rafael J. Wysocki
  2013-11-29  0:27                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-27  1:58 UTC (permalink / raw)
  To: Toshi Kani
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Tuesday, November 26, 2013 06:40:28 PM Toshi Kani wrote:
> On Wed, 2013-11-27 at 02:32 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, November 26, 2013 06:11:57 PM Toshi Kani wrote:
> > > On Wed, 2013-11-27 at 02:02 +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, November 26, 2013 05:33:28 PM Toshi Kani wrote:
> > > > > On Wed, 2013-11-27 at 01:27 +0100, Rafael J. Wysocki wrote:
> > > > > > On Tuesday, November 26, 2013 05:00:42 PM Toshi Kani wrote:
> > > > > > > On Mon, 2013-11-25 at 01:09 +0100, Rafael J. Wysocki wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > The following series of four patches (on top of current linux-pm.git/bleeding-edge)
> > > > > > > > rework child device lookup in drivers/acpi/glue.c and related things:
> > > > > > > > 
> > > > > > > > [1/4] ACPI / bind: Simplify child device lookup
> > > > > > > > [2/4] PCI/ ACPI: Use acpi_find_child_device() for child device lookup
> > > > > > > > [3/4] ACPI / bind: Redefine acpi_get_child()
> > > > > > > > [4/4] ACPI / bind: Redefine acpi_preset_companion()
> > > > > > > 
> > > > > > > This patchset caused the attached panic during boot on a system.
> > > > > > > acpi_pci_find_device() called acpi_find_child_device() with
> > > > > > > ACPI_COMPANION(dev->parent) being a NULL pointer when scanning bus 0xf.
> > > > > > > 
> > > > > > > This bus 0xf seems to be a chipset internal bus, which is not intended
> > > > > > > for the OS to use.  Therefore, ACPI does not list its PCI bridge device.
> > > > > > > 
> > > > > > > # lspci -tv
> > > > > > >    :
> > > > > > >  +-[0000:0f]-+-08.0  Intel Corporation Ivytown QPI Link 0
> > > > > > >  |           +-08.2  Intel Corporation Ivytown QPI Link 0
> > > > > > >    :
> > > > > > > 
> > > > > > > However, pcibios_fixup_peer_bridges(), called from pci_subsys_init(),
> > > > > > > finds this bus as it scans all the buses from 0 to pcibios_last_bus.
> > > > > > > Hence, this dev->parent does not have an associated ACPI device object. 
> > > > > > 
> > > > > > Thanks for the report!
> > > > > > 
> > > > > > I've dropped the patches from bleeding-edge for now.
> > > > > > 
> > > > > > Does "[1/4] ACPI / bind: Simplify child device lookup" alone work on that
> > > > > > system?
> > > > > 
> > > > > Yes, the system boots fine with 1/4 alone.
> > > > 
> > > > Thanks!
> > > > 
> > > > Can you please check if the following modified [2/4] also works on top of it?
> > > 
> > > Yes, it works fine.
> > > 
> > > Thanks for the quick fix!
> > 
> > No problem. :-)
> > 
> > [3/4] and [4/4] on top of this one should work too.
> 
> Yes, the system booted fine with 3/4 & 4/4. :-)

Thanks for the verification!

Rafael


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] ACPI / bind: Simplify child devices lookup
  2013-11-27  1:58               ` Rafael J. Wysocki
@ 2013-11-29  0:27                 ` Rafael J. Wysocki
  0 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2013-11-29  0:27 UTC (permalink / raw)
  To: Toshi Kani
  Cc: ACPI Devel Maling List, LKML, Linux PCI, Bjorn Helgaas, Aaron Lu

On Wednesday, November 27, 2013 02:58:54 AM Rafael J. Wysocki wrote:
> On Tuesday, November 26, 2013 06:40:28 PM Toshi Kani wrote:

[...]

> > 
> > Yes, the system booted fine with 3/4 & 4/4. :-)
> 
> Thanks for the verification!

In fact, I've made one more mistake in that patch.  Namely, acpi_find_child()
has to return an ACPI handle and it returned a pointer to struct acpi_device
after it.

That has been fixed in my tree already, but the updated patch follows for
completness (it requires patch [3/4] to be changed, but that is just a trivial
rebase, so I don't think it is necessary to resend it too).

Thanks,
Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / bind: Simplify child device lookups

Now that we create a struct acpi_device object for every ACPI
namespace node representing a device, it is not necessary to
use acpi_walk_namespace() for child device lookup in
acpi_find_child() any more.  Instead, we can simply walk the
list of children of the given struct acpi_device object and
return the matching one (or the one which is the best match if
there are more of them).  The checks done during the matching
loop can be simplified too so that the secondary namespace walks
in find_child_checks() are not necessary any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c     |  135 +++++++++++++++++++-----------------------------
 include/acpi/acpi_bus.h |    3 +
 2 files changed, 57 insertions(+), 81 deletions(-)

Index: linux-pm/drivers/acpi/glue.c
===================================================================
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -82,107 +82,80 @@ static struct acpi_bus_type *acpi_get_bu
 #define FIND_CHILD_MIN_SCORE	1
 #define FIND_CHILD_MAX_SCORE	2
 
-static acpi_status acpi_dev_present(acpi_handle handle, u32 lvl_not_used,
-				  void *not_used, void **ret_p)
-{
-	struct acpi_device *adev = NULL;
-
-	acpi_bus_get_device(handle, &adev);
-	if (adev) {
-		*ret_p = handle;
-		return AE_CTRL_TERMINATE;
-	}
-	return AE_OK;
-}
-
-static int do_find_child_checks(acpi_handle handle, bool is_bridge)
+static int find_child_checks(struct acpi_device *adev, bool check_children)
 {
 	bool sta_present = true;
 	unsigned long long sta;
 	acpi_status status;
 
-	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+	status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);
 	if (status == AE_NOT_FOUND)
 		sta_present = false;
 	else if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_ENABLED))
 		return -ENODEV;
 
-	if (is_bridge) {
-		void *test = NULL;
+	if (check_children && list_empty(&adev->children))
+		return -ENODEV;
 
-		/* Check if this object has at least one child device. */
-		acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
-				    acpi_dev_present, NULL, NULL, &test);
-		if (!test)
-			return -ENODEV;
-	}
 	return sta_present ? FIND_CHILD_MAX_SCORE : FIND_CHILD_MIN_SCORE;
 }
 
-struct find_child_context {
-	u64 addr;
-	bool is_bridge;
-	acpi_handle ret;
-	int ret_score;
-};
-
-static acpi_status do_find_child(acpi_handle handle, u32 lvl_not_used,
-				 void *data, void **not_used)
+struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
+					   u64 address, bool check_children)
 {
-	struct find_child_context *context = data;
-	unsigned long long addr;
-	acpi_status status;
-	int score;
+	struct acpi_device *adev, *ret = NULL;
+	int ret_score = 0;
 
-	status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &addr);
-	if (ACPI_FAILURE(status) || addr != context->addr)
-		return AE_OK;
-
-	if (!context->ret) {
-		/* This is the first matching object.  Save its handle. */
-		context->ret = handle;
-		return AE_OK;
-	}
-	/*
-	 * There is more than one matching object with the same _ADR value.
-	 * That really is unexpected, so we are kind of beyond the scope of the
-	 * spec here.  We have to choose which one to return, though.
-	 *
-	 * First, check if the previously found object is good enough and return
-	 * its handle if so.  Second, check the same for the object that we've
-	 * just found.
-	 */
-	if (!context->ret_score) {
-		score = do_find_child_checks(context->ret, context->is_bridge);
-		if (score == FIND_CHILD_MAX_SCORE)
-			return AE_CTRL_TERMINATE;
-		else
-			context->ret_score = score;
-	}
-	score = do_find_child_checks(handle, context->is_bridge);
-	if (score == FIND_CHILD_MAX_SCORE) {
-		context->ret = handle;
-		return AE_CTRL_TERMINATE;
-	} else if (score > context->ret_score) {
-		context->ret = handle;
-		context->ret_score = score;
+	list_for_each_entry(adev, &parent->children, node) {
+		unsigned long long addr;
+		acpi_status status;
+		int score;
+
+		status = acpi_evaluate_integer(adev->handle, METHOD_NAME__ADR,
+					       NULL, &addr);
+		if (ACPI_FAILURE(status) || addr != address)
+			continue;
+
+		if (!ret) {
+			/* This is the first matching object.  Save it. */
+			ret = adev;
+			continue;
+		}
+		/*
+		 * There is more than one matching device object with the same
+		 * _ADR value.  That really is unexpected, so we are kind of
+		 * beyond the scope of the spec here.  We have to choose which
+		 * one to return, though.
+		 *
+		 * First, check if the previously found object is good enough
+		 * and return it if so.  Second, do the same for the object that
+		 * we've just found.
+		 */
+		if (!ret_score) {
+			ret_score = find_child_checks(ret, check_children);
+			if (ret_score == FIND_CHILD_MAX_SCORE)
+				return ret;
+		}
+		score = find_child_checks(adev, check_children);
+		if (score == FIND_CHILD_MAX_SCORE) {
+			return adev;
+		} else if (score > ret_score) {
+			ret = adev;
+			ret_score = score;
+		}
 	}
-	return AE_OK;
+	return ret;
 }
 
-acpi_handle acpi_find_child(acpi_handle parent, u64 addr, bool is_bridge)
+acpi_handle acpi_find_child(acpi_handle handle, u64 addr, bool is_bridge)
 {
-	if (parent) {
-		struct find_child_context context = {
-			.addr = addr,
-			.is_bridge = is_bridge,
-		};
-
-		acpi_walk_namespace(ACPI_TYPE_DEVICE, parent, 1, do_find_child,
-				    NULL, &context, NULL);
-		return context.ret;
-	}
-	return NULL;
+	struct acpi_device *adev;
+
+	if (!handle || acpi_bus_get_device(handle, &adev))
+		return NULL;
+
+	adev = acpi_find_child_device(adev, addr, is_bridge);
+	return adev ? adev->handle : NULL;
 }
 EXPORT_SYMBOL_GPL(acpi_find_child);
 
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -433,6 +433,9 @@ struct acpi_pci_root {
 };
 
 /* helper */
+
+struct acpi_device *acpi_find_child_device(struct acpi_device *parent,
+					   u64 address, bool check_children);
 acpi_handle acpi_find_child(acpi_handle, u64, bool);
 static inline acpi_handle acpi_get_child(acpi_handle handle, u64 addr)
 {


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-11-29  0:14 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-25  0:09 [PATCH 0/4] ACPI / bind: Simplify child devices lookup Rafael J. Wysocki
2013-11-25  0:10 ` [PATCH 1/4] ACPI / bind: Simplify child device lookups Rafael J. Wysocki
2013-11-25  0:12 ` [PATCH 2/4] PCI / ACPI: Use acpi_find_child_device() for child devices lookup Rafael J. Wysocki
2013-11-25  0:12 ` [PATCH 3/4] ACPI / bind: Redefine acpi_get_child() Rafael J. Wysocki
2013-11-25  0:14 ` [PATCH 4/4] ACPI / bind: Redefine acpi_preset_companion() Rafael J. Wysocki
2013-11-25  3:17 ` [PATCH 0/4] ACPI / bind: Simplify child devices lookup Aaron Lu
2013-11-27  0:00 ` Toshi Kani
2013-11-27  0:27   ` Rafael J. Wysocki
2013-11-27  0:33     ` Toshi Kani
2013-11-27  1:02       ` Rafael J. Wysocki
2013-11-27  1:11         ` Toshi Kani
2013-11-27  1:32           ` Rafael J. Wysocki
2013-11-27  1:40             ` Toshi Kani
2013-11-27  1:58               ` Rafael J. Wysocki
2013-11-29  0:27                 ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).