[PATCH 0/4] Avoid duplicate registering of ACPI and native power-supplies

[PATCH 0/4] Avoid duplicate registering of ACPI and native power-supplies

[Hans de Goede]

Hi All,

The last 2 patches of this series depend on the first 2, as such it might
be better to merge all 4 through the acpi tree if Sebastius is ok with
that.

Otherwise these patches are pretty self-explanatory.

Regards,

Hans

[PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[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 dstds or because of missing vendor specific acpi opregion
(e.g. BMOP opregion) support, which the acpi battery device in the
dsdt relies on.

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 an acpi_battery_unregister() function which native
pmic drivers can call to tell the acpi-battery driver to unregister
itself so that we do not end up with 2 power_supply-s for the same
battery device.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Sergei Trusov <t.rus76@ya.ru>
---
 drivers/acpi/battery.c     | 32 +++++++++++++++++++++++++++++++-
 include/linux/power/acpi.h | 18 ++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/power/acpi.h

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 4ef1e46..644e154 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -31,6 +31,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
+#include <linux/mutex.h>
 #include <asm/unaligned.h>
 
 #ifdef CONFIG_ACPI_PROCFS_POWER
@@ -41,6 +42,7 @@
 
 #include <linux/acpi.h>
 #include <linux/power_supply.h>
+#include <linux/power/acpi.h>
 
 #include "battery.h"
 
@@ -66,6 +68,10 @@ MODULE_AUTHOR("Alexey Starikovskiy <astarikovskiy@suse.de>");
 MODULE_DESCRIPTION("ACPI Battery Driver");
 MODULE_LICENSE("GPL");
 
+enum init_state_enum { BAT_NONE, BAT_INITIALIZED, BAT_EXITED };
+
+static enum init_state_enum init_state;
+static DEFINE_MUTEX(init_state_mutex);
 static async_cookie_t async_cookie;
 static int battery_bix_broken_package;
 static int battery_notification_delay_ms;
@@ -1336,17 +1342,41 @@ static int __init acpi_battery_init(void)
 	if (acpi_disabled)
 		return -ENODEV;
 
+	/* Check if acpi_battery_unregister got called before _init() */
+	mutex_lock(&init_state_mutex);
+	if (init_state != BAT_NONE)
+		goto out_unlock;
+
 	async_cookie = async_schedule(acpi_battery_init_async, NULL);
+	init_state = BAT_INITIALIZED;
+out_unlock:
+	mutex_unlock(&init_state_mutex);
+
 	return 0;
 }
 
-static void __exit acpi_battery_exit(void)
+void acpi_battery_unregister(void)
 {
+	/* Check if _init() is done and only do unregister once */
+	mutex_lock(&init_state_mutex);
+	if (init_state != BAT_INITIALIZED)
+		goto out_exit;
+
 	async_synchronize_cookie(async_cookie + 1);
 	acpi_bus_unregister_driver(&acpi_battery_driver);
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	acpi_unlock_battery_dir(acpi_battery_dir);
 #endif
+
+out_exit:
+	init_state = BAT_EXITED;
+	mutex_unlock(&init_state_mutex);
+}
+EXPORT_SYMBOL_GPL(acpi_battery_unregister);
+
+static void __exit acpi_battery_exit(void)
+{
+	acpi_battery_unregister();
 }
 
 module_init(acpi_battery_init);
diff --git a/include/linux/power/acpi.h b/include/linux/power/acpi.h
new file mode 100644
index 0000000..83bdfb9
--- /dev/null
+++ b/include/linux/power/acpi.h
@@ -0,0 +1,18 @@
+/*
+ * Functions exported by the acpi power_supply drivers
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_POWER_ACPI_H_
+#define __LINUX_POWER_ACPI_H_
+
+#if IS_ENABLED(CONFIG_ACPI_BATTERY)
+void acpi_battery_unregister(void);
+#else
+static inline void acpi_battery_unregister(void) {}
+#endif
+
+#endif
-- 
2.9.3

[PATCH 2/4] acpi: ac: Add acpi_ac_unregister() function

[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 of missing vendor specific acpi opregion
(e.g. BMOP opregion) support, which the acpi ac device in the dsdt
relies on. This leads for example to a ADP1 power_supply which reports
itself as always online even if no mains are connected.

This commit adds an acpi_ac_unregister() function which native pmic
drivers can call after successfully registering their own power_supply
to unregister the (potentially broken) acpi-ac power_supply.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/ac.c          | 42 +++++++++++++++++++++++++++++++++++++-----
 include/linux/power/acpi.h |  6 ++++++
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index f71b756..1c60e18 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -26,12 +26,14 @@
 #include <linux/types.h>
 #include <linux/dmi.h>
 #include <linux/delay.h>
+#include <linux/mutex.h>
 #ifdef CONFIG_ACPI_PROCFS_POWER
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #endif
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
+#include <linux/power/acpi.h>
 #include <linux/acpi.h>
 #include "battery.h"
 
@@ -422,17 +424,27 @@ static int acpi_ac_remove(struct acpi_device *device)
 	return 0;
 }
 
+enum init_state_enum { AC_NONE, AC_INITIALIZED, AC_EXITED };
+
+static enum init_state_enum init_state;
+static DEFINE_MUTEX(init_state_mutex);
+
 static int __init acpi_ac_init(void)
 {
-	int result;
+	int result, ret = -ENODEV;
 
 	if (acpi_disabled)
 		return -ENODEV;
 
+	/* Check if acpi_ac_unregister got called before _init() */
+	mutex_lock(&init_state_mutex);
+	if (init_state != AC_NONE)
+		goto out_unlock;
+
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	acpi_ac_dir = acpi_lock_ac_dir();
 	if (!acpi_ac_dir)
-		return -ENODEV;
+		goto out_unlock;
 #endif
 
 
@@ -441,18 +453,38 @@ static int __init acpi_ac_init(void)
 #ifdef CONFIG_ACPI_PROCFS_POWER
 		acpi_unlock_ac_dir(acpi_ac_dir);
 #endif
-		return -ENODEV;
+		goto out_unlock;
 	}
 
-	return 0;
+	init_state = AC_INITIALIZED;
+	ret = 0;
+out_unlock:
+	mutex_unlock(&init_state_mutex);
+	return ret;
 }
 
-static void __exit acpi_ac_exit(void)
+void acpi_ac_unregister(void)
 {
+	/* Check if _init() is done and only do unregister once */
+	mutex_lock(&init_state_mutex);
+	if (init_state != AC_INITIALIZED)
+		goto out_exit;
+
 	acpi_bus_unregister_driver(&acpi_ac_driver);
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	acpi_unlock_ac_dir(acpi_ac_dir);
 #endif
+
+out_exit:
+	init_state = AC_EXITED;
+	mutex_unlock(&init_state_mutex);
 }
+EXPORT_SYMBOL_GPL(acpi_ac_unregister);
+
+static void __exit acpi_ac_exit(void)
+{
+	acpi_ac_unregister();
+}
+
 module_init(acpi_ac_init);
 module_exit(acpi_ac_exit);
diff --git a/include/linux/power/acpi.h b/include/linux/power/acpi.h
index 83bdfb9..b50ae69 100644
--- a/include/linux/power/acpi.h
+++ b/include/linux/power/acpi.h
@@ -15,4 +15,10 @@ void acpi_battery_unregister(void);
 static inline void acpi_battery_unregister(void) {}
 #endif
 
+#if IS_ENABLED(CONFIG_ACPI_AC)
+void acpi_ac_unregister(void);
+#else
+static inline void acpi_ac_unregister(void) {}
+#endif
+
 #endif
-- 
2.9.3

[PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

On some systems with axp288 PMIC the dsdt also exports an ACPI battery
device (PNP0C0A device). 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

Note that the ACPI battery device does not work properly this is due
to Linux missing support for the vendor specific BMOP ACPI opregion.
But even if the ACPI battery where to function fine we still do not
want to export the same battery to userspace twice.

Therefor this commit calls acpi_battery_unregister() after successfully
registering the axp288-fuel-gauge power_supply to remove the duplicate
(and often broken) ACPI battery power_supply.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Sergei Trusov <t.rus76@ya.ru>
---
 drivers/power/supply/Kconfig             | 2 ++
 drivers/power/supply/axp288_fuel_gauge.c | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index d0453ca..e504644 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -242,6 +242,8 @@ config AXP288_CHARGER
 config AXP288_FUEL_GAUGE
 	tristate "X-Powers AXP288 Fuel Gauge"
 	depends on MFD_AXP20X && IIO
+	# if ACPI_BATTERY=m, this can't be 'y'
+	depends on ACPI_BATTERY || !ACPI_BATTERY
 	help
 	  Say yes here to have support for X-Power power management IC (PMIC)
 	  Fuel Gauge. The device provides battery statistics and status
diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index a8dcabc..15f10ce 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -26,6 +26,7 @@
 #include <linux/mfd/axp20x.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
+#include <linux/power/acpi.h>
 #include <linux/iio/consumer.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
@@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	acpi_battery_unregister();
+
 	fuel_gauge_create_debugfs(info);
 	fuel_gauge_init_irq(info);
 	schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES);
-- 
2.9.3

[PATCH 4/4] power: supply: axp288_charger: Unregister duplicate ACPI ac supply

[Hans de Goede]

On some systems with an axp288 PMIC the dsdt exports a non functional (*)
ACPI AC device (ACPI0003 device), which results in a Mains power_supply
which reports itself as being always online.

This commit calls acpi_ac_unregister() after successfully registering the
axp288_charger power_supply to remove the broken Mains power_supply.

*) It depends on a vendor specific BMOP ACPI opregion we do not implement

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/Kconfig          | 2 ++
 drivers/power/supply/axp288_charger.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index e504644..8ee03d7 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -235,6 +235,8 @@ config CHARGER_AXP20X
 config AXP288_CHARGER
 	tristate "X-Powers AXP288 Charger"
 	depends on MFD_AXP20X && EXTCON_AXP288
+	# if ACPI_AC=m, this can't be 'y'
+	depends on ACPI_AC || !ACPI_AC
 	help
 	  Say yes here to have support X-Power AXP288 power management IC (PMIC)
 	  integrated charger.
diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 6be2fe2..d3bf4b2 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -23,6 +23,7 @@
 #include <linux/usb/otg.h>
 #include <linux/notifier.h>
 #include <linux/power_supply.h>
+#include <linux/power/acpi.h>
 #include <linux/property.h>
 #include <linux/mfd/axp20x.h>
 #include <linux/extcon.h>
@@ -876,6 +877,7 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		}
 	}
 
+	acpi_ac_unregister();
 	return 0;
 }
 
-- 
2.9.3

Re: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[Andy Shevchenko]

On Thu, 2017-03-16 at 17:15 +0100, 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 dstds or because of missing vendor specific acpi opregion
> (e.g. BMOP opregion) support, which the acpi battery device in the
> dsdt relies on.
> 
> 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 an acpi_battery_unregister() function which native
> pmic drivers can call to tell the acpi-battery driver to unregister
> itself so that we do not end up with 2 power_supply-s for the same
> battery device.

acpi -> ACPI
pmic -> PMIC
dsdt -> DSDT (perhaps not here, but just in case)

> @@ -31,6 +31,7 @@
>  #include <linux/delay.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
> +#include <linux/mutex.h>

Keep in alphabetical order ?

>  #include <linux/acpi.h>
>  #include <linux/power_supply.h>
> +#include <linux/power/acpi.h>

Ditto. (though I don't remember which is first _ or /)
 
> +	/* Check if acpi_battery_unregister got called before _init()
> */

acpi_battery_unregister -> acpi_battery_unregister() ?

> +	mutex_lock(&init_state_mutex);
> +	if (init_state != BAT_NONE)
> +		goto out_unlock;
> +
>  	async_cookie = async_schedule(acpi_battery_init_async, NULL);
> +	init_state = BAT_INITIALIZED;

+ empty line here...

> +out_unlock:
> +	mutex_unlock(&init_state_mutex);

> +

...instead of this ?

> +++ b/include/linux/power/acpi.h

E.g. for GPIO we keep such things directly in linux/acpi.h. Does it make
sense to have separate one in this case?

Rafael, what is expected approach?

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

Re: [PATCH 2/4] acpi: ac: Add acpi_ac_unregister() function

[Andy Shevchenko]

On Thu, 2017-03-16 at 17:15 +0100, 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 of missing vendor specific acpi opregion
> (e.g. BMOP opregion) support, which the acpi ac device in the dsdt
> relies on. This leads for example to a ADP1 power_supply which reports
> itself as always online even if no mains are connected.
> 
> This commit adds an acpi_ac_unregister() function which native pmic
> drivers can call after successfully registering their own power_supply
> to unregister the (potentially broken) acpi-ac power_supply.

Similar comments as per patch 1.

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

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Andy Shevchenko]

On Thu, 2017-03-16 at 17:16 +0100, Hans de Goede wrote:
> On some systems with axp288 PMIC the dsdt also exports an ACPI battery
> device (PNP0C0A device). 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
> 
> Note that the ACPI battery device does not work properly this is due
> to Linux missing support for the vendor specific BMOP ACPI opregion.
> But even if the ACPI battery where to function fine we still do not
> want to export the same battery to userspace twice.
> 
> Therefor this commit calls acpi_battery_unregister() after
> successfully
> registering the axp288-fuel-gauge power_supply to remove the duplicate
> (and often broken) ACPI battery power_supply.

> +	# if ACPI_BATTERY=m, this can't be 'y'

Driver dependencies? Deferred probe?

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

Re: [PATCH 4/4] power: supply: axp288_charger: Unregister duplicate ACPI ac supply

[Andy Shevchenko]

On Thu, 2017-03-16 at 17:16 +0100, Hans de Goede wrote:
> On some systems with an axp288 PMIC the dsdt exports a non functional
> (*)
> ACPI AC device (ACPI0003 device), which results in a Mains
> power_supply
> which reports itself as being always online.
> 
> This commit calls acpi_ac_unregister() after successfully registering
> the
> axp288_charger power_supply to remove the broken Mains power_supply.
> 
> *) It depends on a vendor specific BMOP ACPI opregion we do not
> implement


> +	# if ACPI_AC=m, this can't be 'y'
> +	depends on ACPI_AC || !ACPI_AC

Same comment as per patch 3

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

Re: [PATCH 0/4] Avoid duplicate registering of ACPI and native power-supplies

[Sebastian Reichel]

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

Hi,

On Thu, Mar 16, 2017 at 05:15:57PM +0100, Hans de Goede wrote:
> The last 2 patches of this series depend on the first 2, as such it might
> be better to merge all 4 through the acpi tree if Sebastius is ok with
> that.
> 
> Otherwise these patches are pretty self-explanatory.

I'm fine with this going through acpi. I wonder if acpi has a method
to avoid loading the generic drivers in the first place, though. This
solution is not very nice.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[Hans de Goede]

Hi,

Thank you for the reviews!

On 16-03-17 17:29, Andy Shevchenko wrote:
> On Thu, 2017-03-16 at 17:15 +0100, 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 dstds or because of missing vendor specific acpi opregion
>> (e.g. BMOP opregion) support, which the acpi battery device in the
>> dsdt relies on.
>>
>> 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 an acpi_battery_unregister() function which native
>> pmic drivers can call to tell the acpi-battery driver to unregister
>> itself so that we do not end up with 2 power_supply-s for the same
>> battery device.
>
> acpi -> ACPI
> pmic -> PMIC
> dsdt -> DSDT (perhaps not here, but just in case)

Will fix for v2, note for the rest of the series, to avoid
spamming everyone with a lot of comments I will just fix
everything and send a v2 unless I've a specific remark.

>> @@ -31,6 +31,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/slab.h>
>>  #include <linux/suspend.h>
>> +#include <linux/mutex.h>
>
> Keep in alphabetical order ?

I'm a fan of having headers in alphabetical order myself,
but if you look at the actual file, rather then the diff
context, you will see that this file uses random order.

