[PATCH 0/2] hwmon: (applesmc) add native charge_control_end_threshold support

[PATCH 0/2] hwmon: (applesmc) add native charge_control_end_threshold support

[Jordan Brough]

This series adds native charge-limit support to applesmc, so that
generic userspace (UPower, KDE Powerdevil, GNOME Settings) can read
and write a MacBook's charging cutoff through the standard
charge_control_end_threshold sysfs attribute, with no vendor-specific
userspace daemon required.

Patch 1 is a prerequisite: some Macs register their battery through
the ACPI Smart Battery System driver (drivers/acpi/sbs.c) rather than
the ACPI Control Method Battery driver, which is the only one with an
existing hook mechanism for attaching extra power_supply properties.
Patch 1 adds a parallel, purely additive hook mechanism to sbs.c so
patch 2 can support both registration paths symmetrically.

Patch 2 adds the actual charge_control_end_threshold support to
applesmc, using the real SMC BCLM key (not the cosmetic BFCL key,
which only controls the MagSafe LED color and does not affect
charging behavior).

Tested on a MacBookAir6,2 (SBS-registered battery): sysfs attribute
appears, reads/writes round-trip correctly to the real SMC BCLM key,
and this was verified end-to-end through KDE's charge-limit slider
writing directly to sysfs. Also tested on a non-Mac laptop (CmBatt-
registered battery, no BCLM key) to confirm both new hook
registrations are true no-ops there, with no new sysfs attributes and
no behavioral change.

This was previously discussed on this list in 2020, proposed as a
generic sysfs interface for reading/writing arbitrary SMC keys by
name. Guenter raised concerns at the time about that approach adding
non-standard code to the driver:

  Link: https://www.spinics.net/lists/linux-hwmon/msg09839.html

This series takes a different approach: it exposes only the specific,
well-understood BCLM key through the standard power_supply_ext
charge_control_end_threshold property, rather than a generic raw-key
interface, and gates it at runtime on a probe of that specific key so
it stays inert on any hardware that doesn't implement it.

This is my first kernel submission; I'd welcome any correction on
process or style along with the technical review.

Jordan Brough (2):
  ACPI: sbs: add battery hook mechanism for SBS-registered batteries
  hwmon: (applesmc) add charge_control_end_threshold support

 drivers/acpi/sbs.c       | 147 ++++++++++++++++++++++++++++++++++-
 drivers/hwmon/applesmc.c | 164 +++++++++++++++++++++++++++++++++++++--
 include/acpi/battery.h   |  16 ++++
 3 files changed, 321 insertions(+), 6 deletions(-)


base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
-- 
2.55.0

[PATCH 1/2] ACPI: sbs: add battery hook mechanism for SBS-registered batteries

[Jordan Brough]

drivers/acpi/battery.c provides a battery_hook_register()/unregister()
mechanism that lets other drivers (e.g. hwmon drivers exposing vendor-
specific charge control) attach extra power_supply properties to an
ACPI Control Method Battery (HID PNP0C0A, "CmBatt").

Some machines instead expose their battery through the ACPI Smart
Battery System (HID ACPI0002, "SBS") driver in drivers/acpi/sbs.c,
which has no equivalent hook point. On these machines a hwmon driver
has no supported way to attach additional properties to the battery's
power_supply device.

Add a parallel sbs_battery_hook_register()/unregister() mechanism to
drivers/acpi/sbs.c, mirroring the existing CmBatt hook API in name and
behavior so that callers can support both battery registration paths
symmetrically. This is purely additive: no existing hook consumer or
code path in drivers/acpi/battery.c is touched, and sbs.c's own
behavior is unchanged for any driver that does not call the new API.

This is a prerequisite for hwmon/applesmc gaining
charge_control_end_threshold support, since the Intel MacBook hardware
being targeted registers its battery via SBS, not CmBatt.

Signed-off-by: Jordan Brough <jordan@brough.org>
---
 drivers/acpi/sbs.c     | 147 ++++++++++++++++++++++++++++++++++++++++-
 include/acpi/battery.h |  16 +++++
 2 files changed, 162 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 86b7c797585..305bf62eae3 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -10,6 +10,7 @@
 #define pr_fmt(fmt) "ACPI: " fmt
 
 #include <linux/init.h>
+#include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -54,6 +55,7 @@ struct acpi_battery {
 	struct power_supply *bat;
 	struct power_supply_desc bat_desc;
 	struct acpi_sbs *sbs;
+	struct list_head list;		/* node in sbs_hook_battery_list */
 	unsigned long update_time;
 	char name[8];
 	char manufacturer_name[ACPI_SBS_BLOCK_MAX];
@@ -518,6 +520,145 @@ static int acpi_battery_read(struct acpi_battery *battery)
 	return result;
 }
 
