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

[sashiko-bot@kernel.org]

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