[PATCH v2 0/2] Add acpi_dev_present

[PATCH v2 0/2] Add acpi_dev_present

[Lukas Wunner]

Hi,

On Tue, Nov 24, 2015 at 12:40:51 +0800, Hanjun Guo wrote:
> I think those IDs already cached, in acpi_init_device_object(),
> INIT_LIST_HEAD(&device->pnp.ids);
> ...
> acpi_set_pnp_ids(handle, &device->pnp, type);
> please see API acpi_device_hid(), so I think you can introduce a API with
> acpi_device and HID passed as arguments in scan.c

Thank you for your feedback, I've reworked the patch to iterate over
acpi_bus_id_list instead of walking the namespace.

An alternative approach would have been to traverse the acpi_device
tree but it would have been more code. If you would prefer that over
the solution I've settled on please let me know. It would be useful
for stuff like this to have a function that traverses the acpi_device
tree and invokes a callback for each node, à la acpi_ns_walk_namespace().
Does such a thing exist already? If so I've missed it.


> Will those drivers be loaded before the acpi namespace is scanned?

I verified that all 7 existing users invoke the new API from initcall
level "device". The bus is scanned in initcall level "subsystem",
i.e. earlier. So we're on the safe side here. I added a note in the
kernel-doc that the function may not be called from a subsys_initcall()
or earlier, this will hopefully make it foolproof. (If you feel this is
over the top then just leave it out if/when merging.)


On Tue, Nov 24, 2015 at 15:22:18 +0100, Rafael J. Wysocki wrote:
> I'd prefer that to go to utils.c to be honest, even if the namespace
> needs to be walked.

Done. Since this is not part of ACPICA, would it be okay to merge via
ASoC as requested by Mark Brown?

I've pushed the new version of the series to GitHub for more convenient
reviewing. All driver patches except the one for Mark have acks now:
https://github.com/l1k/linux/commits/acpi_dev_present

Thanks,

Lukas


Lukas Wunner (2):
  ACPI / scan: Fix acpi_bus_id_list bookkeeping
  ACPI / utils: Add acpi_dev_present()

 drivers/acpi/internal.h |  8 ++++++++
 drivers/acpi/scan.c     | 22 +++++++++++++++-------
 drivers/acpi/utils.c    | 31 +++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  2 ++
 4 files changed, 56 insertions(+), 7 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] ACPI / utils: Add acpi_dev_present()

[Lukas Wunner]

There's an idiom in use by 7 Linux drivers to detect the presence of a
particular ACPI HID by walking the namespace with acpi_get_devices().
The callback passed to acpi_get_devices() is mostly identical across
the drivers, leading to lots of duplicate code.

Add acpi_dev_present(), the ACPI equivalent to pci_dev_present(),
allowing us to deduplicate all that boilerplate in the drivers.

v2: * Instead of walking the namespace, iterate over acpi_bus_id_list
      which has all HIDs cached (Hanjun Guo, Robert Moore).
    * Move to utils.c (Rafael J. Wysocki). This necessitates non-static
      declaration of acpi_bus_id_list.

Cc: Hanjun Guo <guohanjun@huawei.com>
Cc: Robert Moore <robert.moore@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/acpi/internal.h |  8 ++++++++
 drivers/acpi/scan.c     |  8 +-------
 drivers/acpi/utils.c    | 31 +++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  2 ++
 4 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 11d87bf..60bda0d 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -86,6 +86,14 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent);
 #define ACPI_STA_DEFAULT (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED | \
 			  ACPI_STA_DEVICE_UI | ACPI_STA_DEVICE_FUNCTIONING)
 
+extern struct list_head acpi_bus_id_list;
+
+struct acpi_device_bus_id{
+	char bus_id[15];
+	unsigned int instance_no;
+	struct list_head node;
+};
+
 int acpi_device_add(struct acpi_device *device,
 		    void (*release)(struct device *));
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a0e942b..4be2179 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -39,7 +39,7 @@ static const char *dummy_hid = "device";
 
 static LIST_HEAD(acpi_dep_list);
 static DEFINE_MUTEX(acpi_dep_list_lock);
-static LIST_HEAD(acpi_bus_id_list);
+LIST_HEAD(acpi_bus_id_list);
 static DEFINE_MUTEX(acpi_scan_lock);
 static LIST_HEAD(acpi_scan_handlers_list);
 DEFINE_MUTEX(acpi_device_lock);
