* [PATCH 2/2] hwmon: (gigabyte_waterforce) Protect status report data with spinlock
2026-10-31 22:31 [PATCH 1/2] hwmon: (gigabyte_waterforce) Make status_report_request_lock IRQ safe Guenter Roeck
@ 2026-10-31 22:31 ` Guenter Roeck
2026-10-31 22:35 ` sashiko-bot
2026-10-31 22:33 ` [PATCH 1/2] hwmon: (gigabyte_waterforce) Make status_report_request_lock IRQ safe sashiko-bot
1 sibling, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2026-10-31 22:31 UTC (permalink / raw)
To: Hardware Monitoring; +Cc: Guenter Roeck, Sashiko, Aleksa Savic
waterforce_raw_event() updates temp_input, speed_input, duty_input, and
the updated timestamp without holding priv->status_report_request_lock
(and updates priv->updated after complete_all()), while
waterforce_get_status() and waterforce_read() read those fields without
synchronization.
Because HID_CONNECT_HIDRAW is enabled, or when an interrupted wait leaves
a requested report in flight, waterforce_raw_event() can execute
concurrently with sysfs reads. Under the Linux kernel memory model this
constitutes a data race, lacks memory ordering between priv->updated and
the cached sensor fields on weakly ordered architectures, and allows a
reader woken by complete_all() to re-enter waterforce_get_status() before
priv->updated is refreshed.
Extend priv->status_report_request_lock to cover updating the sensor
values and priv->updated in waterforce_raw_event(), checking priv->updated
in waterforce_get_status(), and reading the cached sensor values in
waterforce_read().
Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 42ac68e3d4ba ("hwmon: Add driver for Gigabyte AORUS Waterforce AIO coolers")
Cc: Aleksa Savic <savicaleksa83@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/hwmon/gigabyte_waterforce.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
index 65169dbb91d8..34471852fe21 100644
--- a/drivers/hwmon/gigabyte_waterforce.c
+++ b/drivers/hwmon/gigabyte_waterforce.c
@@ -55,7 +55,7 @@ struct waterforce_data {
struct mutex buffer_lock;
/* For queueing multiple readers */
struct mutex status_report_request_mutex;
- /* For reinitializing the completion below */
+ /* Protects status report data, timestamp, and completion below */
spinlock_t status_report_request_lock;
struct completion status_report_received;
struct completion fw_version_processed;
@@ -128,8 +128,10 @@ static int waterforce_get_status(struct waterforce_data *priv)
if (ret < 0)
return ret;
+ spin_lock_irq(&priv->status_report_request_lock);
if (!time_after(jiffies, priv->updated + msecs_to_jiffies(STATUS_VALIDITY))) {
/* Data is up to date */
+ spin_unlock_irq(&priv->status_report_request_lock);
goto unlock_and_return;
}
@@ -139,7 +141,6 @@ static int waterforce_get_status(struct waterforce_data *priv)
* the raw event parsing and marked the priv->status_report_received
* completion as done.
*/
- spin_lock_irq(&priv->status_report_request_lock);
reinit_completion(&priv->status_report_received);
spin_unlock_irq(&priv->status_report_request_lock);
@@ -170,6 +171,8 @@ static int waterforce_read(struct device *dev, enum hwmon_sensor_types type,
if (ret < 0)
return ret;
+ ret = 0;
+ spin_lock_irq(&priv->status_report_request_lock);
switch (type) {
case hwmon_temp:
*val = priv->temp_input[channel];
@@ -183,14 +186,17 @@ static int waterforce_read(struct device *dev, enum hwmon_sensor_types type,
*val = DIV_ROUND_CLOSEST(priv->duty_input[channel] * 255, 100);
break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
break;
default:
- return -EOPNOTSUPP; /* unreachable */
+ ret = -EOPNOTSUPP; /* unreachable */
+ break;
}
+ spin_unlock_irq(&priv->status_report_request_lock);
- return 0;
+ return ret;
}
static int waterforce_read_string(struct device *dev, enum hwmon_sensor_types type,
@@ -276,19 +282,17 @@ static int waterforce_raw_event(struct hid_device *hdev, struct hid_report *repo
if (size <= WATERFORCE_TEMP_SENSOR)
return 0;
+ spin_lock_irqsave(&priv->status_report_request_lock, flags);
priv->temp_input[0] = data[WATERFORCE_TEMP_SENSOR] * 1000;
priv->speed_input[0] = get_unaligned_le16(data + WATERFORCE_FAN_SPEED);
priv->speed_input[1] = get_unaligned_le16(data + WATERFORCE_PUMP_SPEED);
priv->duty_input[0] = data[WATERFORCE_FAN_DUTY];
priv->duty_input[1] = data[WATERFORCE_PUMP_DUTY];
-
- spin_lock_irqsave(&priv->status_report_request_lock, flags);
+ priv->updated = jiffies;
if (!completion_done(&priv->status_report_received))
complete_all(&priv->status_report_received);
spin_unlock_irqrestore(&priv->status_report_request_lock, flags);
- priv->updated = jiffies;
-
return 0;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] hwmon: (gigabyte_waterforce) Make status_report_request_lock IRQ safe
2026-10-31 22:31 [PATCH 1/2] hwmon: (gigabyte_waterforce) Make status_report_request_lock IRQ safe Guenter Roeck
2026-10-31 22:31 ` [PATCH 2/2] hwmon: (gigabyte_waterforce) Protect status report data with spinlock Guenter Roeck
@ 2026-10-31 22:33 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-10-31 22:33 UTC (permalink / raw)
To: Guenter Roeck, Hardware Monitoring
> waterforce_raw_event() is the HID ->raw_event() callback and acquires
> priv->status_report_request_lock with a plain spin_lock(), while
> waterforce_get_status() acquires the same lock with spin_lock_bh().
>
> Depending on the HID transport and USB host controller driver,
> waterforce_raw_event() can run in process context (e.g. via uhid with
> softirqs and hardirqs enabled), softirq context (USB HCDs using HCD_BH),
> or hard interrupt context (USB HCDs without HCD_BH, such as OHCI/UHCI).
>
> Because spin_lock_bh() in waterforce_get_status() leaves hard interrupts
> enabled, a hard-IRQ URB completion invoking waterforce_raw_event() on the
> same CPU will self-deadlock on status_report_request_lock. Likewise, a
> plain spin_lock() in waterforce_raw_event() when called from process
> context via uhid leaves both softirqs and hardirqs enabled, risking a
> self-deadlock if interrupted by softirq or hardirq raw-event delivery on
> the same CPU.
>
> Use spin_lock_irq() in waterforce_get_status(), which always runs in
> process context, and spin_lock_irqsave() in waterforce_raw_event(), which
> may run with interrupts either enabled or disabled.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: 41c71105a845 ("hwmon: (gigabyte_waterforce) Mark status report as received under a spinlock")
> Cc: Aleksa Savic <savicaleksa83@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Sashiko has reviewed this patch and found no issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261031223131.1855967-1-linux@roeck-us.net?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread