Re: [PATCH v2] platform/x86: lenovo-wmi-other: Deconflict battery with thinkpad_acpi

[Rong Zhang <i@rong.moe>]

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

Hi Aaron,

Thanks for working on this.

On Thu, 2026-11-12 at 15:48 +0800, Aaron Ma wrote:
> ThinkPads expose battery charge thresholds and charge_behaviour through
> thinkpad_acpi's ACPI battery hook using raw sysfs attribute groups. The
> Lenovo WMI "Other Mode" driver also registers a power_supply_ext for
> charge_behaviour and charge_types on systems advertising those WMI
> capabilities.
> 
> When lenovo-wmi-other registers first, thinkpad_acpi fails to create a
> duplicate charge_behaviour sysfs attribute and unregisters its battery
> extension. Conversely, registering or updating any power_supply_ext on
> the same battery runs sysfs_update_groups(), which removes standard
> power-supply attributes not owned by power_supply_core and wipes out
> thinkpad_acpi's charge_control_start_threshold,
> charge_control_end_threshold, and charge_behaviour files.
> 
> Deconflict both ideapad_laptop (VPC2004 with GBMD and SBMC) and
> thinkpad_acpi (LEN0268, LEN0068, or IBM0068 with BDSG and BICG) via a
> table check when the corresponding driver is enabled in Kconfig, and
> update the force_load_psy_ext description accordingly.
> 
> Fixes: 9ca8fc065b88b ("platform/x86: lenovo-wmi-other: Add WMI battery charge limiting")
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
> Changes in v2:
> - Consolidate IdeaPad and ThinkPad deconfliction into a table of ACPI
>   device IDs, method pairs, and driver configuration checks, as suggested
>   by Derek J. Clark.
> - Check GBMD and SBMC for IdeaPads, and BDSG and BICG for ThinkPads,
>   instead of relying only on ThinkPad ACPI device presence.
> - Check whether the corresponding driver is enabled before looking up
>   its ACPI device, including for ideapad_laptop.
> - Update the force_load_psy_ext description to cover both drivers.
> 
> v1: https://lore.kernel.org/platform-driver-x86/20261111083529.727285-1-aaron.ma@canonical.com/
> 
>  drivers/platform/x86/lenovo/wmi-other.c | 51 ++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> index bd64e7b44669d..14b2c3f4cc7ae 100644
> --- a/drivers/platform/x86/lenovo/wmi-other.c
> +++ b/drivers/platform/x86/lenovo/wmi-other.c
> @@ -189,10 +189,10 @@ MODULE_PARM_DESC(relax_fan_constraint,
>  static bool force_load_psy_ext;
>  module_param(force_load_psy_ext, bool, 0444);
>  MODULE_PARM_DESC(force_load_psy_ext,
> -	"This option will skip checking if the ideapad_laptop driver will conflict "
> +	"This option will skip checking if the ideapad_laptop or thinkpad_acpi driver will conflict "
>  	"with adding an extension to set the battery charge behavior and battery charge "
>  	"control end threshold. It will also skip checking if the BIOS reports that "
> -	"those features are fully supported. It is recommended to blacklist the ideapad "
> +	"those features are fully supported. It is recommended to blacklist the conflicting "
>  	"driver before using this option.");
>  
>  /* ======== HWMON (component: lenovo-wmi-capdata 00 & fan) ======== */
> @@ -905,17 +905,32 @@ static acpi_status lwmi_acpi_match(acpi_handle handle, u32 lvl,
>  	return AE_CTRL_TERMINATE;
>  }
>  
> +struct lwmi_psy_deconflict {
> +	const char *hid;
> +	char * const *methods;
> +	bool driver_enabled;

Why is this required?

IMO the relevant items should be wrapped by #if IS_ENABLED(CONFIG_*). In
this manner, lwmi_om_psy_ext_init() doesn't need to iterate through
useless items in vain.

> +};
> +
> +static char * const lwmi_ideapad_psy_methods[] = { "GBMD", "SBMC" };
> +static char * const lwmi_thinkpad_psy_methods[] = { "BDSG", "BICG" };
> +
> +static const struct lwmi_psy_deconflict lwmi_psy_deconflict_list[] = {
> +	{ "VPC2004", lwmi_ideapad_psy_methods, IS_ENABLED(CONFIG_IDEAPAD_LAPTOP) },
> +	{ "LEN0268", lwmi_thinkpad_psy_methods, IS_ENABLED(CONFIG_THINKPAD_ACPI) },
> +	{ "LEN0068", lwmi_thinkpad_psy_methods, IS_ENABLED(CONFIG_THINKPAD_ACPI) },
> +	{ "IBM0068", lwmi_thinkpad_psy_methods, IS_ENABLED(CONFIG_THINKPAD_ACPI) },
> +};

It could be flattened and simplified into a 2-items array. See the
attached patch (untested, may be completely nonsense).

If you adopt my approach, feel free to add

   Co-developed-by: Rong Zhang <i@rong.moe>
   Signed-off-by: Rong Zhang <i@rong.moe>

Thanks,
Rong

> +
>  /**
>   * lwmi_om_psy_ext_init() - Hooks power supply extension to device battery
>   * @priv: Pointer to the lwmi_om_priv drvdata.
>   *
> - * Checks if the ideapad_laptop driver is going to manage charge attributes first,
> - * then if not, hooks the battery to our WMI methods if they are supported.
> + * Checks if the ideapad_laptop or thinkpad_acpi driver is going to manage
> + * charge attributes first, then if not, hooks the battery to our WMI methods
> + * if they are supported.
>   */
>  static void lwmi_om_psy_ext_init(struct lwmi_om_priv *priv)
>  {
> -	static const char * const ideapad_hid = "VPC2004";
> -	acpi_handle handle = NULL;
>  	unsigned int props = 0;
>  	int ret;
>  
> @@ -928,14 +943,24 @@ static void lwmi_om_psy_ext_init(struct lwmi_om_priv *priv)
>  	if (force_load_psy_ext)
>  		goto load_psy_ext;
>  
> -	/* Deconflict ideapad_laptop driver */
> -	ret = acpi_get_devices(ideapad_hid, lwmi_acpi_match, &handle, NULL);
> -	if (ret)
> -		return;
> +	for (unsigned int i = 0; i < ARRAY_SIZE(lwmi_psy_deconflict_list); i++) {
> +		const struct lwmi_psy_deconflict *entry = &lwmi_psy_deconflict_list[i];
> +		acpi_handle handle = NULL;
>  
> -	if (handle && acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC")) {
> -		dev_dbg(&priv->wdev->dev, "ideapad_laptop driver manages battery for device\n");
> -		return;
> +		if (!entry->driver_enabled)
> +			continue;
> +
> +		ret = acpi_get_devices(entry->hid, lwmi_acpi_match, &handle, NULL);
> +		if (ret || !handle)
> +			continue;
> +
> +		if (acpi_has_method(handle, entry->methods[0]) &&
> +		    acpi_has_method(handle, entry->methods[1])) {
> +			dev_dbg(&priv->wdev->dev,
> +				"ACPI device %s manages battery for device\n",
> +				entry->hid);
> +			return;
> +		}
>  	}
>  
>  load_psy_ext:

[-- Attachment #2: flattened-charpp.patch --]
[-- Type: text/x-patch, Size: 3680 bytes --]

diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
index bd64e7b44669..0172c4ceed58 100644
--- a/drivers/platform/x86/lenovo/wmi-other.c
+++ b/drivers/platform/x86/lenovo/wmi-other.c
@@ -189,10 +189,10 @@ MODULE_PARM_DESC(relax_fan_constraint,
 static bool force_load_psy_ext;
 module_param(force_load_psy_ext, bool, 0444);
 MODULE_PARM_DESC(force_load_psy_ext,
-	"This option will skip checking if the ideapad_laptop driver will conflict "
+	"This option will skip checking if the ideapad_laptop or thinkpad_acpi driver will conflict "
 	"with adding an extension to set the battery charge behavior and battery charge "
 	"control end threshold. It will also skip checking if the BIOS reports that "
-	"those features are fully supported. It is recommended to blacklist the ideapad "
+	"those features are fully supported. It is recommended to blacklist the conflicting "
 	"driver before using this option.");
 
 /* ======== HWMON (component: lenovo-wmi-capdata 00 & fan) ======== */
@@ -905,17 +905,39 @@ static acpi_status lwmi_acpi_match(acpi_handle handle, u32 lvl,
 	return AE_CTRL_TERMINATE;
 }
 
+struct lwmi_psy_deconflict {
+	const char *const *hids;
+	const char *const *methods;
+};
+
+#define LWMI_PSY_DECONFLICT_STRS(...) \
+	(const char *[]){ __VA_ARGS__, NULL }
+
+static const struct lwmi_psy_deconflict lwmi_psy_deconflict_list[] = {
+#if IS_ENABLED(CONFIG_IDEAPAD_LAPTOP)
+	{
+		.hids = LWMI_PSY_DECONFLICT_STRS("VPC2004"),
+		.methods = LWMI_PSY_DECONFLICT_STRS("GBMD", "SBMC"),
+	},
+#endif /* IS_ENABLED(CONFIG_IDEAPAD_LAPTOP) */
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+	{
+		.hids = LWMI_PSY_DECONFLICT_STRS("LEN0268", "LEN0068", "IBM0068"),
+		.methods = LWMI_PSY_DECONFLICT_STRS("BDSG", "BICG"),
+	},
+#endif /* IS_ENABLED(CONFIG_THINKPAD_ACPI) */
+};
+
 /**
  * lwmi_om_psy_ext_init() - Hooks power supply extension to device battery
  * @priv: Pointer to the lwmi_om_priv drvdata.
  *
- * Checks if the ideapad_laptop driver is going to manage charge attributes first,
- * then if not, hooks the battery to our WMI methods if they are supported.
+ * Checks if the ideapad_laptop or thinkpad_acpi driver is going to manage
+ * charge attributes first, then if not, hooks the battery to our WMI methods
+ * if they are supported.
  */
 static void lwmi_om_psy_ext_init(struct lwmi_om_priv *priv)
 {
-	static const char * const ideapad_hid = "VPC2004";
-	acpi_handle handle = NULL;
 	unsigned int props = 0;
 	int ret;
 
@@ -928,14 +950,30 @@ static void lwmi_om_psy_ext_init(struct lwmi_om_priv *priv)
 	if (force_load_psy_ext)
 		goto load_psy_ext;
 
-	/* Deconflict ideapad_laptop driver */
-	ret = acpi_get_devices(ideapad_hid, lwmi_acpi_match, &handle, NULL);
-	if (ret)
-		return;
-
-	if (handle && acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC")) {
-		dev_dbg(&priv->wdev->dev, "ideapad_laptop driver manages battery for device\n");
-		return;
+	for (unsigned int i = 0; i < ARRAY_SIZE(lwmi_psy_deconflict_list); i++) {
+		const struct lwmi_psy_deconflict *entry = &lwmi_psy_deconflict_list[i];
+		acpi_handle handle = NULL;
+
+		for (unsigned int j = 0; entry->hids[j]; j++) {
+			const char *hid = entry->hids[j];
+			bool has_methods = true;
+
+			ret = acpi_get_devices(hid, lwmi_acpi_match, &handle, NULL);
+			if (ret || !handle)
+				continue;
+
+			for (unsigned int k = 0; entry->methods[k]; k++) {
+				has_methods = acpi_has_method(handle, (char *)entry->methods[k]);
+				if (!has_methods)
+					break;
+			}
+			if (!has_methods)
+				continue;
+
+			dev_dbg(&priv->wdev->dev, "ACPI device %s manages battery for device\n",
+				hid);
+			return;
+		}
 	}
 
 load_psy_ext: