Re: [PATCH 7/7] hwmon: (marvel-env) Notify userspace of environmental events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] hwmon: marvel-env: incomplete alarm notifications in
  marvel_env_event_work()
- [Low] hwmon: marvel-env: private mutex used instead of hwmon_lock()
--

commit d84479ee83faacee9e88f233e9993cccd529e1af
Author: Matt Turner <mattst88@gmail.com>

hwmon: (marvel-env) Notify userspace of environmental events

This commit registers a machine check notifier to refresh all sensor nodes
from a workqueue and notify userspace of alarm status changes via
hwmon_notify_event().

> diff --git a/drivers/hwmon/marvel-env.c b/drivers/hwmon/marvel-env.c
> index cdc7e059e17bb..3dd2ea658fa3b 100644
> --- a/drivers/hwmon/marvel-env.c
> +++ b/drivers/hwmon/marvel-env.c

[ ... ]

> @@ -460,6 +469,72 @@ static const struct {
>  	{ hwmon_power, HWMON_P_ALARM | HWMON_P_LABEL },
>  };
>  
> +static const u32 marvel_env_alarm[hwmon_max] = {
> +	[hwmon_temp] = hwmon_temp_max_alarm,
> +	[hwmon_fan] = hwmon_fan_min_alarm,
> +	[hwmon_in] = hwmon_in_alarm,
> +	[hwmon_power] = hwmon_power_alarm,
> +};
> +
> +/* Reread everything after a system event and report changed alarms. */
> +static void marvel_env_event_work(struct work_struct *work)
> +{
> +	struct marvel_env_data *data =
> +		container_of(work, struct marvel_env_data, work);
> +	unsigned int i, k;
> +
> +	mutex_lock(&data->lock);
> +	for (i = 0; i < data->nr_nodes; i++) {
> +		data->nodes[i].valid = false;
> +		marvel_env_refresh(data, &data->nodes[i]);
> +	}
> +
> +	for (k = 0; k < ARRAY_SIZE(marvel_env_hwmon); k++) {
> +		enum hwmon_sensor_types type = marvel_env_hwmon[k].type;
> +
> +		for (i = 0; i < data->nr_chans[type]; i++) {
> +			struct marvel_env_chan *c = &data->chans[type][i];
> +			u8 status = env_status(c->node,
> +					       c->pkt + env_status_off(type));
> +
> +			if (status == c->last_status)
> +				continue;
> +			c->last_status = status;
> +			hwmon_notify_event(data->hwmon, type,
> +					   marvel_env_alarm[type], i);

[Severity: Medium]
Does marvel_env_event_work() miss notifications for hwmon_temp_crit_alarm
and hwmon_fan_fault while emitting notifications for attributes that did
not change?

In marvel_env_read(), temperature channels expose both
hwmon_temp_max_alarm (via env_status_alarm(), which is true for
ENV_WARNING and ENV_FAULT) and hwmon_temp_crit_alarm (true for ENV_FAULT),
and fan channels expose both hwmon_fan_min_alarm and hwmon_fan_fault:

drivers/hwmon/marvel-env.c:marvel_env_read() {
    ...
		case hwmon_temp_max_alarm:
			*val = env_status_alarm(status);
			break;
		case hwmon_temp_crit_alarm:
			*val = status == ENV_FAULT;
			break;
    ...
		case hwmon_fan_min_alarm:
			*val = env_status_alarm(status);
			break;
		case hwmon_fan_fault:
			*val = status == ENV_FAULT;
			break;
    ...
}

Because marvel_env_alarm[] only maps each sensor type to a single
attribute, a transition into or out of ENV_FAULT never calls
hwmon_notify_event() for hwmon_temp_crit_alarm or hwmon_fan_fault, so
userspace polling tempN_crit_alarm or fanN_fault is not woken.
Conversely, a transition between ENV_WARNING and ENV_FAULT leaves
env_status_alarm() at 1, yet notifies tempN_max_alarm, fanN_min_alarm,
inN_alarm, or powerN_alarm even though their values did not change.
Transitions involving ENV_UNKNOWN or ENV_NOT_PRESENT also trigger
notifications even when env_status_alarm() does not change or the channel
was hidden at probe time by marvel_env_is_visible().

> +		}
> +	}
> +	mutex_unlock(&data->lock);
> +}

[Severity: Low]
This isn't a bug, but the hwmon subsystem prefers hwmon_lock(data->hwmon)
and hwmon_unlock(data->hwmon) over a driver-private mutex when
synchronizing asynchronous work with hwmon callbacks, since the hwmon core
already holds the device lock across .read and .write ops.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-marvel-env-v1-0-890328e4b7e5@gmail.com?part=7