>>  #include <linux/acpi.h>
>>  #include <linux/power_supply.h>
>> +#include <linux/power/acpi.h>
>
> Ditto. (though I don't remember which is first _ or /)
>
>> +	/* Check if acpi_battery_unregister got called before _init()
>> */
>
> acpi_battery_unregister -> acpi_battery_unregister() ?
>
>> +	mutex_lock(&init_state_mutex);
>> +	if (init_state != BAT_NONE)
>> +		goto out_unlock;
>> +
>>  	async_cookie = async_schedule(acpi_battery_init_async, NULL);
>> +	init_state = BAT_INITIALIZED;
>
> + empty line here...

Both fixed for v2.

>
>> +out_unlock:
>> +	mutex_unlock(&init_state_mutex);
>
>> +
>
> ...instead of this ?
>
>> +++ b/include/linux/power/acpi.h
>
> E.g. for GPIO we keep such things directly in linux/acpi.h. Does it make
> sense to have separate one in this case?

I've taken include/acpi/video.h as example here. TBH I do not think
shoving everything acpi related into linux/acpi.h is a good idea.

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 16-03-17 17:33, Andy Shevchenko wrote:
> On Thu, 2017-03-16 at 17:16 +0100, Hans de Goede wrote:
>> On some systems with axp288 PMIC the dsdt also exports an ACPI battery
>> device (PNP0C0A device). 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
>>
>> Note that the ACPI battery device does not work properly this is due
>> to Linux missing support for the vendor specific BMOP ACPI opregion.
>> But even if the ACPI battery where to function fine we still do not
>> want to export the same battery to userspace twice.
>>
>> Therefor this commit calls acpi_battery_unregister() after
>> successfully
>> registering the axp288-fuel-gauge power_supply to remove the duplicate
>> (and often broken) ACPI battery power_supply.
>
>> +	# if ACPI_BATTERY=m, this can't be 'y'
>
> Driver dependencies? Deferred probe?

axp288_fuel_gauge.ko will use a symbol from drivers/acpi/battery.c
now. If ACPI_BATTERY is disabled include/linux/power/acpi.h provides
a stub, but if it is enabled in any form then no stub, so then
if ACPI_BATTERY=m axp288_fuel_gauge.ko needs to be a module too.

Regards,

Hans

Re: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[Andy Shevchenko]

On Mon, 2017-03-20 at 14:03 +0100, Hans de Goede wrote:
> Hi,
> 
> Thank you for the reviews!
> 
> On 16-03-17 17:29, Andy Shevchenko wrote:
> > On Thu, 2017-03-16 at 17:15 +0100, Hans de Goede wrote:

> > > @@ -31,6 +31,7 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/suspend.h>
> > > +#include <linux/mutex.h>
> > 
> > Keep in alphabetical order ?
> 
> I'm a fan of having headers in alphabetical order myself,
> but if you look at the actual file, rather then the diff
> context, you will see that this file uses random order.

Okay, but can you squeeze it in most ordered part?

> > > +++ b/include/linux/power/acpi.h
> > 
> > E.g. for GPIO we keep such things directly in linux/acpi.h. Does it
> > make
> > sense to have separate one in this case?
> 
> I've taken include/acpi/video.h as example here. TBH I do not think
> shoving everything acpi related into linux/acpi.h is a good idea.

So, let Rafael judge then. I have no strong opinion, just recall that
case.

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

Re: [PATCH 0/4] Avoid duplicate registering of ACPI and native power-supplies

[Hans de Goede]

Hi,

On 20-03-17 02:33, Sebastian Reichel wrote:
> Hi,
>
> On Thu, Mar 16, 2017 at 05:15:57PM +0100, Hans de Goede wrote:
>> The last 2 patches of this series depend on the first 2, as such it might
>> be better to merge all 4 through the acpi tree if Sebastius is ok with
>> that.
>>
>> Otherwise these patches are pretty self-explanatory.
>
> I'm fine with this going through acpi. I wonder if acpi has a method
> to avoid loading the generic drivers in the first place, though.

I thought about just not loading them as a first approach, but it is
not really doable in a clean way. For example the Whiskey Cove fuel
gauge driver I'm working on will need to do the same thing, but devices
with that fuel-gauge use the same ACPI HID for different fuel-gauges
and we've some extra magic in the fuel-gauge's probe method to see
if it really is the one we want like this:

         status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp);
         if (ACPI_FAILURE(status)) {
                 dev_err(dev, "Failed to get PTYPE\n");
                 return -ENODEV;
         }

         /*
          * The same ACPI HID is used with different PMICs check PTYP to
          * ensure that we are dealing with a Whiskey Cove PMIC.
          */
         if (ptyp != CHT_WC_FG_PTYPE)
                 return -ENODEV;

And on some Bay Trail / Cherry Trail devices there is a functional
ACPI battery so we really only want to not use the ACPI battery if
we have an alternative battery power_supply driver.

Regards,

Hans

Re: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[Hans de Goede]

Hi,

On 20-03-17 14:10, Andy Shevchenko wrote:
> On Mon, 2017-03-20 at 14:03 +0100, Hans de Goede wrote:
>> Hi,
>>
>> Thank you for the reviews!
>>
>> On 16-03-17 17:29, Andy Shevchenko wrote:
>>> On Thu, 2017-03-16 at 17:15 +0100, Hans de Goede wrote:
>
>>>> @@ -31,6 +31,7 @@
>>>>  #include <linux/delay.h>
>>>>  #include <linux/slab.h>
>>>>  #include <linux/suspend.h>
>>>> +#include <linux/mutex.h>
>>>
>>> Keep in alphabetical order ?
>>
>> I'm a fan of having headers in alphabetical order myself,
>> but if you look at the actual file, rather then the diff
>> context, you will see that this file uses random order.
>
> Okay, but can you squeeze it in most ordered part?
>
>>>> +++ b/include/linux/power/acpi.h
>>>
>>> E.g. for GPIO we keep such things directly in linux/acpi.h. Does it
>>> make
>>> sense to have separate one in this case?
>>
>> I've taken include/acpi/video.h as example here. TBH I do not think
>> shoving everything acpi related into linux/acpi.h is a good idea.
>
> So, let Rafael judge then. I have no strong opinion, just recall that
> case.

Ack to both remarks.

Regards,

Hans

Re: [PATCH 0/4] Avoid duplicate registering of ACPI and native power-supplies

[Andy Shevchenko]

On Mon, 2017-03-20 at 14:11 +0100, Hans de Goede wrote:
> On 20-03-17 02:33, Sebastian Reichel wrote:
> > On Thu, Mar 16, 2017 at 05:15:57PM +0100, Hans de Goede wrote:

> we've some extra magic in the fuel-gauge's probe method to see
> if it really is the one we want like this:
> 
>          status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP",
> NULL, &ptyp);

Just a side note. Looks like abusing (?) ACPI is not limited to GPIO
resources and stuff. I think we will see more stuff due to Windows
driver developers don't give a sh*t to follow standards, consistency,
and in some cases even common sense. For above case they might have used
_UID or _CID (compatible ID) + _HID (exact ID of the type of device) or
something else which is standard / more common.

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

Re: [PATCH 0/4] Avoid duplicate registering of ACPI and native power-supplies

[Hans de Goede]

Hi,

On 20-03-17 14:18, Andy Shevchenko wrote:
> On Mon, 2017-03-20 at 14:11 +0100, Hans de Goede wrote:
>> On 20-03-17 02:33, Sebastian Reichel wrote:
>>> On Thu, Mar 16, 2017 at 05:15:57PM +0100, Hans de Goede wrote:
>
>> we've some extra magic in the fuel-gauge's probe method to see
>> if it really is the one we want like this:
>>
>>          status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP",
>> NULL, &ptyp);
>
> Just a side note. Looks like abusing (?) ACPI is not limited to GPIO
> resources and stuff. I think we will see more stuff due to Windows
> driver developers don't give a sh*t to follow standards, consistency,
> and in some cases even common sense. For above case they might have used
> _UID or _CID (compatible ID) + _HID (exact ID of the type of device) or
> something else which is standard / more common.

Yeah this definitely is ACPI abuse, but nothing we can do about this
I'm afraid.

Regards,

Hans

Re: [PATCH 0/4] Avoid duplicate registering of ACPI and native power-supplies

[Rafael J. Wysocki]

On Monday, March 20, 2017 02:33:05 AM Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Mar 16, 2017 at 05:15:57PM +0100, Hans de Goede wrote:
> > The last 2 patches of this series depend on the first 2, as such it might
> > be better to merge all 4 through the acpi tree if Sebastius is ok with
> > that.
> > 
> > Otherwise these patches are pretty self-explanatory.
> 
> I'm fine with this going through acpi. I wonder if acpi has a method
> to avoid loading the generic drivers in the first place, though. This
> solution is not very nice.

There might be a list of device IDs forcing the ->match callback to fail, but
I'm not sure that would be a viable approach in this particular case from the
top of my head.

Thanks,
Rafael

RE: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[Zheng, Lv]

Hi, Hans

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Hans de
> Goede
> Subject: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function
> 
> 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 dstds or because of missing vendor specific acpi opregion

dstds -> dsdts?

> (e.g. BMOP opregion) support, which the acpi battery device in the
> dsdt relies on.

Why don't we support BMOP and leverage between PMIC driver and BMOP opregion driver?

> 
> 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 an acpi_battery_unregister() function which native
> pmic drivers can call to tell the acpi-battery driver to unregister
> itself so that we do not end up with 2 power_supply-s for the same
> battery device.

I'm not sure if this is a good idea:
Driver A calls Driver B's unregister function in case Driver A should be unware of the existence of Driver B.

For example, acpi_video_unregister() is such a bad practice.

Do we have any other choices?
For example, driver priority and etc.?

Thanks
Lv


> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Sergei Trusov <t.rus76@ya.ru>
> ---
>  drivers/acpi/battery.c     | 32 +++++++++++++++++++++++++++++++-
>  include/linux/power/acpi.h | 18 ++++++++++++++++++
>  2 files changed, 49 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/power/acpi.h
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 4ef1e46..644e154 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -31,6 +31,7 @@
>  #include <linux/delay.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
> +#include <linux/mutex.h>
>  #include <asm/unaligned.h>
> 
>  #ifdef CONFIG_ACPI_PROCFS_POWER
> @@ -41,6 +42,7 @@
> 
>  #include <linux/acpi.h>
>  #include <linux/power_supply.h>
> +#include <linux/power/acpi.h>
> 
>  #include "battery.h"
> 
> @@ -66,6 +68,10 @@ MODULE_AUTHOR("Alexey Starikovskiy <astarikovskiy@suse.de>");
>  MODULE_DESCRIPTION("ACPI Battery Driver");
>  MODULE_LICENSE("GPL");
> 
> +enum init_state_enum { BAT_NONE, BAT_INITIALIZED, BAT_EXITED };
> +
> +static enum init_state_enum init_state;
> +static DEFINE_MUTEX(init_state_mutex);
>  static async_cookie_t async_cookie;
>  static int battery_bix_broken_package;
>  static int battery_notification_delay_ms;
> @@ -1336,17 +1342,41 @@ static int __init acpi_battery_init(void)
>  	if (acpi_disabled)
>  		return -ENODEV;
> 
> +	/* Check if acpi_battery_unregister got called before _init() */
> +	mutex_lock(&init_state_mutex);
> +	if (init_state != BAT_NONE)
> +		goto out_unlock;
> +
>  	async_cookie = async_schedule(acpi_battery_init_async, NULL);
> +	init_state = BAT_INITIALIZED;
> +out_unlock:
> +	mutex_unlock(&init_state_mutex);
> +
>  	return 0;
>  }
> 
> -static void __exit acpi_battery_exit(void)
> +void acpi_battery_unregister(void)
>  {
> +	/* Check if _init() is done and only do unregister once */
> +	mutex_lock(&init_state_mutex);
> +	if (init_state != BAT_INITIALIZED)
> +		goto out_exit;
> +
>  	async_synchronize_cookie(async_cookie + 1);
>  	acpi_bus_unregister_driver(&acpi_battery_driver);
>  #ifdef CONFIG_ACPI_PROCFS_POWER
>  	acpi_unlock_battery_dir(acpi_battery_dir);
>  #endif
> +
> +out_exit:
> +	init_state = BAT_EXITED;
> +	mutex_unlock(&init_state_mutex);
> +}
> +EXPORT_SYMBOL_GPL(acpi_battery_unregister);
> +
> +static void __exit acpi_battery_exit(void)
> +{
> +	acpi_battery_unregister();
>  }
> 
>  module_init(acpi_battery_init);
> diff --git a/include/linux/power/acpi.h b/include/linux/power/acpi.h
> new file mode 100644
> index 0000000..83bdfb9
> --- /dev/null
> +++ b/include/linux/power/acpi.h
> @@ -0,0 +1,18 @@
> +/*
> + * Functions exported by the acpi power_supply drivers
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __LINUX_POWER_ACPI_H_
> +#define __LINUX_POWER_ACPI_H_
> +
> +#if IS_ENABLED(CONFIG_ACPI_BATTERY)
> +void acpi_battery_unregister(void);
> +#else
> +static inline void acpi_battery_unregister(void) {}
> +#endif
> +
> +#endif
> --
> 2.9.3
> 
> --
> 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

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Rafael J. Wysocki]

On Thursday, March 16, 2017 05:16:00 PM Hans de Goede wrote:
> On some systems with axp288 PMIC the dsdt also exports an ACPI battery
> device (PNP0C0A device). 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
> 
> Note that the ACPI battery device does not work properly this is due
> to Linux missing support for the vendor specific BMOP ACPI opregion.
> But even if the ACPI battery where to function fine we still do not
> want to export the same battery to userspace twice.
> 
> Therefor this commit calls acpi_battery_unregister() after successfully
> registering the axp288-fuel-gauge power_supply to remove the duplicate
> (and often broken) ACPI battery power_supply.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Sergei Trusov <t.rus76@ya.ru>
> ---
>  drivers/power/supply/Kconfig             | 2 ++
>  drivers/power/supply/axp288_fuel_gauge.c | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index d0453ca..e504644 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -242,6 +242,8 @@ config AXP288_CHARGER
>  config AXP288_FUEL_GAUGE
>  	tristate "X-Powers AXP288 Fuel Gauge"
>  	depends on MFD_AXP20X && IIO
> +	# if ACPI_BATTERY=m, this can't be 'y'
> +	depends on ACPI_BATTERY || !ACPI_BATTERY
>  	help
>  	  Say yes here to have support for X-Power power management IC (PMIC)
>  	  Fuel Gauge. The device provides battery statistics and status
> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
> index a8dcabc..15f10ce 100644
> --- a/drivers/power/supply/axp288_fuel_gauge.c
> +++ b/drivers/power/supply/axp288_fuel_gauge.c
> @@ -26,6 +26,7 @@
>  #include <linux/mfd/axp20x.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
> +#include <linux/power/acpi.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	acpi_battery_unregister();
> +

What if the ACPI battery driver is loaded after this has been called already?

>  	fuel_gauge_create_debugfs(info);
>  	fuel_gauge_init_irq(info);
>  	schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES);
> 

Thanks,
Rafael

Re: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[Hans de Goede]

Hi,

On 27-03-17 03:16, Zheng, Lv wrote:
> Hi, Hans
>
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Hans de
>> Goede
>> Subject: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function
>>
>> 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 dstds or because of missing vendor specific acpi opregion
>
> dstds -> dsdts?

Yes, fixed locally.

>
>> (e.g. BMOP opregion) support, which the acpi battery device in the
>> dsdt relies on.
>
> Why don't we support BMOP and leverage between PMIC driver and BMOP opregion driver?

Multiple reasons:
1) The BMOP is an undocumented proprietary ACPI interface (not part of the
    ACPI spec) which would need to be reverse engineered and implemented per
    fuel-gauge driver; once for the axp288, once for the max17047 once for
    the ti fuel gauge which can be used on some models according to the DSDTs
    I've.

2) Even if we do this we still will not get a battery everywhere as not all
    DSDTs export the ACPI battery interface, only some do. So we still need
    the native driver + power_supply for those tablets without the ACPI
    battery interface and on those with the ACPI interface we end up with
    multiple battery interfaces for a single battery which breaks the userspace
    ABI.

3) Even if we reverse-engineer the BMOP stuff and get things to work, the
    quality of the ACPI battery implementation in these DSDTs is questionable,
    where as with native fuel-gauge drivers we can guarantee proper reporting
    of the battery stare to userspace

TL;DR: having a native driver is better and there should only be one driver
registered, hence this patch.


>
>>
>> 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 an acpi_battery_unregister() function which native
>> pmic drivers can call to tell the acpi-battery driver to unregister
>> itself so that we do not end up with 2 power_supply-s for the same
>> battery device.
>
> I'm not sure if this is a good idea:
> Driver A calls Driver B's unregister function in case Driver A should be unware of the existence of Driver B.
>
> For example, acpi_video_unregister() is such a bad practice.

acpi_video_unregister() although not pretty is very much necessary
actually the whole backlight sysfs interface were we do sometimes
register multiple sysfs backlight class devices for a single
backlight and let userspace figure things out is a prime
example of why we need this, we need to export only one
interface and in case of the power_supply interface doing
anything else would be an ABI break.

Note that in this case things are actually much better then
the acpi_video stuff, since we've a very clear place when to
do the unregister (in the native x86 only drivers) rather then
needing some magic heuristics.

> Do we have any other choices?

We could alternatively have the ACPI battery and ac drivers
contain a black list of ACPI HIDs which they check and when
these are in the DSDT and enabled/present have their probe
methods return -ENODEV.

I dismissed this at first, but on second thought that should
work.

Regards,

Hans




> For example, driver priority and etc.?
>
> Thanks
> Lv
>
>
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Sergei Trusov <t.rus76@ya.ru>
>> ---
>>  drivers/acpi/battery.c     | 32 +++++++++++++++++++++++++++++++-
>>  include/linux/power/acpi.h | 18 ++++++++++++++++++
>>  2 files changed, 49 insertions(+), 1 deletion(-)
>>  create mode 100644 include/linux/power/acpi.h
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 4ef1e46..644e154 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/slab.h>
>>  #include <linux/suspend.h>
>> +#include <linux/mutex.h>
>>  #include <asm/unaligned.h>
>>
>>  #ifdef CONFIG_ACPI_PROCFS_POWER
>> @@ -41,6 +42,7 @@
>>
>>  #include <linux/acpi.h>
>>  #include <linux/power_supply.h>
>> +#include <linux/power/acpi.h>
>>
>>  #include "battery.h"
>>
>> @@ -66,6 +68,10 @@ MODULE_AUTHOR("Alexey Starikovskiy <astarikovskiy@suse.de>");
>>  MODULE_DESCRIPTION("ACPI Battery Driver");
>>  MODULE_LICENSE("GPL");
>>
>> +enum init_state_enum { BAT_NONE, BAT_INITIALIZED, BAT_EXITED };
>> +
>> +static enum init_state_enum init_state;
>> +static DEFINE_MUTEX(init_state_mutex);
>>  static async_cookie_t async_cookie;
>>  static int battery_bix_broken_package;
>>  static int battery_notification_delay_ms;
>> @@ -1336,17 +1342,41 @@ static int __init acpi_battery_init(void)
>>  	if (acpi_disabled)
>>  		return -ENODEV;
>>
>> +	/* Check if acpi_battery_unregister got called before _init() */
>> +	mutex_lock(&init_state_mutex);
>> +	if (init_state != BAT_NONE)
>> +		goto out_unlock;
>> +
>>  	async_cookie = async_schedule(acpi_battery_init_async, NULL);
>> +	init_state = BAT_INITIALIZED;
>> +out_unlock:
>> +	mutex_unlock(&init_state_mutex);
>> +
>>  	return 0;
>>  }
>>
>> -static void __exit acpi_battery_exit(void)
>> +void acpi_battery_unregister(void)
>>  {
>> +	/* Check if _init() is done and only do unregister once */
>> +	mutex_lock(&init_state_mutex);
>> +	if (init_state != BAT_INITIALIZED)
>> +		goto out_exit;
>> +
>>  	async_synchronize_cookie(async_cookie + 1);
>>  	acpi_bus_unregister_driver(&acpi_battery_driver);
>>  #ifdef CONFIG_ACPI_PROCFS_POWER
>>  	acpi_unlock_battery_dir(acpi_battery_dir);
>>  #endif
>> +
>> +out_exit:
>> +	init_state = BAT_EXITED;
>> +	mutex_unlock(&init_state_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_battery_unregister);
>> +
>> +static void __exit acpi_battery_exit(void)
>> +{
>> +	acpi_battery_unregister();
>>  }
>>
>>  module_init(acpi_battery_init);
>> diff --git a/include/linux/power/acpi.h b/include/linux/power/acpi.h
>> new file mode 100644
>> index 0000000..83bdfb9
>> --- /dev/null
>> +++ b/include/linux/power/acpi.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * Functions exported by the acpi power_supply drivers
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __LINUX_POWER_ACPI_H_
>> +#define __LINUX_POWER_ACPI_H_
>> +
>> +#if IS_ENABLED(CONFIG_ACPI_BATTERY)
>> +void acpi_battery_unregister(void);
>> +#else
>> +static inline void acpi_battery_unregister(void) {}
>> +#endif
>> +
>> +#endif
>> --
>> 2.9.3
>>
>> --
>> 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

Re: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function

[Hans de Goede]

Hi,

On 31-03-17 10:53, Hans de Goede wrote:
> Hi,
>
> On 27-03-17 03:16, Zheng, Lv wrote:
>> Hi, Hans
>>
>>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Hans de
>>> Goede
>>> Subject: [PATCH 1/4] acpi: battery: Add acpi_battery_unregister() function
>>>
>>> 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 dstds or because of missing vendor specific acpi opregion
>>
>> dstds -> dsdts?
>
> Yes, fixed locally.
>
>>
>>> (e.g. BMOP opregion) support, which the acpi battery device in the
>>> dsdt relies on.
>>
>> Why don't we support BMOP and leverage between PMIC driver and BMOP opregion driver?
>
> Multiple reasons:
> 1) The BMOP is an undocumented proprietary ACPI interface (not part of the
>    ACPI spec) which would need to be reverse engineered and implemented per
>    fuel-gauge driver; once for the axp288, once for the max17047 once for
>    the ti fuel gauge which can be used on some models according to the DSDTs
>    I've.
>
> 2) Even if we do this we still will not get a battery everywhere as not all
>    DSDTs export the ACPI battery interface, only some do. So we still need
>    the native driver + power_supply for those tablets without the ACPI
>    battery interface and on those with the ACPI interface we end up with
>    multiple battery interfaces for a single battery which breaks the userspace
>    ABI.
>
> 3) Even if we reverse-engineer the BMOP stuff and get things to work, the
>    quality of the ACPI battery implementation in these DSDTs is questionable,
>    where as with native fuel-gauge drivers we can guarantee proper reporting
>    of the battery stare to userspace
>
> TL;DR: having a native driver is better and there should only be one driver
> registered, hence this patch.
>
>
>>
>>>
>>> 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 an acpi_battery_unregister() function which native
>>> pmic drivers can call to tell the acpi-battery driver to unregister
>>> itself so that we do not end up with 2 power_supply-s for the same
>>> battery device.
>>
>> I'm not sure if this is a good idea:
>> Driver A calls Driver B's unregister function in case Driver A should be unware of the existence of Driver B.
>>
>> For example, acpi_video_unregister() is such a bad practice.
>
> acpi_video_unregister() although not pretty is very much necessary
> actually the whole backlight sysfs interface were we do sometimes
> register multiple sysfs backlight class devices for a single
> backlight and let userspace figure things out is a prime
> example of why we need this, we need to export only one
> interface and in case of the power_supply interface doing
> anything else would be an ABI break.
>
> Note that in this case things are actually much better then
> the acpi_video stuff, since we've a very clear place when to
> do the unregister (in the native x86 only drivers) rather then
> needing some magic heuristics.
>
>> Do we have any other choices?
>
> We could alternatively have the ACPI battery and ac drivers
> contain a black list of ACPI HIDs which they check and when
> these are in the DSDT and enabled/present have their probe
> methods return -ENODEV.
>
> I dismissed this at first, but on second thought that should
> work.