@@ -52,12 +52,6 @@ struct acpi_dep_data {
 	acpi_handle slave;
 };
 
-struct acpi_device_bus_id{
-	char bus_id[15];
-	unsigned int instance_no;
-	struct list_head node;
-};
-
 void acpi_scan_lock_acquire(void)
 {
 	mutex_lock(&acpi_scan_lock);
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 475c907..f2f9873 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -29,6 +29,7 @@
 #include <linux/dynamic_debug.h>
 
 #include "internal.h"
+#include "sleep.h"
 
 #define _COMPONENT		ACPI_BUS_COMPONENT
 ACPI_MODULE_NAME("utils");
@@ -709,6 +710,36 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs)
 }
 EXPORT_SYMBOL(acpi_check_dsm);
 
+/**
+ * acpi_dev_present - Detect presence of a given ACPI device in the system.
+ * @hid: Hardware ID of the device.
+ *
+ * Return %true if the device was present at the moment of invocation.
+ * Note that if the device is pluggable, it may since have disappeared.
+ *
+ * For this function to work, acpi_bus_scan() must have been executed
+ * which happens in the subsys_initcall() subsection. Hence, do not
+ * call from a subsys_initcall() or earlier (use acpi_get_devices()
+ * instead). Calling from module_init() is fine (which is synonymous
+ * with device_initcall()).
+ */
+bool acpi_dev_present(const char *hid)
+{
+	struct acpi_device_bus_id *acpi_device_bus_id;
+	bool found = false;
+
+	mutex_lock(&acpi_device_lock);
+	list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node)
+		if (!strcmp(acpi_device_bus_id->bus_id, hid)) {
+			found = true;
+			break;
+		}
+	mutex_unlock(&acpi_device_lock);
+
+	return found;
+}
+EXPORT_SYMBOL(acpi_dev_present);
+
 /*
  * acpi_backlight= handling, this is done here rather then in video_detect.c
  * because __setup cannot be used in modules.
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ad0a5ff..0fe7bab 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -87,6 +87,8 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, int rev, int func,
 	  .package.elements = (eles)			\
 	}
 
+bool acpi_dev_present(const char *hid);
+
 #ifdef CONFIG_ACPI
 
 #include <linux/proc_fs.h>
-- 
1.8.5.2 (Apple Git-48)

Re: [PATCH v2 2/2] ACPI / utils: Add acpi_dev_present()

[Hanjun Guo]

On 2015/11/26 4:19, Lukas Wunner wrote:
> There's an idiom in use by 7 Linux drivers to detect the presence of a
> particular ACPI HID by walking the namespace with acpi_get_devices().
> The callback passed to acpi_get_devices() is mostly identical across
> the drivers, leading to lots of duplicate code.
>
> Add acpi_dev_present(), the ACPI equivalent to pci_dev_present(),
> allowing us to deduplicate all that boilerplate in the drivers.
>
> v2: * Instead of walking the namespace, iterate over acpi_bus_id_list
>       which has all HIDs cached (Hanjun Guo, Robert Moore).
>     * Move to utils.c (Rafael J. Wysocki). This necessitates non-static
>       declaration of acpi_bus_id_list.
>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Robert Moore <robert.moore@intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/acpi/internal.h |  8 ++++++++
>  drivers/acpi/scan.c     |  8 +-------
>  drivers/acpi/utils.c    | 31 +++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |  2 ++
>  4 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 11d87bf..60bda0d 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -86,6 +86,14 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent);
>  #define ACPI_STA_DEFAULT (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED | \
>  			  ACPI_STA_DEVICE_UI | ACPI_STA_DEVICE_FUNCTIONING)
>  
> +extern struct list_head acpi_bus_id_list;
> +
> +struct acpi_device_bus_id{
> +	char bus_id[15];
> +	unsigned int instance_no;
> +	struct list_head node;
> +};
> +
>  int acpi_device_add(struct acpi_device *device,
>  		    void (*release)(struct device *));
>  void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index a0e942b..4be2179 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -39,7 +39,7 @@ static const char *dummy_hid = "device";
>  
>  static LIST_HEAD(acpi_dep_list);
>  static DEFINE_MUTEX(acpi_dep_list_lock);
> -static LIST_HEAD(acpi_bus_id_list);
> +LIST_HEAD(acpi_bus_id_list);
>  static DEFINE_MUTEX(acpi_scan_lock);
>  static LIST_HEAD(acpi_scan_handlers_list);
>  DEFINE_MUTEX(acpi_device_lock);
> @@ -52,12 +52,6 @@ struct acpi_dep_data {
>  	acpi_handle slave;
>  };
>  
> -struct acpi_device_bus_id{
> -	char bus_id[15];
> -	unsigned int instance_no;
> -	struct list_head node;
> -};
> -
>  void acpi_scan_lock_acquire(void)
>  {
>  	mutex_lock(&acpi_scan_lock);
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 475c907..f2f9873 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -29,6 +29,7 @@
>  #include <linux/dynamic_debug.h>
>  
>  #include "internal.h"
> +#include "sleep.h"
>  
>  #define _COMPONENT		ACPI_BUS_COMPONENT
>  ACPI_MODULE_NAME("utils");
> @@ -709,6 +710,36 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs)
>  }
>  EXPORT_SYMBOL(acpi_check_dsm);
>  
> +/**
> + * acpi_dev_present - Detect presence of a given ACPI device in the system.
> + * @hid: Hardware ID of the device.
> + *
> + * Return %true if the device was present at the moment of invocation.
> + * Note that if the device is pluggable, it may since have disappeared.
> + *
> + * For this function to work, acpi_bus_scan() must have been executed
> + * which happens in the subsys_initcall() subsection. Hence, do not
> + * call from a subsys_initcall() or earlier (use acpi_get_devices()
> + * instead). Calling from module_init() is fine (which is synonymous
> + * with device_initcall()).
> + */
> +bool acpi_dev_present(const char *hid)

