[PATCH 0/3 v2] Find pci root bridges by comparing HID from acpi_device_info, not acpi_device.

[PATCH 0/3 v2] Find pci root bridges by comparing HID from acpi_device_info, not acpi_device.

[Tang Chen]

Hi Yinghai,

I found a little problem in your following patches. 
[PATCH 00/40] PCI, ACPI, x86: pci root bus hotplug support.

In acpi_is_root_bridge(), it gets device's HID from acpi_device struct.
If the device is not added when the system boots, there will be no
acpi_device for it. And as a result, acpi_is_root_bridge() will fail.
But it doesn't mean that the device is not a root bridge.

In this case, the not-added root bridges have no handle_hotplug_event_root()
callback registerred. And when we hot add it later, it will do nothing.

This patch set changes the acpi_is_root_bridge()'s behavior. When we fail
to get HID from acpi_device, we will get HID from acpi_device_info by
acpi_get_object_info(), which gets the HID from ACPI namespace directly.

So no matter the device is present or not, we will always find out if it
is a pci root bridge, and register a handle_hotplug_event_root() callback
for it.

These patches are based on Lu Yinghai's for-pci-root-bus-hotplug branch.

Tang Chen (3):
  Introduce a new acpi to determine HID match.
  Do not use acpi_device to find pci root bridge in _init code.
  Check exit status of acpi_install_notify_handler() in
    find_root_bridges().

 drivers/acpi/pci_root.c    |   19 +++++++++++--------
 drivers/acpi/pci_root_hp.c |   12 +++++++++---
 drivers/acpi/scan.c        |   24 ++++++++++++++++++++++++
 include/acpi/acpi_bus.h    |    2 ++
 4 files changed, 46 insertions(+), 11 deletions(-)

[PATCH 1/3 v2] Introduce a new acpi to determine HID match.

[Tang Chen]