Ah scrap that I remember again why this will not work, the
problem is that Intel re-uses HIDs between generations and
for the Whiskey Cove PMIC we want to not use the ACPI battery
and ac drivers on Cherry Trail (where they are known to be
broken) but things are different on Apollo Lake. Yet both
use the same HID for their companion Whiskey Cove PMIC even
though they are 2 completely different revisions of the PMIC
(e.g. one uses i2c the other does not).

The 2 native drivers have code to detect which revision they
are dealing with and exit with -ENODEV if it is not the
revision they were written for, but this means that simple
HID blacklisting will not work. So IMHO the decision to
unregister the  ACPI battery / ac interface really belongs
in the native driver as that has all the nitty gritty detail
needed to make that decision.

So I really believe we should move forward with this
patch set as-is.

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 29-03-17 22:31, Rafael J. Wysocki wrote:
> On Thursday, March 16, 2017 05:16:00 PM Hans de Goede wrote:
>> On some systems with axp288 PMIC the dsdt also exports an ACPI battery
>> device (PNP0C0A device). 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
>>
>> Note that the ACPI battery device does not work properly this is due
>> to Linux missing support for the vendor specific BMOP ACPI opregion.
>> But even if the ACPI battery where to function fine we still do not
>> want to export the same battery to userspace twice.
>>
>> Therefor this commit calls acpi_battery_unregister() after successfully
>> registering the axp288-fuel-gauge power_supply to remove the duplicate
>> (and often broken) ACPI battery power_supply.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=194811
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Sergei Trusov <t.rus76@ya.ru>
>> ---
>>  drivers/power/supply/Kconfig             | 2 ++
>>  drivers/power/supply/axp288_fuel_gauge.c | 3 +++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index d0453ca..e504644 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -242,6 +242,8 @@ config AXP288_CHARGER
>>  config AXP288_FUEL_GAUGE
>>  	tristate "X-Powers AXP288 Fuel Gauge"
>>  	depends on MFD_AXP20X && IIO
>> +	# if ACPI_BATTERY=m, this can't be 'y'
>> +	depends on ACPI_BATTERY || !ACPI_BATTERY
>>  	help
>>  	  Say yes here to have support for X-Power power management IC (PMIC)
>>  	  Fuel Gauge. The device provides battery statistics and status
>> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
>> index a8dcabc..15f10ce 100644
>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/mfd/axp20x.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/power_supply.h>
>> +#include <linux/power/acpi.h>
>>  #include <linux/iio/consumer.h>
>>  #include <linux/debugfs.h>
>>  #include <linux/seq_file.h>
>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>
>> +	acpi_battery_unregister();
>> +
>
> What if the ACPI battery driver is loaded after this has been called already?

The module exports that symbol so it must be loaded already. If both are
built-in then the probe method of the axp288_fuel_gauge may run first,
acpi_battery_unregister() sets a (mutex proteted) flag which gets
checked by acpi_battery_probe() which will then exit with -ENODEV
when it runs later.

So this is already taken care of :)  Also see the first patch in
this series which implements acpi_battery_unregister().

Regards,

Hans




>
>>  	fuel_gauge_create_debugfs(info);
>>  	fuel_gauge_init_irq(info);
>>  	schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES);
>>
>
> Thanks,
> Rafael
>

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Rafael J. Wysocki]

On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> wrote:

[cut]

>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>> @@ -26,6 +26,7 @@
>>>  #include <linux/mfd/axp20x.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/power_supply.h>
>>> +#include <linux/power/acpi.h>
>>>  #include <linux/iio/consumer.h>
>>>  #include <linux/debugfs.h>
>>>  #include <linux/seq_file.h>
>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>> platform_device *pdev)
>>>                 return ret;
>>>         }
>>>
>>> +       acpi_battery_unregister();
>>> +
>>
>>
>> What if the ACPI battery driver is loaded after this has been called
>> already?
>
>
> The module exports that symbol so it must be loaded already.

But then it may be unloaded manually and loaded again, may it not?

Thanks,
Rafael

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 31-03-17 11:05, Rafael J. Wysocki wrote:
> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>
> [cut]
>
>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include <linux/mfd/axp20x.h>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/power_supply.h>
>>>> +#include <linux/power/acpi.h>
>>>>  #include <linux/iio/consumer.h>
>>>>  #include <linux/debugfs.h>
>>>>  #include <linux/seq_file.h>
>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>> platform_device *pdev)
>>>>                 return ret;
>>>>         }
>>>>
>>>> +       acpi_battery_unregister();
>>>> +
>>>
>>>
>>> What if the ACPI battery driver is loaded after this has been called
>>> already?
>>
>>
>> The module exports that symbol so it must be loaded already.
>
> But then it may be unloaded manually and loaded again, may it not?

Only if you first unload axp288_fuel_gauge.ko otherwise it will
have a refcount > 0.

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Rafael J. Wysocki]

On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 31-03-17 11:05, Rafael J. Wysocki wrote:
>>
>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>
>> [cut]
>>
>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>>> @@ -26,6 +26,7 @@
>>>>>  #include <linux/mfd/axp20x.h>
>>>>>  #include <linux/platform_device.h>
>>>>>  #include <linux/power_supply.h>
>>>>> +#include <linux/power/acpi.h>
>>>>>  #include <linux/iio/consumer.h>
>>>>>  #include <linux/debugfs.h>
>>>>>  #include <linux/seq_file.h>
>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>>> platform_device *pdev)
>>>>>                 return ret;
>>>>>         }
>>>>>
>>>>> +       acpi_battery_unregister();
>>>>> +
>>>>
>>>>
>>>>
>>>> What if the ACPI battery driver is loaded after this has been called
>>>> already?
>>>
>>>
>>>
>>> The module exports that symbol so it must be loaded already.
>>
>>
>> But then it may be unloaded manually and loaded again, may it not?
>
>
> Only if you first unload axp288_fuel_gauge.ko otherwise it will
> have a refcount > 0.

OK

Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.

Thanks,
Rafael

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 31-03-17 11:11, Rafael J. Wysocki wrote:
> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 31-03-17 11:05, Rafael J. Wysocki wrote:
>>>
>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>
>>> [cut]
>>>
>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include <linux/mfd/axp20x.h>
>>>>>>  #include <linux/platform_device.h>
>>>>>>  #include <linux/power_supply.h>
>>>>>> +#include <linux/power/acpi.h>
>>>>>>  #include <linux/iio/consumer.h>
>>>>>>  #include <linux/debugfs.h>
>>>>>>  #include <linux/seq_file.h>
>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>>>> platform_device *pdev)
>>>>>>                 return ret;
>>>>>>         }
>>>>>>
>>>>>> +       acpi_battery_unregister();
>>>>>> +
>>>>>
>>>>>
>>>>>
>>>>> What if the ACPI battery driver is loaded after this has been called
>>>>> already?
>>>>
>>>>
>>>>
>>>> The module exports that symbol so it must be loaded already.
>>>
>>>
>>> But then it may be unloaded manually and loaded again, may it not?
>>
>>
>> Only if you first unload axp288_fuel_gauge.ko otherwise it will
>> have a refcount > 0.
>
> OK
>
> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.

As I explained in my reply to the discussion around the first patch that
is somewhat hard to do and requires encoding knowledge in those drivers
which really does not belong there:

"The problem is that Intel re-uses HIDs between generations and
for the Whiskey Cove PMIC we want to not use the ACPI battery
and ac drivers on Cherry Trail (where they are known to be
broken) but things are different on Apollo Lake. Yet both
use the same HID for their companion Whiskey Cove PMIC even
though they are 2 completely different revisions of the PMIC
(e.g. one uses i2c the other does not).

The 2 native drivers have code to detect which revision they
are dealing with and exit with -ENODEV if it is not the
revision they were written for, but this means that simple
HID blacklisting will not work. So IMHO the decision to
unregister the  ACPI battery / ac interface really belongs
in the native driver as that has all the nitty gritty detail
needed to make that decision."

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Rafael J. Wysocki]

On Friday, March 31, 2017 11:57:47 AM Hans de Goede wrote:
> Hi,

Hi,

[cut]

> >
> > Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.
> 
> As I explained in my reply to the discussion around the first patch that
> is somewhat hard to do and requires encoding knowledge in those drivers
> which really does not belong there:
> 
> "The problem is that Intel re-uses HIDs between generations and
> for the Whiskey Cove PMIC we want to not use the ACPI battery
> and ac drivers on Cherry Trail (where they are known to be
> broken) but things are different on Apollo Lake. Yet both
> use the same HID for their companion Whiskey Cove PMIC even
> though they are 2 completely different revisions of the PMIC
> (e.g. one uses i2c the other does not).
> 
> The 2 native drivers have code to detect which revision they
> are dealing with and exit with -ENODEV if it is not the
> revision they were written for, but this means that simple
> HID blacklisting will not work. So IMHO the decision to
> unregister the  ACPI battery / ac interface really belongs
> in the native driver as that has all the nitty gritty detail
> needed to make that decision."

Do the native drivers bind to platform devices?

Thanks,
Rafael

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 01-04-17 00:30, Rafael J. Wysocki wrote:
> On Friday, March 31, 2017 11:57:47 AM Hans de Goede wrote:
>> Hi,
>
> Hi,
>
> [cut]
>
>>>
>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.
>>
>> As I explained in my reply to the discussion around the first patch that
>> is somewhat hard to do and requires encoding knowledge in those drivers
>> which really does not belong there:
>>
>> "The problem is that Intel re-uses HIDs between generations and
>> for the Whiskey Cove PMIC we want to not use the ACPI battery
>> and ac drivers on Cherry Trail (where they are known to be
>> broken) but things are different on Apollo Lake. Yet both
>> use the same HID for their companion Whiskey Cove PMIC even
>> though they are 2 completely different revisions of the PMIC
>> (e.g. one uses i2c the other does not).
>>
>> The 2 native drivers have code to detect which revision they
>> are dealing with and exit with -ENODEV if it is not the
>> revision they were written for, but this means that simple
>> HID blacklisting will not work. So IMHO the decision to
>> unregister the  ACPI battery / ac interface really belongs
>> in the native driver as that has all the nitty gritty detail
>> needed to make that decision."
>
> Do the native drivers bind to platform devices?

No, (yes, but not really) they are i2c drivers, but for cells of
mfd i2c devices, so under the hood they bind to platform devices.

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 31-03-17 11:57, Hans de Goede wrote:
> Hi,
>
> On 31-03-17 11:11, Rafael J. Wysocki wrote:
>> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 31-03-17 11:05, Rafael J. Wysocki wrote:
>>>>
>>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>
>>>> [cut]
>>>>
>>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>  #include <linux/mfd/axp20x.h>
>>>>>>>  #include <linux/platform_device.h>
>>>>>>>  #include <linux/power_supply.h>
>>>>>>> +#include <linux/power/acpi.h>
>>>>>>>  #include <linux/iio/consumer.h>
>>>>>>>  #include <linux/debugfs.h>
>>>>>>>  #include <linux/seq_file.h>
>>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>>>>> platform_device *pdev)
>>>>>>>                 return ret;
>>>>>>>         }
>>>>>>>
>>>>>>> +       acpi_battery_unregister();
>>>>>>> +
>>>>>>
>>>>>>
>>>>>>
>>>>>> What if the ACPI battery driver is loaded after this has been called
>>>>>> already?
>>>>>
>>>>>
>>>>>
>>>>> The module exports that symbol so it must be loaded already.
>>>>
>>>>
>>>> But then it may be unloaded manually and loaded again, may it not?
>>>
>>>
>>> Only if you first unload axp288_fuel_gauge.ko otherwise it will
>>> have a refcount > 0.
>>
>> OK
>>
>> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.
>
> As I explained in my reply to the discussion around the first patch that
> is somewhat hard to do and requires encoding knowledge in those drivers
> which really does not belong there:
>
> "The problem is that Intel re-uses HIDs between generations and
> for the Whiskey Cove PMIC we want to not use the ACPI battery
> and ac drivers on Cherry Trail (where they are known to be
> broken) but things are different on Apollo Lake. Yet both
> use the same HID for their companion Whiskey Cove PMIC even
> though they are 2 completely different revisions of the PMIC
> (e.g. one uses i2c the other does not).
>
> The 2 native drivers have code to detect which revision they
> are dealing with and exit with -ENODEV if it is not the
> revision they were written for, but this means that simple
> HID blacklisting will not work. So IMHO the decision to
> unregister the  ACPI battery / ac interface really belongs
> in the native driver as that has all the nitty gritty detail
> needed to make that decision."