If the driver can't pass the "dev" as the argument form acpi device or platform
device, this is the way to match HIDs.

But if we get the pointer to acpi device or platform device structure before calling
this function, I think acpi_match_device_ids() can match what you need, and we don't
need to invent another function.

Thanks
Hanjun

Re: [PATCH v2 2/2] ACPI / utils: Add acpi_dev_present()

[Lukas Wunner]

Hi Hanjun Guo,

thank you for taking a look at the patches.

On Mon, Nov 30, 2015 at 04:27:23PM +0800, Hanjun Guo wrote:
> If the driver can't pass the "dev" as the argument form acpi device or
> platform device, this is the way to match HIDs.
> 
> But if we get the pointer to acpi device or platform device structure
> before calling this function, I think acpi_match_device_ids() can match
> what you need, and we don't need to invent another function.

Of the 7 users,

* 4 detect the presence of a particular HID to activate platform-
  specific quirks (acer-wmi.c, eeepc-wmi.c, thinkpad_helper.c,
  cht_bsw_max98090_ti.c). The drivers never obtain a pointer to the
  struct acpi_device. This is also the reason why I need to detect
  the presence of apple-gmux in several DRM drivers, to activate
  quirks specific to MacBook Pros with dual GPUs, and the one thing
  they have in common is the presence of the gmux controller.
  The DRM drivers likewise never have a need to hold a pointer to
  apple-gmux' struct acpi_device.

* 2 detect the presence of a particular HID and *afterwards*
  instantiate a platform_device (atom/sst/sst_acpi.c and
  common/sst-acpi.c). So we can't use acpi_match_device_ids()
  there either.

* 1 is a driver for a platform_device (cht_bsw_rt5645.c) which was
  instantiated by atom/sst/sst_acpi.c. The driver is responsible
  for two chips and differentiates between the two by detecting the
  presence of a particular HID. It would be possible to refactor the
  code so that atom/sst/sst_acpi.c passes down the matched HID to
  cht_bsw_rt5645.c, then it wouldn't be necessary to match for a
  second time. Also, the only difference between the two chipsets
  seems to be a minute change in struct snd_soc_dapm_route, so I'm
  wondering if it's necessary to differentiate between the chipsets
  at all. Mark Brown may want to poke the developers of the driver to
  refactor the code.

TL;DR: 6 of the 7 users can't use acpi_match_device_ids() and the
7th could be refactored so that it doesn't have to detect the presence
of a specific HID at all.

