[PATCH v4 1/4] ACPI: utils: Add new acpi_dev_present helper

[PATCH v4 1/4] ACPI: utils: Add new acpi_dev_present helper

[Hans de Goede]

acpi_dev_found just iterates over all ACPI-ids and sees if one matches.
This means that it will return true for devices which are in the dsdt
but disabled (their _STA method returns 0).

For some drivers it is useful to be able to check if a certain HID
is not only present in the namespace, but also actually present as in
acpi_device_is_present() will return true for the device. For example
because if a certain device is present then the driver will want to use
an extcon or IIO adc channel provided by that device.

This commit adds a new acpi_dev_present helper which drivers can use
to this end.

Like acpi_dev_found, acpi_dev_present take a HID as argument, but
it also has 2 extra optional arguments to only check for an ACPI
device with a specific UID and/or HRV value. This makes it more
generic and allows it to replace custom code doing similar checks
in several places.

Arguably acpi_dev_present is what acpi_dev_found should have been, but
there are too many users to just change acpi_dev_found without the risk
of breaking something.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Switch to using bus_find_device() to avoid "Traversing the namespace
 over and over"
-Add optional (may be NULL / -1) uid and hrv arguments, this will
 allow this new function to replace the custom code for this in
 drivers/firmware/efi/dev-path-parser.c as well as in
 sound/soc/intel/common/sst-match-acpi.c and will allow it to be
 used to implement blacklists to avoid loading the ACPI ac / battery
 driver on systems which have a PMIC / charger acpi device with a
 native driver which offers a better (often working vs not working)
 user experience
-Dropped Mika's reviewd by as this is almost a total rewrite
Changes in v3:
-memset the entire acpi_dev_present_info struct, this fixes
 acpi_device_id.cls not getting cleared
Changes in v4:
-Use empty initializer to zero the acpi_dev_present_info struct
---
 drivers/acpi/utils.c    | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  1 +
 include/linux/acpi.h    |  5 ++++
 3 files changed, 77 insertions(+)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 22c0995..ecd86a9 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid)
 }
 EXPORT_SYMBOL(acpi_dev_found);
 