So thinking more about this, esp. after receiving a bug report
from a user getting ACPI errors because of Linux not implementing
the proprietary undocumented BMOP opregion before the ACPI battery
driver gets unregistered by the native one, I thing we do indeed
need to go with a blacklist.

This means also being able to match by _HRV, as Some HIDs are
re-used for different hardware between Bay Trail / Cherry Trail /
Broxton with just a bump of _HRV.

I'm currently working on respinning my
"acpi: utils: Add new acpi_dev_present helper" to address the
review comments on it. I'm going to give it an optional hrv
function argument for this use, so as to not having to implement
hrv checking code in both ac.c and battery.c .

So self-nack for this series as is.

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 07-04-17 09:18, Hans de Goede wrote:
> Hi,
>
> On 31-03-17 11:57, Hans de Goede wrote:
>> Hi,
>>
>> On 31-03-17 11:11, Rafael J. Wysocki wrote:
>>> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 31-03-17 11:05, Rafael J. Wysocki wrote:
>>>>>
>>>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>>
>>>>> [cut]
>>>>>
>>>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>>  #include <linux/mfd/axp20x.h>
>>>>>>>>  #include <linux/platform_device.h>
>>>>>>>>  #include <linux/power_supply.h>
>>>>>>>> +#include <linux/power/acpi.h>
>>>>>>>>  #include <linux/iio/consumer.h>
>>>>>>>>  #include <linux/debugfs.h>
>>>>>>>>  #include <linux/seq_file.h>
>>>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>                 return ret;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> +       acpi_battery_unregister();
>>>>>>>> +
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> What if the ACPI battery driver is loaded after this has been called
>>>>>>> already?
>>>>>>
>>>>>>
>>>>>>
>>>>>> The module exports that symbol so it must be loaded already.
>>>>>
>>>>>
>>>>> But then it may be unloaded manually and loaded again, may it not?
>>>>
>>>>
>>>> Only if you first unload axp288_fuel_gauge.ko otherwise it will
>>>> have a refcount > 0.
>>>
>>> OK
>>>
>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.
>>
>> As I explained in my reply to the discussion around the first patch that
>> is somewhat hard to do and requires encoding knowledge in those drivers
>> which really does not belong there:
>>
>> "The problem is that Intel re-uses HIDs between generations and
>> for the Whiskey Cove PMIC we want to not use the ACPI battery
>> and ac drivers on Cherry Trail (where they are known to be
>> broken) but things are different on Apollo Lake. Yet both
>> use the same HID for their companion Whiskey Cove PMIC even
>> though they are 2 completely different revisions of the PMIC
>> (e.g. one uses i2c the other does not).
>>
>> The 2 native drivers have code to detect which revision they
>> are dealing with and exit with -ENODEV if it is not the
>> revision they were written for, but this means that simple
>> HID blacklisting will not work. So IMHO the decision to
>> unregister the  ACPI battery / ac interface really belongs
>> in the native driver as that has all the nitty gritty detail
>> needed to make that decision."
>
> So thinking more about this, esp. after receiving a bug report
> from a user getting ACPI errors because of Linux not implementing
> the proprietary undocumented BMOP opregion before the ACPI battery
> driver gets unregistered by the native one, I thing we do indeed
> need to go with a blacklist.
>
> This means also being able to match by _HRV, as Some HIDs are
> re-used for different hardware between Bay Trail / Cherry Trail /
> Broxton with just a bump of _HRV.
>
> I'm currently working on respinning my
> "acpi: utils: Add new acpi_dev_present helper" to address the
> review comments on it. I'm going to give it an optional hrv
> function argument for this use, so as to not having to implement
> hrv checking code in both ac.c and battery.c .

So a quick copy paste from another thread, the black-list approach
causes regressions even before hitting -next and seeing any
substantial testing, so we're back to adding unregister functions
and calling those from native PMIC power_supply drivers when the
native power_supply has been successfully registered.

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

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 10-04-17 09:31, Hans de Goede wrote:
> Hi,
>
> On 07-04-17 09:18, Hans de Goede wrote:
>> Hi,
>>
>> On 31-03-17 11:57, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 31-03-17 11:11, Rafael J. Wysocki wrote:
>>>> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 31-03-17 11:05, Rafael J. Wysocki wrote:
>>>>>>
>>>>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com>
>>>>>> wrote:
>>>>>>
>>>>>> [cut]
>>>>>>
>>>>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>>>  #include <linux/mfd/axp20x.h>
>>>>>>>>>  #include <linux/platform_device.h>
>>>>>>>>>  #include <linux/power_supply.h>
>>>>>>>>> +#include <linux/power/acpi.h>
>>>>>>>>>  #include <linux/iio/consumer.h>
>>>>>>>>>  #include <linux/debugfs.h>
>>>>>>>>>  #include <linux/seq_file.h>
>>>>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>>>>>>> platform_device *pdev)
>>>>>>>>>                 return ret;
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>> +       acpi_battery_unregister();
>>>>>>>>> +
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> What if the ACPI battery driver is loaded after this has been called
>>>>>>>> already?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The module exports that symbol so it must be loaded already.
>>>>>>
>>>>>>
>>>>>> But then it may be unloaded manually and loaded again, may it not?
>>>>>
>>>>>
>>>>> Only if you first unload axp288_fuel_gauge.ko otherwise it will
>>>>> have a refcount > 0.
>>>>
>>>> OK
>>>>
>>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.
>>>
>>> As I explained in my reply to the discussion around the first patch that
>>> is somewhat hard to do and requires encoding knowledge in those drivers
>>> which really does not belong there:
>>>
>>> "The problem is that Intel re-uses HIDs between generations and
>>> for the Whiskey Cove PMIC we want to not use the ACPI battery
>>> and ac drivers on Cherry Trail (where they are known to be
>>> broken) but things are different on Apollo Lake. Yet both
>>> use the same HID for their companion Whiskey Cove PMIC even
>>> though they are 2 completely different revisions of the PMIC
>>> (e.g. one uses i2c the other does not).
>>>
>>> The 2 native drivers have code to detect which revision they
>>> are dealing with and exit with -ENODEV if it is not the
>>> revision they were written for, but this means that simple
>>> HID blacklisting will not work. So IMHO the decision to
>>> unregister the  ACPI battery / ac interface really belongs
>>> in the native driver as that has all the nitty gritty detail
>>> needed to make that decision."
>>
>> So thinking more about this, esp. after receiving a bug report
>> from a user getting ACPI errors because of Linux not implementing
>> the proprietary undocumented BMOP opregion before the ACPI battery
>> driver gets unregistered by the native one, I thing we do indeed
>> need to go with a blacklist.
>>
>> This means also being able to match by _HRV, as Some HIDs are
>> re-used for different hardware between Bay Trail / Cherry Trail /
>> Broxton with just a bump of _HRV.
>>
>> I'm currently working on respinning my
>> "acpi: utils: Add new acpi_dev_present helper" to address the
>> review comments on it. I'm going to give it an optional hrv
>> function argument for this use, so as to not having to implement
>> hrv checking code in both ac.c and battery.c .
>
> So a quick copy paste from another thread, the black-list approach
> causes regressions even before hitting -next and seeing any
> substantial testing, so we're back to adding unregister functions
> and calling those from native PMIC power_supply drivers when the
> native power_supply has been successfully registered.
>
> 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.

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Rafael J. Wysocki]

