[PATCH v6 0/5] ACPI: new acpi_dev_present helper + ac and battery blacklist patches

[PATCH v6 0/5] ACPI: new acpi_dev_present helper + ac and battery blacklist patches

[Hans de Goede]

Hi Rafael,

This is a new version of the requested resend of the latest version of
my acpi_dev_present helper + ac and battery blacklist patches, with
some changes to the first patch as requested by Andy Shevchenko.

This set also includes a power-supply patch, which depends on the
new acpi_dev_present helper and has an Ack from Sebastian Reichel
(the power-supply maintainer) for being merged through the ACPI
tree because of that dependency.

These patches obsolete:
-earlier versions of patches with the same title
-"ACPI: battery: Add acpi_battery_unregister() function"
-"ACPI: ac: Add acpi_ac_unregister() function"
-"power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply"
-"power: supply: axp288_charger: Unregister duplicate ACPI ac supply"

Thanks & Regards,

Hans

[PATCH v6 1/5] 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>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.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 reviewed-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
Changes in v5:
-Use s64 for hrv
-Fix grammar in comment
Changes in v6:
-Drop special handling of "0" uid argument, this was intended for
 compatiblity with the code in drivers/firmware/efi/dev-path-parser.c
 so that acpi_dev_present could replace that code, but it turns out that
 acpi_dev_present is not going to be suitable to replace this, so drop this
-Some minor style fixes to the code and commit msg
---
 drivers/acpi/utils.c    | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  1 +
 include/linux/acpi.h    |  5 ++++
 3 files changed, 72 insertions(+)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 22c0995..27d0dcf 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -736,6 +736,72 @@ 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;
+	s64 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->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 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, s64 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;
+}
+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..b53c058 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, s64 hrv);
 
 #ifdef CONFIG_ACPI
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 9b05886..841a8dc 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, s64 hrv)
+{
+	return false;
+}
+
 static inline bool is_acpi_node(struct fwnode_handle *fwnode)
 {
 	return false;
-- 
2.9.3

[PATCH v6 2/5] 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 v6 3/5] 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 v6 4/5] 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

[PATCH v6 5/5] power: supply: axp288_charger: Only wait for INT3496 device if present

[Hans de Goede]

On some devices with an axp288 pmic setting vbus path based on the
id-pin is handled by an ACPI _AIE interrupt on the gpio and the
INT3496 device is disabled.

Instead of returning -EPROBE_DEFER on these devices waiting for the
never to show up INT3496 device, check for its presence and only
request and monitor the matching extcon if the device is there,
otherwise let the firmware handle the vbus path control.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Sebastian Reichel <sre@kernel.org>
---
Changes in v2:
-Do not schedule otg.work to sync the id-pin state when we don't have
 the extcon, this avoids turning off charging as soon as the driver loads
 on devices without the extcon
---
 drivers/power/supply/axp288_charger.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 6be2fe2..d51ebd1 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -14,6 +14,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/regmap.h>
@@ -113,7 +114,8 @@
 #define ILIM_3000MA			3000	/* 3000mA */
 
 #define AXP288_EXTCON_DEV_NAME		"axp288_extcon"
-#define USB_HOST_EXTCON_DEV_NAME	"INT3496:00"
+#define USB_HOST_EXTCON_HID		"INT3496"
+#define USB_HOST_EXTCON_NAME		"INT3496:00"
 
 static const unsigned int cable_ids[] =
 	{ EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
@@ -807,10 +809,14 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
-	info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_DEV_NAME);
-	if (info->otg.cable == NULL) {
-		dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
-		return -EPROBE_DEFER;
+	if (acpi_dev_present(USB_HOST_EXTCON_HID, NULL, -1)) {
+		info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_NAME);
+		if (info->otg.cable == NULL) {
+			dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
+			return -EPROBE_DEFER;
+		}
+		dev_info(&pdev->dev,
+			 "Using " USB_HOST_EXTCON_HID " extcon for usb-id\n");
 	}
 
 	platform_set_drvdata(pdev, info);
@@ -849,13 +855,15 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	/* Register for OTG notification */
 	INIT_WORK(&info->otg.work, axp288_charger_otg_evt_worker);
 	info->otg.id_nb.notifier_call = axp288_charger_handle_otg_evt;
-	ret = devm_extcon_register_notifier(&pdev->dev, info->otg.cable,
+	if (info->otg.cable) {
+		ret = devm_extcon_register_notifier(&pdev->dev, info->otg.cable,
 					EXTCON_USB_HOST, &info->otg.id_nb);
-	if (ret) {
-		dev_err(dev, "failed to register EXTCON_USB_HOST notifier\n");
-		return ret;
+		if (ret) {
+			dev_err(dev, "failed to register EXTCON_USB_HOST notifier\n");
+			return ret;
+		}
+		schedule_work(&info->otg.work);
 	}
-	schedule_work(&info->otg.work);
 
 	/* Register charger interrupts */
 	for (i = 0; i < CHRG_INTR_END; i++) {
-- 
2.9.3

Re: [PATCH v6 3/5] ACPI: battery: Add a blacklist with PMIC ACPI HIDs with a native battery driver

[Andy Shevchenko]

On Tue, 2017-04-18 at 13:58 +0200, Hans de Goede wrote:
> 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.
> 

Some minor comments, sorry, didn't sent earlier.

Since they are minor, consider to change if some more serious stuff
comes up.


> +/* 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

Can we use ARRAY_SIZE() instead?

> +};
> +
>  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",

pr_info(PREFIX "%s: ...

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Andy Shevchenko]

On Tue, 2017-04-18 at 13:58 +0200, Hans de Goede wrote:
> 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.

Same remark, change those minors if something else comes up.

>  static int __init acpi_ac_init(void)
>  {
> -	int result;
> +	int i, result;

I would go with

unsigned int i;

(Same for previous patch considering ARRAY_SIZE() in use)

>  
>  	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",

Do we have PREFIX defined in this module as well?

> +				acpi_ac_blacklist[i].hid);
> +			return -ENODEV;
> +		}
> +

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v6 2/5] ACPI: battery: Fix acpi_battery_exit on acpi_battery_init_async errors

[Rafael J. Wysocki]

On Tue, Apr 18, 2017 at 1:58 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 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);

Maybe

      battery_driver_registered = !result;

>  }
>
>  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
>  }

Thanks,
Rafael

Re: [PATCH v6 2/5] ACPI: battery: Fix acpi_battery_exit on acpi_battery_init_async errors

[Hans de Goede]

Hi,

On 18-04-17 15:36, Rafael J. Wysocki wrote:
> On Tue, Apr 18, 2017 at 1:58 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> 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);
>
> Maybe
>
>       battery_driver_registered = !result;

I prefer == 0 here because we are checking for success (which is 0)
not for "not true".

Regards,

Hans



>
>>  }
>>
>>  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
>>  }
>
> Thanks,
> Rafael
>