+/*
+ * Battery hook support, mirroring the mechanism in drivers/acpi/battery.c
+ * for batteries registered there. That mechanism only sees batteries
+ * added via drivers/acpi/battery.c, not the ones this driver registers,
+ * so drivers wanting to attach optional functionality (e.g. an extra
+ * power_supply_ext) to an SBS battery need this separate copy; see the
+ * comment above the declarations in include/acpi/battery.h.
+ */
+static LIST_HEAD(sbs_hook_list);
+static LIST_HEAD(sbs_hook_battery_list);
+static DEFINE_MUTEX(sbs_hook_mutex);
+
+static void sbs_battery_hook_unregister_unlocked(struct acpi_battery_hook *hook)
+{
+	struct acpi_battery *battery;
+
+	/*
+	 * In order to remove a hook, we first need to
+	 * de-register all the batteries that are registered.
+	 */
+	list_for_each_entry(battery, &sbs_hook_battery_list, list) {
+		if (!hook->remove_battery(battery->bat, hook))
+			power_supply_changed(battery->bat);
+	}
+	list_del_init(&hook->list);
+}
+
+void sbs_battery_hook_unregister(struct acpi_battery_hook *hook)
+{
+	mutex_lock(&sbs_hook_mutex);
+	/*
+	 * Ignore already unregistered battery hooks. This might happen
+	 * if a battery hook was previously unloaded due to an error when
+	 * adding a new battery.
+	 */
+	if (!list_empty(&hook->list))
+		sbs_battery_hook_unregister_unlocked(hook);
+	mutex_unlock(&sbs_hook_mutex);
+}
+EXPORT_SYMBOL_GPL(sbs_battery_hook_unregister);
+
+void sbs_battery_hook_register(struct acpi_battery_hook *hook)
+{
+	struct acpi_battery *battery;
+
+	mutex_lock(&sbs_hook_mutex);
+	list_add(&hook->list, &sbs_hook_list);
+	/*
+	 * Now that the driver is registered, we need
+	 * to notify the hook that a battery is available
+	 * for each battery, so that the driver may add
+	 * its attributes.
+	 */
+	list_for_each_entry(battery, &sbs_hook_battery_list, list) {
+		if (hook->add_battery(battery->bat, hook)) {
+			/*
+			 * If a add-battery returns non-zero,
+			 * the registration of the hook has failed,
+			 * and we will not add it to the list of loaded
+			 * hooks.
+			 */
+			pr_err("hook failed to load: %s", hook->name);
+			sbs_battery_hook_unregister_unlocked(hook);
+			goto end;
+		}
+
+		power_supply_changed(battery->bat);
+	}
+	pr_info("new hook: %s\n", hook->name);
+end:
+	mutex_unlock(&sbs_hook_mutex);
+}
+EXPORT_SYMBOL_GPL(sbs_battery_hook_register);
+
+static void devm_sbs_battery_hook_unregister(void *data)
+{
+	struct acpi_battery_hook *hook = data;
+
+	sbs_battery_hook_unregister(hook);
+}
+
+int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook)
+{
+	sbs_battery_hook_register(hook);
+
+	return devm_add_action_or_reset(dev, devm_sbs_battery_hook_unregister, hook);
+}
+EXPORT_SYMBOL_GPL(devm_sbs_battery_hook_register);
+
+/*
+ * This function gets called right after the battery sysfs
+ * attributes have been added, so that the drivers that
+ * define custom sysfs attributes can add their own.
+ */
+static void sbs_battery_hook_add_battery(struct acpi_battery *battery)
+{
+	struct acpi_battery_hook *hook_node, *tmp;
+
+	mutex_lock(&sbs_hook_mutex);
+	INIT_LIST_HEAD(&battery->list);
+	list_add(&battery->list, &sbs_hook_battery_list);
+	/*
+	 * Since we added a new battery to the list, we need to
+	 * iterate over the hooks and call add_battery for each
+	 * hook that was registered. This usually happens
+	 * when a battery gets hotplugged or initialized
+	 * during the battery module initialization.
+	 */
+	list_for_each_entry_safe(hook_node, tmp, &sbs_hook_list, list) {
+		if (hook_node->add_battery(battery->bat, hook_node)) {
+			/*
+			 * The notification of the hook has failed, to
+			 * prevent further errors we will unload the hook.
+			 */
+			pr_err("error in hook, unloading: %s",
+			       hook_node->name);
+			sbs_battery_hook_unregister_unlocked(hook_node);
+		}
+	}
+	mutex_unlock(&sbs_hook_mutex);
+}
+
+static void sbs_battery_hook_remove_battery(struct acpi_battery *battery)
+{
+	struct acpi_battery_hook *hook;
+
+	mutex_lock(&sbs_hook_mutex);
+	/*
+	 * Before removing the hook, we need to remove all
+	 * custom attributes from the battery.
+	 */
+	list_for_each_entry(hook, &sbs_hook_list, list) {
+		hook->remove_battery(battery->bat, hook);
+	}
+	/* Then, just remove the battery from the list */
+	list_del(&battery->list);
+	mutex_unlock(&sbs_hook_mutex);
+}
+
 /* Smart Battery */
 static int acpi_battery_add(struct acpi_sbs *sbs, int id)
 {
@@ -555,6 +696,8 @@ static int acpi_battery_add(struct acpi_sbs *sbs, int id)
 		goto end;
 	}
 