On Mon, Apr 10, 2017 at 8:13 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 10-04-17 09:31, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 07-04-17 09:18, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 31-03-17 11:57, Hans de Goede wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 31-03-17 11:11, Rafael J. Wysocki wrote:
>>>>>
>>>>> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 31-03-17 11:05, Rafael J. Wysocki wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>> [cut]
>>>>>>>
>>>>>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>>>>  #include <linux/mfd/axp20x.h>
>>>>>>>>>>  #include <linux/platform_device.h>
>>>>>>>>>>  #include <linux/power_supply.h>
>>>>>>>>>> +#include <linux/power/acpi.h>
>>>>>>>>>>  #include <linux/iio/consumer.h>
>>>>>>>>>>  #include <linux/debugfs.h>
>>>>>>>>>>  #include <linux/seq_file.h>
>>>>>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>>>>>>>> platform_device *pdev)
>>>>>>>>>>                 return ret;
>>>>>>>>>>         }
>>>>>>>>>>
>>>>>>>>>> +       acpi_battery_unregister();
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> What if the ACPI battery driver is loaded after this has been
>>>>>>>>> called
>>>>>>>>> already?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The module exports that symbol so it must be loaded already.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> But then it may be unloaded manually and loaded again, may it not?
>>>>>>
>>>>>>
>>>>>>
>>>>>> Only if you first unload axp288_fuel_gauge.ko otherwise it will
>>>>>> have a refcount > 0.
>>>>>
>>>>>
>>>>> OK
>>>>>
>>>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be
>>>>> honest.
>>>>
>>>>
>>>> As I explained in my reply to the discussion around the first patch that
>>>> is somewhat hard to do and requires encoding knowledge in those drivers
>>>> which really does not belong there:
>>>>
>>>> "The problem is that Intel re-uses HIDs between generations and
>>>> for the Whiskey Cove PMIC we want to not use the ACPI battery
>>>> and ac drivers on Cherry Trail (where they are known to be
>>>> broken) but things are different on Apollo Lake. Yet both
>>>> use the same HID for their companion Whiskey Cove PMIC even
>>>> though they are 2 completely different revisions of the PMIC
>>>> (e.g. one uses i2c the other does not).
>>>>
>>>> The 2 native drivers have code to detect which revision they
>>>> are dealing with and exit with -ENODEV if it is not the
>>>> revision they were written for, but this means that simple
>>>> HID blacklisting will not work. So IMHO the decision to
>>>> unregister the  ACPI battery / ac interface really belongs
>>>> in the native driver as that has all the nitty gritty detail
>>>> needed to make that decision."
>>>
>>>
>>> So thinking more about this, esp. after receiving a bug report
>>> from a user getting ACPI errors because of Linux not implementing
>>> the proprietary undocumented BMOP opregion before the ACPI battery
>>> driver gets unregistered by the native one, I thing we do indeed
>>> need to go with a blacklist.
>>>
>>> This means also being able to match by _HRV, as Some HIDs are
>>> re-used for different hardware between Bay Trail / Cherry Trail /
>>> Broxton with just a bump of _HRV.
>>>
>>> I'm currently working on respinning my
>>> "acpi: utils: Add new acpi_dev_present helper" to address the
>>> review comments on it. I'm going to give it an optional hrv
>>> function argument for this use, so as to not having to implement
>>> hrv checking code in both ac.c and battery.c .
>>
>>
>> So a quick copy paste from another thread, the black-list approach
>> causes regressions even before hitting -next and seeing any
>> substantial testing, so we're back to adding unregister functions
>> and calling those from native PMIC power_supply drivers when the
>> native power_supply has been successfully registered.
>>
>> 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.

OK, but there's one more possibility to consider.

Instead of unregistering the ACPI battery (or AC) driver from the
platform device driver superseding it, you could clear the
match_driver flag for the ACPI companion device and call
device_release_driver() on it, like acpi_bus_trim().  Then (because
the match_driver flag is unset) the driver core will not try to attach
the driver to the thing again - until the next invocation of
acpi_bus_attach() on it, which I think is a bug, because there seems
to be a flags.visited check missing in there (I need to go back and
recall why it is not there).

Arguably, that would be a more lightweight way of getting what you
want without using a blacklist.

Thanks,
Rafael

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede]

Hi,

On 10-04-17 22:01, Rafael J. Wysocki wrote:
> On Mon, Apr 10, 2017 at 8:13 PM, Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>>>>>> OK
>>>>>>
>>>>>> Anyway, I'd prefer blacklists in the battery and ac drivers to be
>>>>>> honest.
>>>>>
>>>>>
>>>>> As I explained in my reply to the discussion around the first patch that
>>>>> is somewhat hard to do and requires encoding knowledge in those drivers
>>>>> which really does not belong there:
>>>>>
>>>>> "The problem is that Intel re-uses HIDs between generations and
>>>>> for the Whiskey Cove PMIC we want to not use the ACPI battery
>>>>> and ac drivers on Cherry Trail (where they are known to be
>>>>> broken) but things are different on Apollo Lake. Yet both
>>>>> use the same HID for their companion Whiskey Cove PMIC even
>>>>> though they are 2 completely different revisions of the PMIC
>>>>> (e.g. one uses i2c the other does not).
>>>>>
>>>>> The 2 native drivers have code to detect which revision they
>>>>> are dealing with and exit with -ENODEV if it is not the
>>>>> revision they were written for, but this means that simple
>>>>> HID blacklisting will not work. So IMHO the decision to
>>>>> unregister the  ACPI battery / ac interface really belongs
>>>>> in the native driver as that has all the nitty gritty detail
>>>>> needed to make that decision."
>>>>
>>>>
>>>> So thinking more about this, esp. after receiving a bug report
>>>> from a user getting ACPI errors because of Linux not implementing
>>>> the proprietary undocumented BMOP opregion before the ACPI battery
>>>> driver gets unregistered by the native one, I thing we do indeed
>>>> need to go with a blacklist.
>>>>
>>>> This means also being able to match by _HRV, as Some HIDs are
>>>> re-used for different hardware between Bay Trail / Cherry Trail /
>>>> Broxton with just a bump of _HRV.
>>>>
>>>> I'm currently working on respinning my
>>>> "acpi: utils: Add new acpi_dev_present helper" to address the
>>>> review comments on it. I'm going to give it an optional hrv
>>>> function argument for this use, so as to not having to implement
>>>> hrv checking code in both ac.c and battery.c .
>>>
>>>
>>> So a quick copy paste from another thread, the black-list approach
>>> causes regressions even before hitting -next and seeing any
>>> substantial testing, so we're back to adding unregister functions
>>> and calling those from native PMIC power_supply drivers when the
>>> native power_supply has been successfully registered.
>>>
>>> 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.
>
> OK, but there's one more possibility to consider.
>
> Instead of unregistering the ACPI battery (or AC) driver from the
> platform device driver superseding it, you could clear the
> match_driver flag for the ACPI companion device and call
> device_release_driver() on it, like acpi_bus_trim().  Then (because
> the match_driver flag is unset) the driver core will not try to attach
> the driver to the thing again - until the next invocation of
> acpi_bus_attach() on it, which I think is a bug, because there seems
> to be a flags.visited check missing in there (I need to go back and
> recall why it is not there).
>
> Arguably, that would be a more lightweight way of getting what you
> want without using a blacklist.

The reason I changed my mind on the blacklist approach and went
from unregistering on native-power-supply register to the blacklist
is because some users where seeing a ton of ACPI errors from the
ACPI battery driver trying to use the broken ACPI battery dev in
the DTSD before the native PMIC driver loaded and unregistered
the battery dev. So I really believe the blacklist is the right
approach.

I just got confirmation from the user that the regression on the
Dell 5855 was indeed a false positive.

So from my pov we are good to go with v5 of patch 1 of this
set and v2 (also send as v3 and v4 but unchanged) of patches
2-4 of this set.

Regards,

Hans

Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Rafael J. Wysocki]

On Tue, Apr 11, 2017 at 11:18 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 10-04-17 22:01, Rafael J. Wysocki wrote:
>>
>> On Mon, Apr 10, 2017 at 8:13 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>

[cut]

>
> The reason I changed my mind on the blacklist approach and went
> from unregistering on native-power-supply register to the blacklist
> is because some users where seeing a ton of ACPI errors from the
> ACPI battery driver trying to use the broken ACPI battery dev in
> the DTSD before the native PMIC driver loaded and unregistered
> the battery dev. So I really believe the blacklist is the right
> approach.

Fair enough.

> I just got confirmation from the user that the regression on the
> Dell 5855 was indeed a false positive.
>
> So from my pov we are good to go with v5 of patch 1 of this
> set and v2 (also send as v3 and v4 but unchanged) of patches
> 2-4 of this set.

OK, but please resend the patches you're happy with afresh and let me
know what is superseded by them, so that I don't have to carry out any
research on that.

Thanks,
Rafael