This introduce a new api to determine if a HID matches a list of IDs.
Different from acpi_match_device_ids(), the new api gets HID from
acpi_device_info struct.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/scan.c     |   24 ++++++++++++++++++++++++
 include/acpi/acpi_bus.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 03c12e6..41fd4c9 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -312,6 +312,30 @@ int acpi_match_device_ids(struct acpi_device *device,
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
+int acpi_match_object_info_ids(struct acpi_device_info *info,
+			       const struct acpi_device_id *ids)
+{
+	char *hardware_id;
+	int i, j;
+
+	for (i = 0; ids[i].id[0]; i++) {
+		hardware_id = info->hardware_id.string;
+		if (hardware_id &&
+		    !strcmp((char *)ids[i].id, hardware_id))
+				return 0;
+
+		for (j = 0; j < info->compatible_id_list.count; j++) {
+			hardware_id = info->compatible_id_list.ids[j].string;
+			if (hardware_id &&
+			    !strcmp((char *)ids[i].id, hardware_id))
+				return 0;
+		}
+	}
+
+	return 1;
+}
+EXPORT_SYMBOL(acpi_match_object_info_ids);
+
 static void acpi_free_ids(struct acpi_device *device)
 {
 	struct acpi_hardware_id *id, *tmp;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index f0b9681..f83d581 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -379,6 +379,8 @@ int acpi_bus_start(struct acpi_device *device);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids);
+int acpi_match_object_info_ids(struct acpi_device_info *info,
+			       const struct acpi_device_id *ids);
 int acpi_create_dir(struct acpi_device *);
 void acpi_remove_dir(struct acpi_device *);
 
-- 
1.7.1

Re: [PATCH 1/3 v2] Introduce a new acpi to determine HID match.

[Yinghai Lu]

On Fri, Oct 12, 2012 at 5:34 AM, Tang Chen <tangchen@cn.fujitsu.com> wrote:
> This introduce a new api to determine if a HID matches a list of IDs.
> Different from acpi_match_device_ids(), the new api gets HID from
> acpi_device_info struct.
>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  drivers/acpi/scan.c     |   24 ++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |    2 ++
>  2 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 03c12e6..41fd4c9 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -312,6 +312,30 @@ int acpi_match_device_ids(struct acpi_device *device,
>  }
>  EXPORT_SYMBOL(acpi_match_device_ids);
>
> +int acpi_match_object_info_ids(struct acpi_device_info *info,
> +                              const struct acpi_device_id *ids)
> +{
> +       char *hardware_id;
> +       int i, j;
> +
> +       for (i = 0; ids[i].id[0]; i++) {
> +               hardware_id = info->hardware_id.string;
> +               if (hardware_id &&
> +                   !strcmp((char *)ids[i].id, hardware_id))
> +                               return 0;
> +
> +               for (j = 0; j < info->compatible_id_list.count; j++) {
> +                       hardware_id = info->compatible_id_list.ids[j].string;
> +                       if (hardware_id &&
> +                           !strcmp((char *)ids[i].id, hardware_id))
> +                               return 0;
> +               }
> +       }
> +
> +       return 1;

-ENOENT instead of 1?

> +}
> +EXPORT_SYMBOL(acpi_match_object_info_ids);
> +
>  static void acpi_free_ids(struct acpi_device *device)
>  {
>         struct acpi_hardware_id *id, *tmp;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index f0b9681..f83d581 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -379,6 +379,8 @@ int acpi_bus_start(struct acpi_device *device);
>  acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
>  int acpi_match_device_ids(struct acpi_device *device,
>                           const struct acpi_device_id *ids);
> +int acpi_match_object_info_ids(struct acpi_device_info *info,
> +                              const struct acpi_device_id *ids);
>  int acpi_create_dir(struct acpi_device *);
>  void acpi_remove_dir(struct acpi_device *);
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3 v2] Do not use acpi_device to find pci root bridge in _init code.

[Tang Chen]

When the kernel is being initialized, and some hardwares are not added
to system, there won't be acpi_device structs for these devices. But
acpi_is_root_bridge() depends on acpi_device struct. As a result, all
the not-added root bridge will not be judged as a root bridge in
find_root_bridges(). And further more, no handle_hotplug_event_root()
notifier will be installed for them.

This patch introduces a new api to find all root bridges in system by
getting HID directly from ACPI namespace, not depending on acpi_device
struct.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Liu Jiang <jiang.liu@huawei.com>
---
 drivers/acpi/pci_root.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6151d83..582eb11 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -129,20 +129,23 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
  * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
  * @handle - the ACPI CA node in question.
  *
- * Note: we could make this API take a struct acpi_device * instead, but
- * for now, it's more convenient to operate on an acpi_handle.
+ * Note: If a device is not added to the system yet, there won't be an
+ * acpi_device struct for it. So do not get HID and CID from acpi_device,
+ * get them from ACPI namespace directly.
  */
 int acpi_is_root_bridge(acpi_handle handle)
 {
-	int ret;
-	struct acpi_device *device;
+	struct acpi_device_info *info;
+	acpi_status status;
 
-	ret = acpi_bus_get_device(handle, &device);
-	if (ret)
+	status = acpi_get_object_info(handle, &info);
+	if (ACPI_FAILURE(status)) {
+		printk(KERN_ERR PREFIX "%s: Error reading"
+				       "device info\n", __func__);
 		return 0;
+	}
 
-	ret = acpi_match_device_ids(device, root_device_ids);
-	if (ret)
+	if (acpi_match_object_info_ids(info, root_device_ids))
 		return 0;
 	else
 		return 1;
-- 
1.7.1

Re: [PATCH 2/3 v2] Do not use acpi_device to find pci root bridge in _init code.

[Yinghai Lu]

On Fri, Oct 12, 2012 at 5:34 AM, Tang Chen <tangchen@cn.fujitsu.com> wrote:
> When the kernel is being initialized, and some hardwares are not added
> to system, there won't be acpi_device structs for these devices. But
> acpi_is_root_bridge() depends on acpi_device struct. As a result, all
> the not-added root bridge will not be judged as a root bridge in
> find_root_bridges(). And further more, no handle_hotplug_event_root()
> notifier will be installed for them.
>
> This patch introduces a new api to find all root bridges in system by
> getting HID directly from ACPI namespace, not depending on acpi_device
> struct.
>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Liu Jiang <jiang.liu@huawei.com>
> ---
>  drivers/acpi/pci_root.c |   19 +++++++++++--------
>  1 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6151d83..582eb11 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -129,20 +129,23 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
>   * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
>   * @handle - the ACPI CA node in question.
>   *
> - * Note: we could make this API take a struct acpi_device * instead, but
> - * for now, it's more convenient to operate on an acpi_handle.
> + * Note: If a device is not added to the system yet, there won't be an
> + * acpi_device struct for it. So do not get HID and CID from acpi_device,
> + * get them from ACPI namespace directly.
>   */
>  int acpi_is_root_bridge(acpi_handle handle)
>  {
> -       int ret;
> -       struct acpi_device *device;
> +       struct acpi_device_info *info;
> +       acpi_status status;
>
> -       ret = acpi_bus_get_device(handle, &device);
> -       if (ret)
> +       status = acpi_get_object_info(handle, &info);
> +       if (ACPI_FAILURE(status)) {
> +               printk(KERN_ERR PREFIX "%s: Error reading"
> +                                      "device info\n", __func__);
>                 return 0;
> +       }
>
> -       ret = acpi_match_device_ids(device, root_device_ids);
> -       if (ret)
> +       if (acpi_match_object_info_ids(info, root_device_ids))
>                 return 0;
>         else
>                 return 1;

there are some other users for acpi_is_root_bridge.

drivers/acpi/pci_root.c:        while (!acpi_is_root_bridge(phandle)) {
drivers/pci/hotplug/acpi_pcihp.c:               if (acpi_is_root_bridge(handle))
drivers/pci/hotplug/acpi_pcihp.c:               if (acpi_is_root_bridge(handle))

and they are ok to have acpi_device ready.

so we could have another separated static version for those not added ones?

Thanks

Yinghai

Re: [PATCH 2/3 v2] Do not use acpi_device to find pci root bridge in _init code.

[Taku Izumi]

On Fri, 12 Oct 2012 20:34:20 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> When the kernel is being initialized, and some hardwares are not added
> to system, there won't be acpi_device structs for these devices. But
> acpi_is_root_bridge() depends on acpi_device struct. As a result, all
> the not-added root bridge will not be judged as a root bridge in
> find_root_bridges(). And further more, no handle_hotplug_event_root()
> notifier will be installed for them.
> 
> This patch introduces a new api to find all root bridges in system by
> getting HID directly from ACPI namespace, not depending on acpi_device
> struct.

  How about squashing patch #2 into patch #1 ?
  The caller and callee should be the same place in my mind.

  Best regards,
  Taku Izumi

> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> Signed-off-by: Liu Jiang <jiang.liu@huawei.com>
> ---
>  drivers/acpi/pci_root.c |   19 +++++++++++--------
>  1 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6151d83..582eb11 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -129,20 +129,23 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
>   * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
>   * @handle - the ACPI CA node in question.
>   *
> - * Note: we could make this API take a struct acpi_device * instead, but
> - * for now, it's more convenient to operate on an acpi_handle.
> + * Note: If a device is not added to the system yet, there won't be an
> + * acpi_device struct for it. So do not get HID and CID from acpi_device,
> + * get them from ACPI namespace directly.
>   */
>  int acpi_is_root_bridge(acpi_handle handle)
>  {
> -	int ret;
> -	struct acpi_device *device;
> +	struct acpi_device_info *info;
> +	acpi_status status;
>  
> -	ret = acpi_bus_get_device(handle, &device);
> -	if (ret)
> +	status = acpi_get_object_info(handle, &info);
> +	if (ACPI_FAILURE(status)) {
> +		printk(KERN_ERR PREFIX "%s: Error reading"
> +				       "device info\n", __func__);
>  		return 0;
> +	}
>  
> -	ret = acpi_match_device_ids(device, root_device_ids);
> -	if (ret)
> +	if (acpi_match_object_info_ids(info, root_device_ids))
>  		return 0;
>  	else
>  		return 1;
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Taku Izumi <izumi.taku@jp.fujitsu.com>

[PATCH 3/3 v2] Check exit status of acpi_install_notify_handler() in find_root_bridges().

[Tang Chen]

acpi_install_notify_handler() could fail. So check the exit status
and give a better debug info.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/pci_root_hp.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_root_hp.c b/drivers/acpi/pci_root_hp.c
index 1f60533..4c18573 100644
--- a/drivers/acpi/pci_root_hp.c
+++ b/drivers/acpi/pci_root_hp.c
@@ -242,6 +242,7 @@ static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 static acpi_status __init
 find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
+	acpi_status status;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
@@ -254,9 +255,14 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 
-	acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
-				handle_hotplug_event_root, NULL);
-	printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
+	status = acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+					handle_hotplug_event_root, NULL);
+	if (ACPI_FAILURE(status))
+		printk(KERN_DEBUG "acpi root: %s notify handler not installed\n"
+				  "acpi root: exit status: %u\n",
+				  objname, (unsigned int)status);
+	else
+		printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
 
 	add_acpi_root_bridge(handle);
 
-- 
1.7.1