Best regards,

Lukas

Re: [PATCH v2 2/2] ACPI / utils: Add acpi_dev_present()

[Hanjun Guo]

On 2015/12/1 20:58, Lukas Wunner wrote:
> Hi Hanjun Guo,
>
> thank you for taking a look at the patches.
>
> On Mon, Nov 30, 2015 at 04:27:23PM +0800, Hanjun Guo wrote:
>> If the driver can't pass the "dev" as the argument form acpi device or
>> platform device, this is the way to match HIDs.
>>
>> But if we get the pointer to acpi device or platform device structure
>> before calling this function, I think acpi_match_device_ids() can match
>> what you need, and we don't need to invent another function.
> Of the 7 users,
>
> * 4 detect the presence of a particular HID to activate platform-
>   specific quirks (acer-wmi.c, eeepc-wmi.c, thinkpad_helper.c,
>   cht_bsw_max98090_ti.c). The drivers never obtain a pointer to the
>   struct acpi_device. This is also the reason why I need to detect
>   the presence of apple-gmux in several DRM drivers, to activate
>   quirks specific to MacBook Pros with dual GPUs, and the one thing
>   they have in common is the presence of the gmux controller.
>   The DRM drivers likewise never have a need to hold a pointer to
>   apple-gmux' struct acpi_device.
>
> * 2 detect the presence of a particular HID and *afterwards*
>   instantiate a platform_device (atom/sst/sst_acpi.c and
>   common/sst-acpi.c). So we can't use acpi_match_device_ids()
>   there either.
>
> * 1 is a driver for a platform_device (cht_bsw_rt5645.c) which was
>   instantiated by atom/sst/sst_acpi.c. The driver is responsible
>   for two chips and differentiates between the two by detecting the
>   presence of a particular HID. It would be possible to refactor the
>   code so that atom/sst/sst_acpi.c passes down the matched HID to
>   cht_bsw_rt5645.c, then it wouldn't be necessary to match for a
>   second time. Also, the only difference between the two chipsets
>   seems to be a minute change in struct snd_soc_dapm_route, so I'm
>   wondering if it's necessary to differentiate between the chipsets
>   at all. Mark Brown may want to poke the developers of the driver to
>   refactor the code.
>
> TL;DR: 6 of the 7 users can't use acpi_match_device_ids() and the
> 7th could be refactored so that it doesn't have to detect the presence
> of a specific HID at all.

Thanks for the clarify, in this case, this patch makes sense to me.

Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>

Hanjun

[PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping

[Lukas Wunner]

acpi_device_add() allocates and adds an element to acpi_bus_id_list
(or increments the instance count if the device's HID is already
present in the list), but the element is never deleted from the list
nor freed. Fix it.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/acpi/scan.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 78d5f02..a0e942b 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -471,10 +471,24 @@ static void acpi_device_release(struct device *dev)
 
 static void acpi_device_del(struct acpi_device *device)
 {
+	struct acpi_device_bus_id *acpi_device_bus_id;
+
 	mutex_lock(&acpi_device_lock);
 	if (device->parent)
 		list_del(&device->node);
 
+	list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node)
+		if (!strcmp(acpi_device_bus_id->bus_id,
+			    acpi_device_hid(device))) {
+			if (acpi_device_bus_id->instance_no > 0)
+				acpi_device_bus_id->instance_no--;
+			else {
+				list_del(&acpi_device_bus_id->node);
+				kfree(acpi_device_bus_id);
+			}
+			break;
+		}
+
 	list_del(&device->wakeup_list);
 	mutex_unlock(&acpi_device_lock);
 
-- 
1.8.5.2 (Apple Git-48)

Re: [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping

[Hanjun Guo]

Hi Lucas,

Sorry for the late reply, please see comments below.

On 2015/11/26 4:19, Lukas Wunner wrote:
> acpi_device_add() allocates and adds an element to acpi_bus_id_list
> (or increments the instance count if the device's HID is already
> present in the list), but the element is never deleted from the list
> nor freed. Fix it.

Hmm, I didn't get it here. Seems the device's ID already freed in device core:

 In acpi_add_single_object(), acpi_device_release() registered as a callback,
...
  result = acpi_device_add(device, acpi_device_release);
...

And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
IDs, did I miss some something?

Thanks
Hanjun

Re: [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping

[Hanjun Guo]

On 2015/11/30 14:27, Hanjun Guo wrote:
> Hi Lucas,
>
> Sorry for the late reply, please see comments below.
>
> On 2015/11/26 4:19, Lukas Wunner wrote:
>> acpi_device_add() allocates and adds an element to acpi_bus_id_list
>> (or increments the instance count if the device's HID is already
>> present in the list), but the element is never deleted from the list
>> nor freed. Fix it.
> Hmm, I didn't get it here. Seems the device's ID already freed in device core:
>
>  In acpi_add_single_object(), acpi_device_release() registered as a callback,
> ...
>   result = acpi_device_add(device, acpi_device_release);
> ...
>
> And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
> IDs, did I miss some something?

Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device,
actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise.

Hanjun

Re: [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping

[Lukas Wunner]

Hi,

On Mon, Nov 30, 2015 at 04:14:32PM +0800, Hanjun Guo wrote:
> On 2015/11/30 14:27, Hanjun Guo wrote:
> > On 2015/11/26 4:19, Lukas Wunner wrote:
> >> acpi_device_add() allocates and adds an element to acpi_bus_id_list
> >> (or increments the instance count if the device's HID is already
> >> present in the list), but the element is never deleted from the list
> >> nor freed. Fix it.
> > Hmm, I didn't get it here. Seems the device's ID already freed in device core:
> >
> >  In acpi_add_single_object(), acpi_device_release() registered as a callback,
> > ...
> >   result = acpi_device_add(device, acpi_device_release);
> > ...
> >
> > And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
> > IDs, did I miss some something?
> 
> Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device,
> actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise.

Yes, I should have been more clear about this in the first place:

When the bus is scanned and acpi_device_add() is called for each device,
not only do we initialize a struct acpi_device and attach it to the
device tree, but we also add an element to acpi_bus_id_list.

Hence there are two ways to detect the presence of a HID: By traversing
the device tree or by iterating over the list. I chose the latter because
it is obviously cheaper and requires less code.

However elements only ever get added to the list, never deleted. I'm not
sure if hotpluggable ACPI devices exist but if they do, this is a bug
which is fixed by this patch.

The list was added with e49bd2dd5a50 ("ACPI: use PNPID:instance_no as
bus_id of ACPI device") 9 years ago and the author even mentioned that
the list elements are never freed:

   "NOTE:       Now I don't take the memory free work in charge.
        If necessary, I can add a reference count in
        struct acpi_device_bus_id, and check the reference and
        when unregister a device, i.e. memory is freed when
        the reference count of a given PNPID is 0."

Best regards,

Lukas

Re: [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping

[Hanjun Guo]

On 2015/12/1 21:08, Lukas Wunner wrote:
> Hi,
>
> On Mon, Nov 30, 2015 at 04:14:32PM +0800, Hanjun Guo wrote:
>> On 2015/11/30 14:27, Hanjun Guo wrote:
>>> On 2015/11/26 4:19, Lukas Wunner wrote:
>>>> acpi_device_add() allocates and adds an element to acpi_bus_id_list
>>>> (or increments the instance count if the device's HID is already
>>>> present in the list), but the element is never deleted from the list
>>>> nor freed. Fix it.
>>> Hmm, I didn't get it here. Seems the device's ID already freed in device core:
>>>
>>>  In acpi_add_single_object(), acpi_device_release() registered as a callback,
>>> ...
>>>   result = acpi_device_add(device, acpi_device_release);
>>> ...
>>>
>>> And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
>>> IDs, did I miss some something?
>> Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device,
>> actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise.
> Yes, I should have been more clear about this in the first place:
>
> When the bus is scanned and acpi_device_add() is called for each device,
> not only do we initialize a struct acpi_device and attach it to the
> device tree, but we also add an element to acpi_bus_id_list.
>
> Hence there are two ways to detect the presence of a HID: By traversing
> the device tree or by iterating over the list. I chose the latter because
> it is obviously cheaper and requires less code.
>
> However elements only ever get added to the list, never deleted. I'm not
> sure if hotpluggable ACPI devices exist but if they do, this is a bug
> which is fixed by this patch.

ACPI devices can be hotpluggable :) , but it will have no memory leak I think, it
only increase the instance number for ACPI devices removed and hot-added later,
I don't know if it make sense to do that, for example, if you remove device A and
B with same HID (such as ACPI0007) with your patch added:

remove processor 0, the sysfs for device A /sys/bus/acpi/devices/ACPI0007:00 will be
removed;

remove processer 1, the sysfs for device B /sys/bus/acpi/devices/ACPI0007:01 will be
removed;

But if we add it in reverse with your patch:

Add dprocesser 1, the sysfs /sys/bus/acpi/devices/ACPI0007:00 will be created,

add processor 0...

I'm not sure it will confuse user space or not.

Rafael, what's your opinion here?

Thanks
Hanjun

Re: [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping

[Lukas Wunner]

Hi Rafael,

a gentle ping, not sure if you just haven't had time to look at this
thread or if it's fallen between the cracks.

(Further comments from me below.)

On Wed, Dec 02, 2015 at 04:56:47PM +0800, Hanjun Guo wrote:
> On 2015/12/1 21:08, Lukas Wunner wrote:
> > Hi,
> >
> > On Mon, Nov 30, 2015 at 04:14:32PM +0800, Hanjun Guo wrote:
> >> On 2015/11/30 14:27, Hanjun Guo wrote:
> >>> On 2015/11/26 4:19, Lukas Wunner wrote:
> >>>> acpi_device_add() allocates and adds an element to acpi_bus_id_list
> >>>> (or increments the instance count if the device's HID is already
> >>>> present in the list), but the element is never deleted from the list
> >>>> nor freed. Fix it.
> >>> Hmm, I didn't get it here. Seems the device's ID already freed in device core:
> >>>
> >>>  In acpi_add_single_object(), acpi_device_release() registered as a callback,
> >>> ...
> >>>   result = acpi_device_add(device, acpi_device_release);
> >>> ...
> >>>
> >>> And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
> >>> IDs, did I miss some something?
> >> Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device,
> >> actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise.
> > Yes, I should have been more clear about this in the first place:
> >
> > When the bus is scanned and acpi_device_add() is called for each device,
> > not only do we initialize a struct acpi_device and attach it to the
> > device tree, but we also add an element to acpi_bus_id_list.
> >
> > Hence there are two ways to detect the presence of a HID: By traversing
> > the device tree or by iterating over the list. I chose the latter because
> > it is obviously cheaper and requires less code.
> >
> > However elements only ever get added to the list, never deleted. I'm not
> > sure if hotpluggable ACPI devices exist but if they do, this is a bug
> > which is fixed by this patch.
> 
> ACPI devices can be hotpluggable :) , but it will have no memory leak I think, it
> only increase the instance number for ACPI devices removed and hot-added later,
> I don't know if it make sense to do that, for example, if you remove device A and
> B with same HID (such as ACPI0007) with your patch added:
> 
> remove processor 0, the sysfs for device A /sys/bus/acpi/devices/ACPI0007:00 will be
> removed;
> 
> remove processer 1, the sysfs for device B /sys/bus/acpi/devices/ACPI0007:01 will be
> removed;
> 
> But if we add it in reverse with your patch:
> 
> Add dprocesser 1, the sysfs /sys/bus/acpi/devices/ACPI0007:00 will be created,
> 
> add processor 0...
> 
> I'm not sure it will confuse user space or not.
> 
> Rafael, what's your opinion here?

Yes, with this patch the instance number is no longer strictly increasing
and if devices are unplugged and replaced, instance numbers are recycled
for the newly plugged devices. I have no idea if this can break things.

If this patch is not acceptable, the semantics of patch [v2 2/2] change
in that acpi_dev_present() doesn't return the presence of a HID at the
time of invocation, but rather whether such a device has ever been present
since boot. It wouldn't be a problem because acpi_dev_present() is
meant for presence detection of devices which are built into the system
(same goes for the PCI counterpart pci_dev_present(), see the kerneldoc
in drivers/pci/search.c). I'd have to adjust the kerneldoc though to
reflect the changed semantics.

The other option would be to traverse the acpi_device tree instead of
iterating over acpi_bus_id_list, then it would be possible to retain
the semantics of detecting presence at the moment of invocation.

Please let me know which of these options makes the most sense to you
so that I can adjust the patches.

Thank you,

Lukas

> 
> Thanks
> Hanjun
> 

Re: [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping

[Rafael J. Wysocki]

On Sunday, December 06, 2015 10:09:19 PM Lukas Wunner wrote:
> Hi Rafael,

Hi,
 
> a gentle ping, not sure if you just haven't had time to look at this
> thread or if it's fallen between the cracks.

Neither of these.

I'm actually going to apply your patches.

> (Further comments from me below.)
>
> On Wed, Dec 02, 2015 at 04:56:47PM +0800, Hanjun Guo wrote:
> > On 2015/12/1 21:08, Lukas Wunner wrote:
> > > Hi,
> > >
> > > On Mon, Nov 30, 2015 at 04:14:32PM +0800, Hanjun Guo wrote:
> > >> On 2015/11/30 14:27, Hanjun Guo wrote:
> > >>> On 2015/11/26 4:19, Lukas Wunner wrote:
> > >>>> acpi_device_add() allocates and adds an element to acpi_bus_id_list
> > >>>> (or increments the instance count if the device's HID is already
> > >>>> present in the list), but the element is never deleted from the list
> > >>>> nor freed. Fix it.
> > >>> Hmm, I didn't get it here. Seems the device's ID already freed in device core:
> > >>>
> > >>>  In acpi_add_single_object(), acpi_device_release() registered as a callback,
> > >>> ...
> > >>>   result = acpi_device_add(device, acpi_device_release);
> > >>> ...
> > >>>
> > >>> And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
> > >>> IDs, did I miss some something?
> > >> Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device,
> > >> actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise.
> > > Yes, I should have been more clear about this in the first place:
> > >
> > > When the bus is scanned and acpi_device_add() is called for each device,
> > > not only do we initialize a struct acpi_device and attach it to the
> > > device tree, but we also add an element to acpi_bus_id_list.
> > >
> > > Hence there are two ways to detect the presence of a HID: By traversing
> > > the device tree or by iterating over the list. I chose the latter because
> > > it is obviously cheaper and requires less code.
> > >
> > > However elements only ever get added to the list, never deleted. I'm not
> > > sure if hotpluggable ACPI devices exist but if they do, this is a bug
> > > which is fixed by this patch.
> > 
> > ACPI devices can be hotpluggable :) , but it will have no memory leak I think, it
> > only increase the instance number for ACPI devices removed and hot-added later,
> > I don't know if it make sense to do that, for example, if you remove device A and
> > B with same HID (such as ACPI0007) with your patch added:
> > 
> > remove processor 0, the sysfs for device A /sys/bus/acpi/devices/ACPI0007:00 will be
> > removed;
> > 
> > remove processer 1, the sysfs for device B /sys/bus/acpi/devices/ACPI0007:01 will be
> > removed;
> > 
> > But if we add it in reverse with your patch:
> > 
> > Add dprocesser 1, the sysfs /sys/bus/acpi/devices/ACPI0007:00 will be created,
> > 
> > add processor 0...
> > 
> > I'm not sure it will confuse user space or not.
> > 
> > Rafael, what's your opinion here?
> 
> Yes, with this patch the instance number is no longer strictly increasing
> and if devices are unplugged and replaced, instance numbers are recycled
> for the newly plugged devices. I have no idea if this can break things.

Maybe.  I guess it may confuse user space on some systems if it cached
the old device names somehow, but since the instance numbers change right
now anyway, that is unlikely.

> If this patch is not acceptable, the semantics of patch [v2 2/2] change
> in that acpi_dev_present() doesn't return the presence of a HID at the
> time of invocation, but rather whether such a device has ever been present
> since boot. It wouldn't be a problem because acpi_dev_present() is
> meant for presence detection of devices which are built into the system
> (same goes for the PCI counterpart pci_dev_present(), see the kerneldoc
> in drivers/pci/search.c). I'd have to adjust the kerneldoc though to
> reflect the changed semantics.
> 
> The other option would be to traverse the acpi_device tree instead of
> iterating over acpi_bus_id_list, then it would be possible to retain
> the semantics of detecting presence at the moment of invocation.
> 
> Please let me know which of these options makes the most sense to you
> so that I can adjust the patches.

The simplest one that works for all of the users in question should be fine.

Thanks,
Rafael