+struct acpi_dev_present_info {
+	struct acpi_device_id hid[2];
+	const char *uid;
+	int hrv;
+};
+
+static int acpi_dev_present_cb(struct device *dev, void *data)
+{
+	struct acpi_device *adev = to_acpi_device(dev);
+	struct acpi_dev_present_info *match = data;
+	unsigned long long hrv;
+	acpi_status status;
+
+	if (acpi_match_device_ids(adev, match->hid))
+		return 0;
+
+	if (match->uid && adev->pnp.unique_id &&
+	    strcmp(adev->pnp.unique_id, match->uid))
+		return 0;
+
+	if (match->uid && !adev->pnp.unique_id &&
+	    strcmp("0", match->uid))
+		return 0;
+
+	if (match->hrv == -1)
+		return 1;
+
+	status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv);
+	if (ACPI_FAILURE(status))
+		return 0;
+
+	return hrv == match->hrv;
+}
+
+/**
+ * acpi_dev_present - Detect that a given ACPI device is present
+ * @hid: Hardware ID of the device.
+ * @uid: Unique ID of the device, pass "0" for devices without a _UID,
+ *       pass NULL to not check _UID
+ * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
+ *
+ * Return %true if a matching device was present at the moment of invocation.
+ * Note that if the device is pluggable, it may since have disappeared.
+ *
+ * Note that unlike acpi_dev_found() this function checks the status
+ * of the device so for devices which are present in the dsdt, but
+ * which are disabled (their _STA callback returns 0) this function
+ * will return false.
+ *
+ * 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, const char *uid, int hrv)
+{
+	struct acpi_dev_present_info match = {};
+	struct device *dev;
+
+	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
+	match.uid = uid;
+	match.hrv = hrv;
+
+	dev = bus_find_device(&acpi_bus_type, NULL, &match,
+			      acpi_dev_present_cb);
+
+	return dev ? true : false;
+}
+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 ef0ae8a..64498d5 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
 	}
 
 bool acpi_dev_found(const char *hid);
+bool acpi_dev_present(const char *hid, const char *uid, int hrv);
 
 #ifdef CONFIG_ACPI
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 9b05886..e5dd0f1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid)
 	return false;
 }
 
+static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv)
+{
+	return false;
+}
+
 static inline bool is_acpi_node(struct fwnode_handle *fwnode)
 {
 	return false;
-- 
2.9.3

[PATCH v4 2/4] ACPI: battery: Fix acpi_battery_exit on acpi_battery_init_async errors

[Hans de Goede]

The acpi_lock_battery_dir() / acpi_bus_register_driver() calls in
acpi_battery_init_async() may fail. Check that they succeeded before
undoing them.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/battery.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 4ef1e46..b35fca4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -67,6 +67,7 @@ MODULE_DESCRIPTION("ACPI Battery Driver");
 MODULE_LICENSE("GPL");
 
 static async_cookie_t async_cookie;
+static bool battery_driver_registered;
 static int battery_bix_broken_package;
 static int battery_notification_delay_ms;
 static unsigned int cache_time = 1000;
@@ -1329,6 +1330,7 @@ static void __init acpi_battery_init_async(void *unused, async_cookie_t cookie)
 	if (result < 0)
 		acpi_unlock_battery_dir(acpi_battery_dir);
 #endif
+	battery_driver_registered = (result == 0);
 }
 
 static int __init acpi_battery_init(void)
@@ -1343,9 +1345,11 @@ static int __init acpi_battery_init(void)
 static void __exit acpi_battery_exit(void)
 {
 	async_synchronize_cookie(async_cookie + 1);
-	acpi_bus_unregister_driver(&acpi_battery_driver);
+	if (battery_driver_registered)
+		acpi_bus_unregister_driver(&acpi_battery_driver);
 #ifdef CONFIG_ACPI_PROCFS_POWER
-	acpi_unlock_battery_dir(acpi_battery_dir);
+	if (acpi_battery_dir)
+		acpi_unlock_battery_dir(acpi_battery_dir);
 #endif
 }
 
-- 
2.9.3

[PATCH v4 3/4] ACPI: battery: Add a blacklist with PMIC ACPI HIDs with a native battery driver

[Hans de Goede]

On some systems we have a native PMIC driver which provides battery
monitoring, while the ACPI battery driver is broken on these systems
due to bad DSDTs or because we do not support the proprietary and
undocumented ACPI opregions these ACPI battery devices rely on
(e.g. BMOP opregion).

This leads to there being 2 battery power_supply-s registed like this:

~$ acpi
Battery 0: Charging, 84%, 00:49:39 until charged
Battery 1: Unknown, 0%, rate information unavailable

Even if the ACPI battery where to function fine (which on systems
where we have a native PMIC driver it often doesn't) we still do not
want to export the same battery to userspace twice.

This commit adds a blacklist with PMIC ACPI HIDs for which we've a
native battery driver and makes the ACPI battery driver not register
itself when a PMIC on this list is present.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set replacing:
 "ACPI: battery: Add acpi_battery_unregister() function"
 "power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply"
---
 drivers/acpi/battery.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index b35fca4..eacb816 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -94,6 +94,12 @@ static const struct acpi_device_id battery_device_ids[] = {
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
+/* Lists of PMIC ACPI HIDs with an (often better) native battery driver */
+static const char * const acpi_battery_blacklist[] = {
+	"INT33F4", /* X-Powers AXP288 PMIC */
+	NULL
+};
+
 enum {
 	ACPI_BATTERY_ALARM_PRESENT,
 	ACPI_BATTERY_XINFO_PRESENT,
@@ -1316,7 +1322,15 @@ static struct acpi_driver acpi_battery_driver = {
 
 static void __init acpi_battery_init_async(void *unused, async_cookie_t cookie)
 {
-	int result;
+	int i, result;
+
+	for (i = 0; acpi_battery_blacklist[i]; i++)
+		if (acpi_dev_present(acpi_battery_blacklist[i], "1", -1)) {
+			pr_info("ACPI: %s: found native %s PMIC, not loading\n",
+				ACPI_BATTERY_DEVICE_NAME,
+				acpi_battery_blacklist[i]);
+			return;
+		}
 
 	dmi_check_system(bat_dmi_table);
 
-- 
2.9.3

[PATCH v4 4/4] ACPI: ac: Add a blacklist with PMIC ACPI HIDs with a native charger driver

[Hans de Goede]

On some systems we have a native PMIC driver which provides Mains
monitoring, while the ACPI ac driver is broken on these systems
due to bad DSTDs or because we do not support the proprietary and
undocumented ACPI opregions these ACPI battery devices rely on
(e.g. BMOP opregion).

This leads for example to a ADP1 power_supply which reports
itself as always online even if no mains are connected.

This commit adds a blacklist with PMIC ACPI HIDs for which we've a
native charger or extcon driver and makes the ACPI ac driver not
register itself when a PMIC on this list is present.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set replacing:
 "ACPI: ac: Add acpi_ac_unregister() function"
 "power: supply: axp288_charger: Unregister duplicate ACPI ac supply"
 And also this patch from my local patch-queue (not submitted yet):
 "i2c-cht-wc: remove broken ACPI AC power-supply"
---
 drivers/acpi/ac.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index f71b756..8df95a1 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -57,12 +57,23 @@ static int acpi_ac_add(struct acpi_device *device);
 static int acpi_ac_remove(struct acpi_device *device);
 static void acpi_ac_notify(struct acpi_device *device, u32 event);
 
+struct acpi_ac_bl {
+	const char *hid;
+	int hrv;
+};
+
 static const struct acpi_device_id ac_device_ids[] = {
 	{"ACPI0003", 0},
 	{"", 0},
 };
 MODULE_DEVICE_TABLE(acpi, ac_device_ids);
 
+/* Lists of PMIC ACPI HIDs with an (often better) native charger driver */
+static const struct acpi_ac_bl acpi_ac_blacklist[] = {
+	{ "INT33F4", -1 }, /* X-Powers AXP288 PMIC */
+	{ "INT34D3",  3 }, /* Intel Cherrytrail Whiskey Cove PMIC */
+};
+
 #ifdef CONFIG_PM_SLEEP
 static int acpi_ac_resume(struct device *dev);
 #endif
@@ -424,11 +435,19 @@ static int acpi_ac_remove(struct acpi_device *device)
 
 static int __init acpi_ac_init(void)
 {
-	int result;
+	int i, result;
 
 	if (acpi_disabled)
 		return -ENODEV;
 
+	for (i = 0; i < ARRAY_SIZE(acpi_ac_blacklist); i++)
+		if (acpi_dev_present(acpi_ac_blacklist[i].hid, "1",
+				     acpi_ac_blacklist[i].hrv)) {
+			pr_info("ACPI: AC: found native %s PMIC, not loading\n",
+				acpi_ac_blacklist[i].hid);
+			return -ENODEV;
+		}
+
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	acpi_ac_dir = acpi_lock_ac_dir();
 	if (!acpi_ac_dir)
-- 
2.9.3

Re: [PATCH v4 1/4] ACPI: utils: Add new acpi_dev_present helper

[Lukas Wunner]

On Sat, Apr 08, 2017 at 11:48:27PM +0200, Hans de Goede wrote:
> acpi_dev_found just iterates over all ACPI-ids and sees if one matches.
> This means that it will return true for devices which are in the dsdt
> but disabled (their _STA method returns 0).
> 
> For some drivers it is useful to be able to check if a certain HID
> is not only present in the namespace, but also actually present as in
> acpi_device_is_present() will return true for the device. For example
> because if a certain device is present then the driver will want to use
> an extcon or IIO adc channel provided by that device.
> 
> This commit adds a new acpi_dev_present helper which drivers can use
> to this end.
> 
> Like acpi_dev_found, acpi_dev_present take a HID as argument, but
> it also has 2 extra optional arguments to only check for an ACPI
> device with a specific UID and/or HRV value. This makes it more
> generic and allows it to replace custom code doing similar checks
> in several places.
> 
> Arguably acpi_dev_present is what acpi_dev_found should have been, but
> there are too many users to just change acpi_dev_found without the risk
> of breaking something.
> 
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Switch to using bus_find_device() to avoid "Traversing the namespace
>  over and over"
> -Add optional (may be NULL / -1) uid and hrv arguments, this will
>  allow this new function to replace the custom code for this in
>  drivers/firmware/efi/dev-path-parser.c as well as in
>  sound/soc/intel/common/sst-match-acpi.c and will allow it to be
>  used to implement blacklists to avoid loading the ACPI ac / battery
>  driver on systems which have a PMIC / charger acpi device with a
>  native driver which offers a better (often working vs not working)
>  user experience
> -Dropped Mika's reviewd by as this is almost a total rewrite
> Changes in v3:
> -memset the entire acpi_dev_present_info struct, this fixes
>  acpi_device_id.cls not getting cleared
> Changes in v4:
> -Use empty initializer to zero the acpi_dev_present_info struct
> ---
>  drivers/acpi/utils.c    | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |  1 +
>  include/linux/acpi.h    |  5 ++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 22c0995..ecd86a9 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid)
>  }
>  EXPORT_SYMBOL(acpi_dev_found);
>  
> +struct acpi_dev_present_info {
> +	struct acpi_device_id hid[2];
> +	const char *uid;
> +	int hrv;
> +};

There's a (somewhat theoretical) issue on 32 bit arches with your choice
of int for hrv:  _HRV returns an "Integer (DWORD)" per the spec.  That's
32 bit and clashes with your use of -1 to signify "don't care" if the
arch uses 32 bit to represent an int.

So you need to use a "signed long long" or "s64" here.


> +
> +static int acpi_dev_present_cb(struct device *dev, void *data)
> +{
> +	struct acpi_device *adev = to_acpi_device(dev);
> +	struct acpi_dev_present_info *match = data;
> +	unsigned long long hrv;
> +	acpi_status status;
> +
> +	if (acpi_match_device_ids(adev, match->hid))
> +		return 0;
> +
> +	if (match->uid && adev->pnp.unique_id &&
> +	    strcmp(adev->pnp.unique_id, match->uid))
> +		return 0;
> +
> +	if (match->uid && !adev->pnp.unique_id &&
> +	    strcmp("0", match->uid))
> +		return 0;
> +
> +	if (match->hrv == -1)
> +		return 1;
> +
> +	status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv);
> +	if (ACPI_FAILURE(status))
> +		return 0;
> +
> +	return hrv == match->hrv;
> +}
> +
> +/**
> + * acpi_dev_present - Detect that a given ACPI device is present
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass "0" for devices without a _UID,
> + *       pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * Return %true if a matching device was present at the moment of invocation.
> + * Note that if the device is pluggable, it may since have disappeared.
> + *
> + * Note that unlike acpi_dev_found() this function checks the status
> + * of the device so for devices which are present in the dsdt, but
                   ^
Minor nit:  I think readability improves if you start a new sentence or
insert a comma here.  Sorry, just my grammar OCD.

Otherwise,
Reviewed-by: Lukas Wunner <lukas@wunner.de>

I'm clueless about ACPI battery or charging, so can't say anything about
the remainder of the series.

Note that the code in drivers/firmware/efi/dev-path-parser.c deliberately
increments the refcount of the device found and returns a pointer to it,
whereas acpi_dev_present() as introduced by this patch merely checks
presence and returns a bool.  So you won't be able to replace the code
in dev-path-parser.c with acpi_dev_present(), but I think that's
perfectly fine.

Thanks,

Lukas

> + * which are disabled (their _STA callback returns 0) this function
> + * will return false.
> + *
> + * 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, const char *uid, int hrv)
> +{
> +	struct acpi_dev_present_info match = {};
> +	struct device *dev;
> +
> +	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
> +	match.uid = uid;
> +	match.hrv = hrv;
> +
> +	dev = bus_find_device(&acpi_bus_type, NULL, &match,
> +			      acpi_dev_present_cb);
> +
> +	return dev ? true : false;
> +}
> +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 ef0ae8a..64498d5 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
>  	}
>  
>  bool acpi_dev_found(const char *hid);
> +bool acpi_dev_present(const char *hid, const char *uid, int hrv);
>  
>  #ifdef CONFIG_ACPI
>  
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 9b05886..e5dd0f1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid)
>  	return false;
>  }
>  
> +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv)
> +{
> +	return false;
> +}
> +
>  static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>  {
>  	return false;
> -- 
> 2.9.3

Re: [PATCH v4 1/4] ACPI: utils: Add new acpi_dev_present helper

[Hans de Goede]

Hi All,

Self-nack for patches 3-4 in this series, it turns out that
doing the global blacklist for the ACPI ac / battery is not a
good idea.

Some Bay Trail / Cherry Trail users are running kernels build
from my personal tree to get early access to various fixes
in there and I got a regression report on the DELL 5855, where
the blacklisting of the ACPI battery driver if INT33F4 is
present caused the battery monitoring to stop working, that
devices has an INT33F4 node with _STA returning 15 yet it
is not using an axp288 PMIC at all, I'm still gathering more
info, but I believe atm that Dell simply disabled the i2c
controller to which the axp288 would be connected if present
and left the other bits of the DTSD unmodified.

One option which comes to mind would be to only count devices
as present if all their deps are met, but that will only
work if the blacklist check is done after all other drivers
have loaded which is not how things work.

So I believe that my earlier attempts at fixing the double
power_supply registration by unregistering the ACPI one when
the native one has successfully loaded is better. That guarantees
regressions like this one will not happen, because the ACPI
power_supply does not get unregistered until the native one
has loaded.

Regards,

Hans



On Sat, Apr 08, 2017 at 11:48:27PM +0200, Hans de Goede wrote:
> acpi_dev_found just iterates over all ACPI-ids and sees if one matches.
> This means that it will return true for devices which are in the dsdt
> but disabled (their _STA method returns 0).
>
> For some drivers it is useful to be able to check if a certain HID
> is not only present in the namespace, but also actually present as in
> acpi_device_is_present() will return true for the device. For example
> because if a certain device is present then the driver will want to use
> an extcon or IIO adc channel provided by that device.
>
> This commit adds a new acpi_dev_present helper which drivers can use
> to this end.
>
> Like acpi_dev_found, acpi_dev_present take a HID as argument, but
> it also has 2 extra optional arguments to only check for an ACPI
> device with a specific UID and/or HRV value. This makes it more
> generic and allows it to replace custom code doing similar checks
> in several places.
>
> Arguably acpi_dev_present is what acpi_dev_found should have been, but
> there are too many users to just change acpi_dev_found without the risk
> of breaking something.
>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Switch to using bus_find_device() to avoid "Traversing the namespace
>  over and over"
> -Add optional (may be NULL / -1) uid and hrv arguments, this will
>  allow this new function to replace the custom code for this in
>  drivers/firmware/efi/dev-path-parser.c as well as in
>  sound/soc/intel/common/sst-match-acpi.c and will allow it to be
>  used to implement blacklists to avoid loading the ACPI ac / battery
>  driver on systems which have a PMIC / charger acpi device with a
>  native driver which offers a better (often working vs not working)
>  user experience
> -Dropped Mika's reviewd by as this is almost a total rewrite
> Changes in v3:
> -memset the entire acpi_dev_present_info struct, this fixes
>  acpi_device_id.cls not getting cleared
> Changes in v4:
> -Use empty initializer to zero the acpi_dev_present_info struct
> ---
>  drivers/acpi/utils.c    | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |  1 +
>  include/linux/acpi.h    |  5 ++++
>  3 files changed, 77 insertions(+)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 22c0995..ecd86a9 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid)
>  }
>  EXPORT_SYMBOL(acpi_dev_found);
>
> +struct acpi_dev_present_info {
> +	struct acpi_device_id hid[2];
> +	const char *uid;
> +	int hrv;
> +};
> +
> +static int acpi_dev_present_cb(struct device *dev, void *data)
> +{
> +	struct acpi_device *adev = to_acpi_device(dev);
> +	struct acpi_dev_present_info *match = data;
> +	unsigned long long hrv;
> +	acpi_status status;
> +
> +	if (acpi_match_device_ids(adev, match->hid))
> +		return 0;
> +
> +	if (match->uid && adev->pnp.unique_id &&
> +	    strcmp(adev->pnp.unique_id, match->uid))
> +		return 0;
> +
> +	if (match->uid && !adev->pnp.unique_id &&
> +	    strcmp("0", match->uid))
> +		return 0;
> +
> +	if (match->hrv == -1)
> +		return 1;
> +
> +	status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv);
> +	if (ACPI_FAILURE(status))
> +		return 0;
> +
> +	return hrv == match->hrv;
> +}
> +
> +/**
> + * acpi_dev_present - Detect that a given ACPI device is present
> + * @hid: Hardware ID of the device.
> + * @uid: Unique ID of the device, pass "0" for devices without a _UID,
> + *       pass NULL to not check _UID
> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
> + *
> + * Return %true if a matching device was present at the moment of invocation.
> + * Note that if the device is pluggable, it may since have disappeared.
> + *
> + * Note that unlike acpi_dev_found() this function checks the status
> + * of the device so for devices which are present in the dsdt, but
> + * which are disabled (their _STA callback returns 0) this function
> + * will return false.
> + *
> + * 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, const char *uid, int hrv)
> +{
> +	struct acpi_dev_present_info match = {};
> +	struct device *dev;
> +
> +	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
> +	match.uid = uid;
> +	match.hrv = hrv;
> +
> +	dev = bus_find_device(&acpi_bus_type, NULL, &match,
> +			      acpi_dev_present_cb);
> +
> +	return dev ? true : false;
> +}
> +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 ef0ae8a..64498d5 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
>  	}
>
>  bool acpi_dev_found(const char *hid);
> +bool acpi_dev_present(const char *hid, const char *uid, int hrv);
>
>  #ifdef CONFIG_ACPI
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 9b05886..e5dd0f1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid)
>  	return false;
>  }
>
> +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv)
> +{
> +	return false;
> +}
> +
>  static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>  {
>  	return false;
> --
> 2.9.3

Re: [PATCH v4 1/4] ACPI: utils: Add new acpi_dev_present helper

[Hans de Goede]

Hi,

On 10-04-17 09:23, Hans de Goede wrote:
> Hi All,
>
> Self-nack for patches 3-4 in this series, it turns out that
> doing the global blacklist for the ACPI ac / battery is not a
> good idea.
>
> Some Bay Trail / Cherry Trail users are running kernels build
> from my personal tree to get early access to various fixes
> in there and I got a regression report on the DELL 5855, where
> the blacklisting of the ACPI battery driver if INT33F4 is
> present caused the battery monitoring to stop working, that
> devices has an INT33F4 node with _STA returning 15 yet it
> is not using an axp288 PMIC at all, I'm still gathering more
> info, but I believe atm that Dell simply disabled the i2c
> controller to which the axp288 would be connected if present
> and left the other bits of the DTSD unmodified.
>
> One option which comes to mind would be to only count devices
> as present if all their deps are met, but that will only
> work if the blacklist check is done after all other drivers
> have loaded which is not how things work.
>
> So I believe that my earlier attempts at fixing the double
> power_supply registration by unregistering the ACPI one when
> the native one has successfully loaded is better. That guarantees
> regressions like this one will not happen, because the ACPI
> power_supply does not get unregistered until the native one
> has loaded.

Ok scrap that, I've some more info and the INT33F4 node on
the Dell 5855 is returning 0 from _STA and the blacklist is
causing problems on that machine for other reason, it could
be the user was using an older version with the uninitialized
.cls match info problem, I've asked the user to test the latest
version.

So assuming that does work, we are good to go with the blacklist
approach (which seems to be the solution everyone prefers),
sorry about the noise.

I will send a v5 of this patch addressing Lukas' comments,
I will re-submit patches 2-4 when I hear back from the Dell
5855 user.

Regards,

Hans



>
> Regards,
>
> Hans
>
>
>
> On Sat, Apr 08, 2017 at 11:48:27PM +0200, Hans de Goede wrote:
>> acpi_dev_found just iterates over all ACPI-ids and sees if one matches.
>> This means that it will return true for devices which are in the dsdt
>> but disabled (their _STA method returns 0).
>>
>> For some drivers it is useful to be able to check if a certain HID
>> is not only present in the namespace, but also actually present as in
>> acpi_device_is_present() will return true for the device. For example
>> because if a certain device is present then the driver will want to use
>> an extcon or IIO adc channel provided by that device.
>>
>> This commit adds a new acpi_dev_present helper which drivers can use
>> to this end.
>>
>> Like acpi_dev_found, acpi_dev_present take a HID as argument, but
>> it also has 2 extra optional arguments to only check for an ACPI
>> device with a specific UID and/or HRV value. This makes it more
>> generic and allows it to replace custom code doing similar checks
>> in several places.
>>
>> Arguably acpi_dev_present is what acpi_dev_found should have been, but
>> there are too many users to just change acpi_dev_found without the risk
>> of breaking something.
>>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: Lukas Wunner <lukas@wunner.de>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Switch to using bus_find_device() to avoid "Traversing the namespace
>>  over and over"
>> -Add optional (may be NULL / -1) uid and hrv arguments, this will
>>  allow this new function to replace the custom code for this in
>>  drivers/firmware/efi/dev-path-parser.c as well as in
>>  sound/soc/intel/common/sst-match-acpi.c and will allow it to be
>>  used to implement blacklists to avoid loading the ACPI ac / battery
>>  driver on systems which have a PMIC / charger acpi device with a
>>  native driver which offers a better (often working vs not working)
>>  user experience
>> -Dropped Mika's reviewd by as this is almost a total rewrite
>> Changes in v3:
>> -memset the entire acpi_dev_present_info struct, this fixes
>>  acpi_device_id.cls not getting cleared
>> Changes in v4:
>> -Use empty initializer to zero the acpi_dev_present_info struct
>> ---
>>  drivers/acpi/utils.c    | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/acpi/acpi_bus.h |  1 +
>>  include/linux/acpi.h    |  5 ++++
>>  3 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index 22c0995..ecd86a9 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid)
>>  }
>>  EXPORT_SYMBOL(acpi_dev_found);
>>
>> +struct acpi_dev_present_info {
>> +    struct acpi_device_id hid[2];
>> +    const char *uid;
>> +    int hrv;
>> +};
>> +
>> +static int acpi_dev_present_cb(struct device *dev, void *data)
>> +{
>> +    struct acpi_device *adev = to_acpi_device(dev);
>> +    struct acpi_dev_present_info *match = data;
>> +    unsigned long long hrv;
>> +    acpi_status status;
>> +
>> +    if (acpi_match_device_ids(adev, match->hid))
>> +        return 0;
>> +
>> +    if (match->uid && adev->pnp.unique_id &&
>> +        strcmp(adev->pnp.unique_id, match->uid))
>> +        return 0;
>> +
>> +    if (match->uid && !adev->pnp.unique_id &&
>> +        strcmp("0", match->uid))
>> +        return 0;
>> +
>> +    if (match->hrv == -1)
>> +        return 1;
>> +
>> +    status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv);
>> +    if (ACPI_FAILURE(status))
>> +        return 0;
>> +
>> +    return hrv == match->hrv;
>> +}
>> +
>> +/**
>> + * acpi_dev_present - Detect that a given ACPI device is present
>> + * @hid: Hardware ID of the device.
>> + * @uid: Unique ID of the device, pass "0" for devices without a _UID,
>> + *       pass NULL to not check _UID
>> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
>> + *
>> + * Return %true if a matching device was present at the moment of invocation.
>> + * Note that if the device is pluggable, it may since have disappeared.
>> + *
>> + * Note that unlike acpi_dev_found() this function checks the status
>> + * of the device so for devices which are present in the dsdt, but
>> + * which are disabled (their _STA callback returns 0) this function
>> + * will return false.
>> + *
>> + * 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, const char *uid, int hrv)
>> +{
>> +    struct acpi_dev_present_info match = {};
>> +    struct device *dev;
>> +
>> +    strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
>> +    match.uid = uid;
>> +    match.hrv = hrv;
>> +
>> +    dev = bus_find_device(&acpi_bus_type, NULL, &match,
>> +                  acpi_dev_present_cb);
>> +
>> +    return dev ? true : false;
>> +}
>> +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 ef0ae8a..64498d5 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
>>      }
>>
>>  bool acpi_dev_found(const char *hid);
>> +bool acpi_dev_present(const char *hid, const char *uid, int hrv);
>>
>>  #ifdef CONFIG_ACPI
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 9b05886..e5dd0f1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid)
>>      return false;
>>  }
>>
>> +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv)
>> +{
>> +    return false;
>> +}
>> +
>>  static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>>  {
>>      return false;
>> --
>> 2.9.3

Re: [PATCH v4 1/4] ACPI: utils: Add new acpi_dev_present helper

[Hans de Goede]

Hi,

On 09-04-17 10:16, Lukas Wunner wrote:
> On Sat, Apr 08, 2017 at 11:48:27PM +0200, Hans de Goede wrote:
>> acpi_dev_found just iterates over all ACPI-ids and sees if one matches.
>> This means that it will return true for devices which are in the dsdt
>> but disabled (their _STA method returns 0).
>>
>> For some drivers it is useful to be able to check if a certain HID
>> is not only present in the namespace, but also actually present as in
>> acpi_device_is_present() will return true for the device. For example
>> because if a certain device is present then the driver will want to use
>> an extcon or IIO adc channel provided by that device.
>>
>> This commit adds a new acpi_dev_present helper which drivers can use
>> to this end.
>>
>> Like acpi_dev_found, acpi_dev_present take a HID as argument, but
>> it also has 2 extra optional arguments to only check for an ACPI
>> device with a specific UID and/or HRV value. This makes it more
>> generic and allows it to replace custom code doing similar checks
>> in several places.
>>
>> Arguably acpi_dev_present is what acpi_dev_found should have been, but
>> there are too many users to just change acpi_dev_found without the risk
>> of breaking something.
>>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: Lukas Wunner <lukas@wunner.de>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Switch to using bus_find_device() to avoid "Traversing the namespace
>>  over and over"
>> -Add optional (may be NULL / -1) uid and hrv arguments, this will
>>  allow this new function to replace the custom code for this in
>>  drivers/firmware/efi/dev-path-parser.c as well as in
>>  sound/soc/intel/common/sst-match-acpi.c and will allow it to be
>>  used to implement blacklists to avoid loading the ACPI ac / battery
>>  driver on systems which have a PMIC / charger acpi device with a
>>  native driver which offers a better (often working vs not working)
>>  user experience
>> -Dropped Mika's reviewd by as this is almost a total rewrite
>> Changes in v3:
>> -memset the entire acpi_dev_present_info struct, this fixes
>>  acpi_device_id.cls not getting cleared
>> Changes in v4:
>> -Use empty initializer to zero the acpi_dev_present_info struct
>> ---
>>  drivers/acpi/utils.c    | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/acpi/acpi_bus.h |  1 +
>>  include/linux/acpi.h    |  5 ++++
>>  3 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
>> index 22c0995..ecd86a9 100644
>> --- a/drivers/acpi/utils.c
>> +++ b/drivers/acpi/utils.c
>> @@ -736,6 +736,77 @@ bool acpi_dev_found(const char *hid)
>>  }
>>  EXPORT_SYMBOL(acpi_dev_found);
>>
>> +struct acpi_dev_present_info {
>> +	struct acpi_device_id hid[2];
>> +	const char *uid;
>> +	int hrv;
>> +};
>
> There's a (somewhat theoretical) issue on 32 bit arches with your choice
> of int for hrv:  _HRV returns an "Integer (DWORD)" per the spec.  That's
> 32 bit and clashes with your use of -1 to signify "don't care" if the
> arch uses 32 bit to represent an int.
>
> So you need to use a "signed long long" or "s64" here.

Switched to s64 for v5.

>> +
>> +static int acpi_dev_present_cb(struct device *dev, void *data)
>> +{
>> +	struct acpi_device *adev = to_acpi_device(dev);
>> +	struct acpi_dev_present_info *match = data;
>> +	unsigned long long hrv;
>> +	acpi_status status;
>> +
>> +	if (acpi_match_device_ids(adev, match->hid))
>> +		return 0;
>> +
>> +	if (match->uid && adev->pnp.unique_id &&
>> +	    strcmp(adev->pnp.unique_id, match->uid))
>> +		return 0;
>> +
>> +	if (match->uid && !adev->pnp.unique_id &&
>> +	    strcmp("0", match->uid))
>> +		return 0;
>> +
>> +	if (match->hrv == -1)
>> +		return 1;
>> +
>> +	status = acpi_evaluate_integer(adev->handle, "_HRV", NULL, &hrv);
>> +	if (ACPI_FAILURE(status))
>> +		return 0;
>> +
>> +	return hrv == match->hrv;
>> +}
>> +
>> +/**
>> + * acpi_dev_present - Detect that a given ACPI device is present
>> + * @hid: Hardware ID of the device.
>> + * @uid: Unique ID of the device, pass "0" for devices without a _UID,
>> + *       pass NULL to not check _UID
>> + * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
>> + *
>> + * Return %true if a matching device was present at the moment of invocation.
>> + * Note that if the device is pluggable, it may since have disappeared.
>> + *
>> + * Note that unlike acpi_dev_found() this function checks the status
>> + * of the device so for devices which are present in the dsdt, but
>                    ^
> Minor nit:  I think readability improves if you start a new sentence or
> insert a comma here.  Sorry, just my grammar OCD.

Started a new sentence at "So for" for v5.

>
> Otherwise,
> Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thank you.

> I'm clueless about ACPI battery or charging, so can't say anything about
> the remainder of the series.
>
> Note that the code in drivers/firmware/efi/dev-path-parser.c deliberately
> increments the refcount of the device found and returns a pointer to it,
> whereas acpi_dev_present() as introduced by this patch merely checks
> presence and returns a bool.  So you won't be able to replace the code
> in dev-path-parser.c with acpi_dev_present(), but I think that's
> perfectly fine.

Ah, bummer, would have been nice to be able to re-use the code.

Regards,

Hans


>
> Thanks,
>
> Lukas
>
>> + * which are disabled (their _STA callback returns 0) this function
>> + * will return false.
>> + *
>> + * 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, const char *uid, int hrv)
>> +{
>> +	struct acpi_dev_present_info match = {};
>> +	struct device *dev;
>> +
>> +	strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id));
>> +	match.uid = uid;
>> +	match.hrv = hrv;
>> +
>> +	dev = bus_find_device(&acpi_bus_type, NULL, &match,
>> +			      acpi_dev_present_cb);
>> +
>> +	return dev ? true : false;
>> +}
>> +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 ef0ae8a..64498d5 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -88,6 +88,7 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
>>  	}
>>
>>  bool acpi_dev_found(const char *hid);
>> +bool acpi_dev_present(const char *hid, const char *uid, int hrv);
>>
>>  #ifdef CONFIG_ACPI
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 9b05886..e5dd0f1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -611,6 +611,11 @@ static inline bool acpi_dev_found(const char *hid)
>>  	return false;
>>  }
>>
>> +static inline bool acpi_dev_present(const char *hid, const char *uid, int hrv)
>> +{
>> +	return false;
>> +}
>> +
>>  static inline bool is_acpi_node(struct fwnode_handle *fwnode)
>>  {
>>  	return false;
>> --
>> 2.9.3