Re: [PATCH 2/3] hwmon: (acpi_power_meter) Prevent spurious power alarm when power is unknown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: acpi_power_meter: get_power_alarm_state() re-latches software alarm
--

commit f84e0084ffbbd96f84c563d4aa65b6dcc7bb7480
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (acpi_power_meter) Prevent spurious power alarm when power is unknown

This commit updates get_power_alarm_state() to return a boolean alarm state
and ignore unknown power readings (UNKNOWN_POWER_32 and UNKNOWN_POWER_64) as
well as meter or cap update errors when evaluating the power alarm.

> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 38d6a1e56ffcc..1f1d58995693c 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -377,28 +377,30 @@ static int set_avg_interval(struct acpi_power_meter_resource *resource,
>  	return 0;
>  }
>  
> -static int get_power_alarm_state(struct acpi_power_meter_resource *resource,
> -				 long *val)
> +static bool get_power_alarm_state(struct acpi_power_meter_resource *resource)
>  {
> +	bool alarm = false;
>  	int ret;
>  
> +	if (resource->caps.flags & POWER_METER_CAN_NOTIFY)
> +		alarm = resource->power_alarm;
> +	resource->power_alarm = false;
> +
>  	ret = update_meter(resource);
> -	if (ret)
> -		return ret;
> +	if (ret ||
> +	    resource->power == UNKNOWN_POWER_32 ||
> +	    resource->power == UNKNOWN_POWER_64)
> +		return alarm;
>  
>  	/* need to update cap if not to support the notification. */
>  	if (!(resource->caps.flags & POWER_METER_CAN_NOTIFY)) {
>  		ret = update_cap(resource);
>  		if (ret)
> -			return ret;
> -		resource->power_alarm = resource->power > resource->cap;
> -		*val = resource->power_alarm;
> -	} else {
> -		*val = resource->power_alarm || resource->power > resource->cap;
> -		resource->power_alarm = resource->power > resource->cap;
> +			return false;
>  	}
>  
> -	return 0;
> +	resource->power_alarm = resource->power > resource->cap;
> +	return alarm || resource->power_alarm;
>  }

[Severity: Medium]
When POWER_METER_CAN_NOTIFY is set in resource->caps.flags, does assigning
resource->power_alarm = resource->power > resource->cap at the end of
get_power_alarm_state() re-latch the polled comparison result into the next
power1_alarm read?

On a first read via power_meter_read() -> get_power_alarm_state() where
resource->power > resource->cap, resource->power_alarm is set back to true
after having been cleared at the start of the function.

On the next read of power1_alarm (with no intervening METER_NOTIFY_CAPPING
ACPI notification):

	if (resource->caps.flags & POWER_METER_CAN_NOTIFY)
		alarm = resource->power_alarm;
	resource->power_alarm = false;

alarm becomes true from that prior software comparison. If _PMM now returns
UNKNOWN_POWER_32 or UNKNOWN_POWER_64, or if update_meter() fails, the early
return:

	if (ret ||
	    resource->power == UNKNOWN_POWER_32 ||
	    resource->power == UNKNOWN_POWER_64)
		return alarm;

returns true even though no hardware capping notification occurred since the
previous read. Likewise, if resource->power has since dropped to or below
resource->cap, get_power_alarm_state() still returns true for that second
read.

When POWER_METER_CAN_NOTIFY is not set, writing resource->power_alarm at the
end of get_power_alarm_state() has no effect because the next call
unconditionally resets resource->power_alarm = false. Should the end of
get_power_alarm_state() simply return alarm || resource->power > resource->cap
without writing resource->power_alarm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031165447.1771473-1-linux@roeck-us.net?part=2