Re: [PATCH v2 2/2] 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:
- [High] hwmon: applesmc: AB-BA deadlock between applesmc_bat_lock and extensions_sem
- [Medium] hwmon: applesmc: secondary battery functionality not implemented as auxiliary device
--

commit 1bfe7b94e596c2681653ce0f082904d5c162cf4d
Author: Jordan Brough <jordan@brough.org>

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 API and ACPI battery hook
infrastructure. It introduces read/write capabilities for battery charge limit
registers (BCLM and BFCL) directly within the hwmon driver.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index d0baa10502f7d..2444bea8a0f19 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
[ ... ]
> @@ -724,6 +751,119 @@ static struct platform_driver applesmc_driver = {
>  	},
>  };
>  
> +static DEFINE_MUTEX(applesmc_bat_lock);
> +
> +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:
> +		scoped_guard(mutex, &applesmc_bat_lock) {

[Severity: High]
Can this introduce an AB-BA deadlock with the power supply core's
extensions_sem?

When userspace reads a property via sysfs, the power supply core's
__power_supply_get_property() acquires psy->extensions_sem for reading
before invoking this get_property callback. This establishes a lock
order where extensions_sem is held before attempting to acquire
applesmc_bat_lock.

However, in applesmc_battery_remove() below, applesmc_bat_lock is
acquired before calling power_supply_unregister_extension(), which
acquires psy->extensions_sem for writing, establishing the reverse lock
order.

> +			ret = applesmc_read_key(BATTERY_CHARGE_LIMIT_KEY, &limit, 1);
> +			if (ret)
> +				return ret;
> +			val->intval = limit;
> +			return 0;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> +static int applesmc_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> +	int ret;
> +
> +	guard(mutex)(&applesmc_bat_lock);
> +	if (applesmc_hooked_battery)
> +		return 0;
> +
> +	ret = power_supply_register_extension(battery, &applesmc_bat_ext, &pdev->dev, NULL);

[Severity: Medium]
Is hooking directly into the power_supply subsystem from within the
hwmon subsystem directory a violation of the hardware monitoring
subsystem layering guidelines?

The hwmon subsystem guidelines explicitly require that secondary
functionality (like battery charge control) be implemented as an
auxiliary device, with the driver residing in the appropriate subsystem
directory.

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

This acquires psy->extensions_sem for writing while applesmc_bat_lock
is already held, completing the AB-BA deadlock cycle mentioned above.

> +	applesmc_hooked_battery = NULL;
> +	return 0;
> +}
> +
[ ... ]
> @@ -1563,6 +1703,10 @@ static int __init applesmc_init(void)
>  		goto out_light_ledclass;
>  	}
>  
> +	if (smcreg.has_battery_charge_limit &&
> +	    (IS_REACHABLE(CONFIG_ACPI_BATTERY) || IS_REACHABLE(CONFIG_ACPI_SBS)))
> +		battery_hook_register(&applesmc_battery_hook);

[Severity: Medium]
Should this secondary battery functionality be implemented as an
auxiliary device instead of being registered natively during the hwmon
driver initialization?

Embedding the ACPI battery hooks directly in drivers/hwmon/applesmc.c
appears to bypass power supply subsystem maintainer review and violate
layering rules.

> +
>  	return 0;
>  
>  out_light_ledclass:

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