+	sbs_battery_hook_add_battery(battery);
+
       end:
 	pr_info("%s [%s]: Battery Slot [%s] (battery %s)\n",
 	       ACPI_SBS_DEVICE_NAME, acpi_device_bid(sbs->device),
@@ -566,8 +709,10 @@ static void acpi_battery_remove(struct acpi_sbs *sbs, int id)
 {
 	struct acpi_battery *battery = &sbs->battery[id];
 
-	if (battery->bat)
+	if (battery->bat) {
+		sbs_battery_hook_remove_battery(battery);
 		power_supply_unregister(battery->bat);
+	}
 }
 
 static int acpi_charger_add(struct acpi_sbs *sbs)
diff --git a/include/acpi/battery.h b/include/acpi/battery.h
index c93f16dfb94..ddbfa4ed135 100644
--- a/include/acpi/battery.h
+++ b/include/acpi/battery.h
@@ -18,8 +18,24 @@ struct acpi_battery_hook {
 	struct list_head list;
 };
 
+/*
+ * battery_hook_register() and friends only see batteries registered by
+ * drivers/acpi/battery.c, the ACPI Control Method Battery driver (ACPI HID
+ * "PNP0C0A"). Batteries registered by drivers/acpi/sbs.c, the ACPI Smart
+ * Battery System driver (ACPI HID "ACPI0002", common on hardware with
+ * SMBus/SBS fuel-gauge chips such as many Intel MacBooks), are invisible
+ * to them; use the sbs_battery_hook_* equivalents below for those. A
+ * caller wanting to support both kinds of hardware needs two separate
+ * struct acpi_battery_hook instances, one per registration call, since a
+ * given instance's embedded list node can only belong to one list at a
+ * time.
+ */
 void battery_hook_register(struct acpi_battery_hook *hook);
 void battery_hook_unregister(struct acpi_battery_hook *hook);
 int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
 
+void sbs_battery_hook_register(struct acpi_battery_hook *hook);
+void sbs_battery_hook_unregister(struct acpi_battery_hook *hook);
+int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
+
 #endif
-- 
2.55.0

[PATCH 2/2] hwmon: (applesmc) add charge_control_end_threshold support

[Jordan Brough]

Apple's SMC firmware exposes a real charging-cutoff control via the
BCLM (Battery Charge Limit Max) key, and a separate cosmetic BFCL
(Battery Final Charge Level) key that only controls when the MagSafe
LED switches from orange to green. Userspace tools and out-of-tree
kernel patches have historically conflated the two or exposed neither
through a standard interface, leaving desktop environments unable to
offer a native charge-limit control on Intel MacBooks.

Add native charge_control_end_threshold support to applesmc using the
power_supply_ext extension API, so that generic userspace (UPower,
KDE Powerdevil, GNOME Settings) can read and write the threshold
through the standard sysfs attribute with no vendor-specific daemon.
BFCL is kept in sync automatically (BCLM - 5) purely so the MagSafe
LED still changes color at a sensible point relative to the real
cutoff; it is not independently exposed.

Apple MacBooks register their battery through either the ACPI Control
Method Battery driver (CmBatt, drivers/acpi/battery.c) or the ACPI
Smart Battery System driver (SBS, drivers/acpi/sbs.c), depending on
model. Register against both hook mechanisms, guarded at compile/link
time with IS_REACHABLE() so this stays a no-op when the corresponding
subsystem isn't built in, and at runtime with a probe of the BCLM SMC
key so this is a no-op on any Mac (or non-Mac, given applesmc's DMI
match table) that doesn't actually implement it.

Tested on a MacBookAir6,2 (SBS-registered battery): the sysfs
attribute appears, reads/writes correctly propagate to the real SMC
BCLM key end-to-end via KDE's charge-limit slider, and BFCL tracks
BCLM - 5 as designed. Tested on a non-Mac laptop (CmBatt-registered
battery, no BCLM key) to confirm both new hook registrations are
true no-ops there: no new sysfs attributes, no behavioral change,
clean module load/unload.

Signed-off-by: Jordan Brough <jordan@brough.org>
---
 drivers/hwmon/applesmc.c | 164 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 159 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index d0baa10502f..39276cdba3d 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -33,6 +33,8 @@
 #include <linux/workqueue.h>
 #include <linux/err.h>
 #include <linux/bits.h>
+#include <linux/power_supply.h>
+#include <acpi/battery.h>
 #include <asm/barrier.h>
 
 /* data port used by Apple SMC */
@@ -76,6 +78,26 @@
 
 #define TEMP_SENSOR_TYPE	"sp78"
 
+/*
+ * BCLM caps charging at a percentage; it is the only key that affects
+ * charging behavior. BFCL only controls when the charging LED switches
+ * from orange to green and has no effect on charging itself.
+ */
+#define BATTERY_CHARGE_LIMIT_KEY	"BCLM" /* r/w ui8 */
+#define BATTERY_CHARGE_LIMIT_LED_KEY	"BFCL" /* r/w ui8 */
+
+/*
+ * Margin kept between BCLM and BFCL so the charging LED still reports
+ * "done" once BCLM halts charging below 100%. Left equal to BCLM, the
+ * two can race: charging can stop a moment before the SMC's internal
+ * (sub-percent) charge counter actually reaches BFCL, leaving the LED
+ * stuck on "charging" indefinitely since no further current ever flows
+ * to push it over the threshold. A margin of 1 was sufficient to avoid
+ * this on a MacBookAir6,2; 5 matches Apple's own firmware convention and
+ * gives headroom on hardware we haven't tested.
+ */
+#define APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN 5
+
 /* List of keys used to read/write fan speeds */
 static const char *const fan_speed_fmt[] = {
 	"F%dAc",		/* actual speed */
@@ -131,6 +153,7 @@ static struct applesmc_registers {
 	int num_light_sensors;		/* number of light sensors */
 	bool has_accelerometer;		/* has motion sensor */
 	bool has_key_backlight;		/* has keyboard backlight */
+	bool has_battery_charge_limit;	/* has BCLM battery charge limit */
 	bool init_complete;		/* true when fully initialized */
 	struct applesmc_entry *cache;	/* cached key entries */
 	const char **index;		/* temperature key index */
@@ -633,17 +656,21 @@ static int applesmc_init_smcreg_try(void)
 	if (ret)
 		return ret;
 	ret = applesmc_has_key(BACKLIGHT_KEY, &s->has_key_backlight);
+	if (ret)
+		return ret;
+	ret = applesmc_has_key(BATTERY_CHARGE_LIMIT_KEY, &s->has_battery_charge_limit);
 	if (ret)
 		return ret;
 
 	s->num_light_sensors = left_light_sensor + right_light_sensor;
 	s->init_complete = true;
 
-	pr_info("key=%d fan=%d temp=%d index=%d acc=%d lux=%d kbd=%d\n",
-	       s->key_count, s->fan_count, s->temp_count, s->index_count,
-	       s->has_accelerometer,
-	       s->num_light_sensors,
-	       s->has_key_backlight);
+	pr_info("key=%d fan=%d temp=%d index=%d acc=%d lux=%d kbd=%d bat=%d\n",
+		s->key_count, s->fan_count, s->temp_count, s->index_count,
+		s->has_accelerometer,
+		s->num_light_sensors,
+		s->has_key_backlight,
+		s->has_battery_charge_limit);
 
 	return 0;
 }
@@ -724,6 +751,120 @@ static struct platform_driver applesmc_driver = {
 	},
 };
 
+static int applesmc_bat_get_property(struct power_supply *psy,
+				     const struct power_supply_ext *ext,
+				     void *data,
+				     enum power_supply_property psp,
+				     union power_supply_propval *val)
+{
+	u8 limit;
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		ret = applesmc_read_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
+		if (ret)
+			return ret;
+		val->intval = limit;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int applesmc_bat_set_property(struct power_supply *psy,
+				     const struct power_supply_ext *ext,
+				     void *data,
+				     enum power_supply_property psp,
+				     const union power_supply_propval *val)
+{
+	u8 limit, led_limit;
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		if (val->intval < 20 || val->intval > 100)
+			return -EINVAL;
+
+		limit = val->intval;
+		ret = applesmc_write_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
+		if (ret)
+			return ret;
+
+		led_limit = limit > APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN ?
+			limit - APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN : limit;
+		applesmc_write_key(BATTERY_CHARGE_LIMIT_LED_KEY, &led_limit, 1);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int applesmc_bat_property_is_writeable(struct power_supply *psy,
+					      const struct power_supply_ext *ext,
+					      void *data,
+					      enum power_supply_property psp)
+{
+	return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD;
+}
+
+static const enum power_supply_property applesmc_bat_props[] = {
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
+};
+
+static const struct power_supply_ext applesmc_bat_ext = {
+	.name			= "applesmc-charge-control",
+	.properties		= applesmc_bat_props,
+	.num_properties		= ARRAY_SIZE(applesmc_bat_props),
+	.get_property		= applesmc_bat_get_property,
+	.set_property		= applesmc_bat_set_property,
+	.property_is_writeable	= applesmc_bat_property_is_writeable,
+};
+
+static struct power_supply *applesmc_hooked_battery;
+
+static int applesmc_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	if (applesmc_hooked_battery)
+		return 0;
+
+	applesmc_hooked_battery = battery;
+	return power_supply_register_extension(battery, &applesmc_bat_ext, &pdev->dev, NULL);
+}
+
+static int applesmc_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
+{
+	if (applesmc_hooked_battery != battery)
+		return 0;
+
+	power_supply_unregister_extension(battery, &applesmc_bat_ext);
+	applesmc_hooked_battery = NULL;
+	return 0;
+}
+
+/*
+ * Two separate hook registrations are required: Intel Macs register
+ * their battery through either drivers/acpi/battery.c (ACPI Control
+ * Method Battery) or drivers/acpi/sbs.c (ACPI Smart Battery System,
+ * common on hardware with SMBus/SBS fuel-gauge chips), depending on
+ * model, and each mechanism only sees batteries registered through
+ * itself. A single struct acpi_battery_hook cannot be registered with
+ * both, since its embedded list node can only belong to one list at a
+ * time. Whichever mechanism matches the actual hardware will hook the
+ * battery; the other simply never finds one to call back for.
+ */
+static struct acpi_battery_hook applesmc_battery_hook = {
+	.name = "Apple SMC Battery Charge Control",
+	.add_battery = applesmc_battery_add,
+	.remove_battery = applesmc_battery_remove,
+};
+
+static struct acpi_battery_hook applesmc_sbs_battery_hook = {
+	.name = "Apple SMC Battery Charge Control (SBS)",
+	.add_battery = applesmc_battery_add,
+	.remove_battery = applesmc_battery_remove,
+};
+
 /*
  * applesmc_calibrate - Set our "resting" values.  Callers must
  * hold applesmc_lock.
@@ -1563,6 +1704,13 @@ static int __init applesmc_init(void)
 		goto out_light_ledclass;
 	}
 
+	if (smcreg.has_battery_charge_limit) {
+		if (IS_REACHABLE(CONFIG_ACPI_BATTERY))
+			battery_hook_register(&applesmc_battery_hook);
+		if (IS_REACHABLE(CONFIG_ACPI_SBS))
+			sbs_battery_hook_register(&applesmc_sbs_battery_hook);
+	}
+
 	return 0;
 
 out_light_ledclass:
@@ -1589,6 +1737,12 @@ static int __init applesmc_init(void)
 
 static void __exit applesmc_exit(void)
 {
+	if (smcreg.has_battery_charge_limit) {
+		if (IS_REACHABLE(CONFIG_ACPI_BATTERY))
+			battery_hook_unregister(&applesmc_battery_hook);
+		if (IS_REACHABLE(CONFIG_ACPI_SBS))
+			sbs_battery_hook_unregister(&applesmc_sbs_battery_hook);
+	}
 	hwmon_device_unregister(hwmon_dev);
 	applesmc_release_key_backlight();
 	applesmc_release_light_sensor();
-- 
2.55.0

Re: [PATCH 1/2] ACPI: sbs: add battery hook mechanism for SBS-registered batteries

[sashiko-bot]

> drivers/acpi/battery.c provides a battery_hook_register()/unregister()
> mechanism that lets other drivers (e.g. hwmon drivers exposing vendor-
> specific charge control) attach extra power_supply properties to an
> ACPI Control Method Battery (HID PNP0C0A, "CmBatt").
> 
> Some machines instead expose their battery through the ACPI Smart
> Battery System (HID ACPI0002, "SBS") driver in drivers/acpi/sbs.c,
> which has no equivalent hook point. On these machines a hwmon driver
> has no supported way to attach additional properties to the battery's
> power_supply device.
> 
> Add a parallel sbs_battery_hook_register()/unregister() mechanism to
> drivers/acpi/sbs.c, mirroring the existing CmBatt hook API in name and
> behavior so that callers can support both battery registration paths
> symmetrically. This is purely additive: no existing hook consumer or
> code path in drivers/acpi/battery.c is touched, and sbs.c's own
> behavior is unchanged for any driver that does not call the new API.
> 
> This is a prerequisite for hwmon/applesmc gaining
> charge_control_end_threshold support, since the Intel MacBook hardware
> being targeted registers its battery via SBS, not CmBatt.
> 
> Signed-off-by: Jordan Brough <jordan@brough.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913231410.416922-1-jordan@brough.org?part=1

Re: [PATCH 2/2] hwmon: (applesmc) add charge_control_end_threshold support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Concurrent battery additions cause a TOCTOU data race on the `applesmc_hooked_battery` global pointer, risking leaked extensions and Use-After-Free.
- [Medium] Secondary functionality (battery charge limit) is implemented directly in the hwmon driver instead of using an auxiliary device.
- [Medium] Multi-variable race condition when updating BCLM and BFCL SMC keys.
- [Low] Incorrect state assignment on failure causes a spurious kernel warning during rollback.
- [Low] The newly registered power supply attribute callbacks do not use the mandated `hwmon_lock()` API for mutual exclusion.
--

commit 31b80fd6344420398af17504b367d0f876c4eee2
Author: Jordan Brough <jordan@brough.org>
Subject: hwmon: (applesmc) add charge_control_end_threshold support

This commit adds native charge_control_end_threshold support to the applesmc
driver using the power_supply_ext extension API. It allows standard userspace
tools to read and write the BCLM threshold via sysfs, while automatically
keeping the cosmetic BFCL key synchronized.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index d0baa10502f7d..39276cdba3da7 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
[ ... ]
> @@ -724,6 +751,120 @@ static struct platform_driver applesmc_driver = {
>  	},
>  };
>  
> +static int applesmc_bat_get_property(struct power_supply *psy,
[ ... ]
> +static int applesmc_bat_set_property(struct power_supply *psy,
> +				     const struct power_supply_ext *ext,
> +				     void *data,
> +				     enum power_supply_property psp,
> +				     const union power_supply_propval *val)
> +{
> +	u8 limit, led_limit;
> +	int ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
> +		if (val->intval < 20 || val->intval > 100)
> +			return -EINVAL;
> +
> +		limit = val->intval;
> +		ret = applesmc_write_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);

[Severity: Low]
Are we required to use hwmon_lock() here? The hwmon subsystem guidelines
state that drivers must use hwmon_lock() and hwmon_unlock() to serialize
operations for attributes registered out-of-band. These power supply
extension callbacks seem to rely only on internal driver locks without invoking
the mandated hwmon API.

[Severity: Medium]
Could concurrent sysfs writes to the charge control threshold attribute cause
the BCLM and BFCL keys to become desynchronized?

Since these two sequential writes happen without a unifying lock across the
entire function block, it appears that if two sysfs writes interleave, the
hardware could end up with mismatched keys, breaking the LED state machine.

> +		if (ret)
> +			return ret;
> +
> +		led_limit = limit > APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN ?
> +			limit - APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN : limit;
> +		applesmc_write_key(BATTERY_CHARGE_LIMIT_LED_KEY, &led_limit, 1);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> +static const struct power_supply_ext applesmc_bat_ext = {
> +	.name			= "applesmc-charge-control",
> +	.properties		= applesmc_bat_props,
> +	.num_properties		= ARRAY_SIZE(applesmc_bat_props),
> +	.get_property		= applesmc_bat_get_property,
> +	.set_property		= applesmc_bat_set_property,
> +	.property_is_writeable	= applesmc_bat_property_is_writeable,
> +};

[Severity: Medium]
Does embedding this power supply extension natively inside the hwmon driver
violate the auxiliary device architecture mandate? 

The subsystem guidelines indicate that secondary functionalities should be
isolated into auxiliary devices and placed in the appropriate subsystem
directory. By doing this here, the hwmon driver bypasses the auxiliary device
framework and registers itself directly into the ACPI battery subsystem.

> +
> +static struct power_supply *applesmc_hooked_battery;
> +
> +static int applesmc_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	if (applesmc_hooked_battery)
> +		return 0;
> +
> +	applesmc_hooked_battery = battery;

[Severity: High]
Is there a potential data race on the applesmc_hooked_battery global pointer
here?

If concurrent battery hotplug events are triggered by the ACPI and SBS
subsystems, could two threads race during this lockless check-and-set sequence?
It seems like both threads might register the extension, but only one would
ultimately be recorded in the global pointer. If that happens, would the
unrecorded extension be leaked and cause a use-after-free on module unload,
since applesmc_battery_remove() only unregisters the recorded battery?

> +	return power_supply_register_extension(battery, &applesmc_bat_ext, &pdev->dev, NULL);

[Severity: Low]
What happens if power_supply_register_extension() fails? 

The global applesmc_hooked_battery pointer was already assigned just prior to
this call. Does this leave the pointer set on failure, such that the ACPI
battery hook core's removal hook will later invoke
power_supply_unregister_extension() on a never-registered extension and trigger
a kernel warning?

> +}
> +
> +static int applesmc_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	if (applesmc_hooked_battery != battery)
> +		return 0;
> +
> +	power_supply_unregister_extension(battery, &applesmc_bat_ext);
> +	applesmc_hooked_battery = NULL;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913231410.416922-1-jordan@brough.org?part=2

Re: [PATCH 1/2] ACPI: sbs: add battery hook mechanism for SBS-registered batteries

[Rafael J. Wysocki (Intel)]

On Mon, Sep 14, 2026 at 1:14 AM Jordan Brough <jordan@brough.org> wrote:
>
> drivers/acpi/battery.c provides a battery_hook_register()/unregister()
> mechanism that lets other drivers (e.g. hwmon drivers exposing vendor-
> specific charge control) attach extra power_supply properties to an
> ACPI Control Method Battery (HID PNP0C0A, "CmBatt").
>
> Some machines instead expose their battery through the ACPI Smart
> Battery System (HID ACPI0002, "SBS") driver in drivers/acpi/sbs.c,
> which has no equivalent hook point. On these machines a hwmon driver
> has no supported way to attach additional properties to the battery's
> power_supply device.
>
> Add a parallel sbs_battery_hook_register()/unregister() mechanism to
> drivers/acpi/sbs.c, mirroring the existing CmBatt hook API in name and
> behavior so that callers can support both battery registration paths
> symmetrically. This is purely additive: no existing hook consumer or
> code path in drivers/acpi/battery.c is touched, and sbs.c's own
> behavior is unchanged for any driver that does not call the new API.
>
> This is a prerequisite for hwmon/applesmc gaining
> charge_control_end_threshold support, since the Intel MacBook hardware
> being targeted registers its battery via SBS, not CmBatt.
>
> Signed-off-by: Jordan Brough <jordan@brough.org>

Acked-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>

and feel free to route this patch along with the hwmon changes depending on it.

Thanks!

> ---
>  drivers/acpi/sbs.c     | 147 ++++++++++++++++++++++++++++++++++++++++-
>  include/acpi/battery.h |  16 +++++
>  2 files changed, 162 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index 86b7c797585..305bf62eae3 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c
> @@ -10,6 +10,7 @@
>  #define pr_fmt(fmt) "ACPI: " fmt
>
>  #include <linux/init.h>
> +#include <linux/list.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> @@ -54,6 +55,7 @@ struct acpi_battery {
>         struct power_supply *bat;
>         struct power_supply_desc bat_desc;
>         struct acpi_sbs *sbs;
> +       struct list_head list;          /* node in sbs_hook_battery_list */
>         unsigned long update_time;
>         char name[8];
>         char manufacturer_name[ACPI_SBS_BLOCK_MAX];
> @@ -518,6 +520,145 @@ static int acpi_battery_read(struct acpi_battery *battery)
>         return result;
>  }
>
> +/*
> + * Battery hook support, mirroring the mechanism in drivers/acpi/battery.c
> + * for batteries registered there. That mechanism only sees batteries
> + * added via drivers/acpi/battery.c, not the ones this driver registers,
> + * so drivers wanting to attach optional functionality (e.g. an extra
> + * power_supply_ext) to an SBS battery need this separate copy; see the
> + * comment above the declarations in include/acpi/battery.h.
> + */
> +static LIST_HEAD(sbs_hook_list);
> +static LIST_HEAD(sbs_hook_battery_list);
> +static DEFINE_MUTEX(sbs_hook_mutex);
> +
> +static void sbs_battery_hook_unregister_unlocked(struct acpi_battery_hook *hook)
> +{
> +       struct acpi_battery *battery;
> +
> +       /*
> +        * In order to remove a hook, we first need to
> +        * de-register all the batteries that are registered.
> +        */
> +       list_for_each_entry(battery, &sbs_hook_battery_list, list) {
> +               if (!hook->remove_battery(battery->bat, hook))
> +                       power_supply_changed(battery->bat);
> +       }
> +       list_del_init(&hook->list);
> +}
> +
> +void sbs_battery_hook_unregister(struct acpi_battery_hook *hook)
> +{
> +       mutex_lock(&sbs_hook_mutex);
> +       /*
> +        * Ignore already unregistered battery hooks. This might happen
> +        * if a battery hook was previously unloaded due to an error when
> +        * adding a new battery.
> +        */
> +       if (!list_empty(&hook->list))
> +               sbs_battery_hook_unregister_unlocked(hook);
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(sbs_battery_hook_unregister);
> +
> +void sbs_battery_hook_register(struct acpi_battery_hook *hook)
> +{
> +       struct acpi_battery *battery;
> +
> +       mutex_lock(&sbs_hook_mutex);
> +       list_add(&hook->list, &sbs_hook_list);
> +       /*
> +        * Now that the driver is registered, we need
> +        * to notify the hook that a battery is available
> +        * for each battery, so that the driver may add
> +        * its attributes.
> +        */
> +       list_for_each_entry(battery, &sbs_hook_battery_list, list) {
> +               if (hook->add_battery(battery->bat, hook)) {
> +                       /*
> +                        * If a add-battery returns non-zero,
> +                        * the registration of the hook has failed,
> +                        * and we will not add it to the list of loaded
> +                        * hooks.
> +                        */
> +                       pr_err("hook failed to load: %s", hook->name);
> +                       sbs_battery_hook_unregister_unlocked(hook);
> +                       goto end;
> +               }
> +
> +               power_supply_changed(battery->bat);
> +       }
> +       pr_info("new hook: %s\n", hook->name);
> +end:
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +EXPORT_SYMBOL_GPL(sbs_battery_hook_register);
> +
> +static void devm_sbs_battery_hook_unregister(void *data)
> +{
> +       struct acpi_battery_hook *hook = data;
> +
> +       sbs_battery_hook_unregister(hook);
> +}
> +
> +int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook)
> +{
> +       sbs_battery_hook_register(hook);
> +
> +       return devm_add_action_or_reset(dev, devm_sbs_battery_hook_unregister, hook);
> +}
> +EXPORT_SYMBOL_GPL(devm_sbs_battery_hook_register);
> +
> +/*
> + * This function gets called right after the battery sysfs
> + * attributes have been added, so that the drivers that
> + * define custom sysfs attributes can add their own.
> + */
> +static void sbs_battery_hook_add_battery(struct acpi_battery *battery)
> +{
> +       struct acpi_battery_hook *hook_node, *tmp;
> +
> +       mutex_lock(&sbs_hook_mutex);
> +       INIT_LIST_HEAD(&battery->list);
> +       list_add(&battery->list, &sbs_hook_battery_list);
> +       /*
> +        * Since we added a new battery to the list, we need to
> +        * iterate over the hooks and call add_battery for each
> +        * hook that was registered. This usually happens
> +        * when a battery gets hotplugged or initialized
> +        * during the battery module initialization.
> +        */
> +       list_for_each_entry_safe(hook_node, tmp, &sbs_hook_list, list) {
> +               if (hook_node->add_battery(battery->bat, hook_node)) {
> +                       /*
> +                        * The notification of the hook has failed, to
> +                        * prevent further errors we will unload the hook.
> +                        */
> +                       pr_err("error in hook, unloading: %s",
> +                              hook_node->name);
> +                       sbs_battery_hook_unregister_unlocked(hook_node);
> +               }
> +       }
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +
> +static void sbs_battery_hook_remove_battery(struct acpi_battery *battery)
> +{
> +       struct acpi_battery_hook *hook;
> +
> +       mutex_lock(&sbs_hook_mutex);
> +       /*
> +        * Before removing the hook, we need to remove all
> +        * custom attributes from the battery.
> +        */
> +       list_for_each_entry(hook, &sbs_hook_list, list) {
> +               hook->remove_battery(battery->bat, hook);
> +       }
> +       /* Then, just remove the battery from the list */
> +       list_del(&battery->list);
> +       mutex_unlock(&sbs_hook_mutex);
> +}
> +
>  /* Smart Battery */
>  static int acpi_battery_add(struct acpi_sbs *sbs, int id)
>  {
> @@ -555,6 +696,8 @@ static int acpi_battery_add(struct acpi_sbs *sbs, int id)
>                 goto end;
>         }
>
> +       sbs_battery_hook_add_battery(battery);
> +
>        end:
>         pr_info("%s [%s]: Battery Slot [%s] (battery %s)\n",
>                ACPI_SBS_DEVICE_NAME, acpi_device_bid(sbs->device),
> @@ -566,8 +709,10 @@ static void acpi_battery_remove(struct acpi_sbs *sbs, int id)
>  {
>         struct acpi_battery *battery = &sbs->battery[id];
>
> -       if (battery->bat)
> +       if (battery->bat) {
> +               sbs_battery_hook_remove_battery(battery);
>                 power_supply_unregister(battery->bat);
> +       }
>  }
>
>  static int acpi_charger_add(struct acpi_sbs *sbs)
> diff --git a/include/acpi/battery.h b/include/acpi/battery.h
> index c93f16dfb94..ddbfa4ed135 100644
> --- a/include/acpi/battery.h
> +++ b/include/acpi/battery.h
> @@ -18,8 +18,24 @@ struct acpi_battery_hook {
>         struct list_head list;
>  };
>
> +/*
> + * battery_hook_register() and friends only see batteries registered by
> + * drivers/acpi/battery.c, the ACPI Control Method Battery driver (ACPI HID
> + * "PNP0C0A"). Batteries registered by drivers/acpi/sbs.c, the ACPI Smart
> + * Battery System driver (ACPI HID "ACPI0002", common on hardware with
> + * SMBus/SBS fuel-gauge chips such as many Intel MacBooks), are invisible
> + * to them; use the sbs_battery_hook_* equivalents below for those. A
> + * caller wanting to support both kinds of hardware needs two separate
> + * struct acpi_battery_hook instances, one per registration call, since a
> + * given instance's embedded list node can only belong to one list at a
> + * time.
> + */
>  void battery_hook_register(struct acpi_battery_hook *hook);
>  void battery_hook_unregister(struct acpi_battery_hook *hook);
>  int devm_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
>
> +void sbs_battery_hook_register(struct acpi_battery_hook *hook);
> +void sbs_battery_hook_unregister(struct acpi_battery_hook *hook);
> +int devm_sbs_battery_hook_register(struct device *dev, struct acpi_battery_hook *hook);
> +
>  #endif
> --
> 2.55.0
>

Re: [PATCH 1/2] ACPI: sbs: add battery hook mechanism for SBS-registered batteries

[Thomas Weißschuh]

On 2026-09-13 17:14:09-0600, Jordan Brough wrote:
(...)

> Add a parallel sbs_battery_hook_register()/unregister() mechanism to
> drivers/acpi/sbs.c, mirroring the existing CmBatt hook API in name and
> behavior so that callers can support both battery registration paths
> symmetrically. This is purely additive: no existing hook consumer or
> code path in drivers/acpi/battery.c is touched, and sbs.c's own
> behavior is unchanged for any driver that does not call the new API.

(...)

> ---
>  drivers/acpi/sbs.c     | 147 ++++++++++++++++++++++++++++++++++++++++-
>  include/acpi/battery.h |  16 +++++
>  2 files changed, 162 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
> index 86b7c797585..305bf62eae3 100644
> --- a/drivers/acpi/sbs.c
> +++ b/drivers/acpi/sbs.c

(...)

> +/*
> + * Battery hook support, mirroring the mechanism in drivers/acpi/battery.c
> + * for batteries registered there. That mechanism only sees batteries
> + * added via drivers/acpi/battery.c, not the ones this driver registers,
> + * so drivers wanting to attach optional functionality (e.g. an extra
> + * power_supply_ext) to an SBS battery need this separate copy; see the
> + * comment above the declarations in include/acpi/battery.h.
> + */

(...)

This completely duplicates the hooking logic in battery.c.
Also it forces drivers to register both kinds of hooks explicitly.

Couldn't we have a single shared hooking infrastructure and registry
which avoid both issues?

(...)


Thomas