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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hwmon: applesmc implements secondary power supply feature natively instead of using auxiliary device
- [Medium] hwmon: applesmc uses custom mutex instead of hwmon_lock() for external attributes
--

commit 7f74cc774d21a32964e467d3d8da2004ccb22a1e
Author: Jordan Brough <jordan@brough.org>

hwmon: (applesmc) add charge_control_end_threshold support

This commit exposes the MacBook battery charge limit (BCLM) as
charge_control_end_threshold through the power_supply extension API.
It attaches to the battery with ACPI battery hooks and serializes writes
with a newly introduced mutex.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index d0baa10502f7d..b6d9ca3027308 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
[ ... ]
> @@ -724,6 +748,120 @@ static struct platform_driver applesmc_driver = {
>  	},
>  };
>  
> +static DEFINE_MUTEX(applesmc_bat_lock);

[Severity: Medium]
Does this custom lock violate the hwmon subsystem locking guidelines? 

The guidelines mandate that drivers use hwmon_lock() and hwmon_unlock() for
serializing attributes registered by any means other than the info parameter 
of hwmon_device_register_with_info(). Since this lock is used to serialize 
power supply property access, it appears we should be using hwmon_lock() 
instead.

> +
> +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, readback;
> +	int ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
> +		/* 20 is known to work and 10 is not accepted; the floor is unknown. */
> +		if (val->intval < 20 || val->intval > 100)
> +			return -EINVAL;
> +
> +		limit = val->intval;
> +		/* At 100% charging is never cut short, so no margin is needed. */
> +		led_limit = limit < 100 ?
> +			limit - APPLESMC_BATTERY_CHARGE_LIMIT_LED_MARGIN : limit;
> +
> +		scoped_guard(mutex, &applesmc_bat_lock) {

[Severity: Medium]
As mentioned above, should this be using hwmon_lock() to serialize access
instead of the custom applesmc_bat_lock?

> +			ret = applesmc_write_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
[ ... ]
> +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 int applesmc_bat_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	return power_supply_register_extension(battery, &applesmc_bat_ext, &pdev->dev, NULL);
> +}

[Severity: Medium]
Is this direct registration of the power supply extension bypassing the hwmon
subsystem layering rules?

The hardware monitoring subsystem guidelines state that secondary
functionality (like power supply properties) should be implemented as an
auxiliary device driver, with the driver residing in the appropriate
subsystem directory, rather than being registered directly from the hwmon
driver.

> +
> +static int applesmc_bat_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	power_supply_unregister_extension(battery, &applesmc_bat_ext);
> +	return 0;
> +}
> +
> +static struct acpi_battery_hook applesmc_bat_hook = {
> +	.name = "Apple SMC Battery Charge Control",
> +	.add_battery = applesmc_bat_add,
> +	.remove_battery = applesmc_bat_remove,
> +};

[Severity: Medium]
Similar to the power supply extension above, should this ACPI battery hook
also be separated into an auxiliary device driver?

Registering it directly here in the hardware monitoring driver tightly couples
unrelated subsystems and bypasses proper review from the power supply
maintainers.

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