corsair-hydro-platinum: Add hwmon driver for Corsair H150i Elite RGB and other Hydro AIOs#78
Open
ProjectSynchro wants to merge 46 commits intoliquidctl:masterfrom
Open
corsair-hydro-platinum: Add hwmon driver for Corsair H150i Elite RGB and other Hydro AIOs#78ProjectSynchro wants to merge 46 commits intoliquidctl:masterfrom
ProjectSynchro wants to merge 46 commits intoliquidctl:masterfrom
Conversation
Contributor
Author
|
Marked as draft just in case this somehow bricks those untested devices. |
ProjectSynchro
changed the title
ProjectSynchro
force-pushed
the
h150i_support
branch
from
a1a77d0 to
ff70c8d
Compare
Contributor
Author
|
Should pass CI now 😄 |
Contributor
Author
|
I think I have solved everything that was up (may still fail builds for some reason) Since CI tests with kernels with CRC8 disabled, I added a fallback implementation that should allow the driver to still function and build for kernels without it. I also added documentation and a file in debugfs that returns the firmware version, can add more / remove some if wanted. Going to open this for review, this shouldn't brick untested devices, worst case they will need a reset. |
Contributor
Author
|
Will rebase on the master branch when #80 is merged, that fixes the CI issues for the NZXT drivers which should clear up CI here. |
Adds a new hwmon driver `corsair-hydro-platinum` to support Corsair AIO coolers using the "Hydro Platinum" protocol (e.g., H100i/H150i Elite RGB). This device uses a distinct protocol from the "Commander Core" based devices, communicating via 64-byte HID reports. Technical Implementation Details: 1. Write Operations (Control Transfer): Standard `hid_hw_output_report` fails with -38 (ENOSYS) as the device lacks an Interrupt OUT endpoint. Commands are instead sent via `hid_hw_raw_request` using `HID_REQ_SET_REPORT` (Control Transfer) over Endpoint 0. 2. Packet Structure: To avoid -32 (EPIPE) stalls during Control Transfers, the firmware requires a 65-byte padded buffer structure: [0x00] (Report ID padding) + [0x3F] (Command Prefix) + Payload. 3. Asynchronous Reporting: Status reports are received asynchronously via standard HID Input Reports (raw_event). `hid_device_io_start()` is explicitly called in probe to ensure these reports are delivered to the driver when using `HID_CONNECT_HIDRAW`. 4. Protocol Split (Fan 3 Quirk): The main cooling command (Feature 0x00) only supports 2 fans + Pump. For 360mm models (H150i/Elite), a second command (Feature 0x03) must be sent to control the 3rd fan. Command ordering is critical: Feature 0x00 MUST be sent before Feature 0x03 to avoid device stalls. Features: - Monitoring: - Liquid Temperature. - Pump Speed and Duty Cycle. - Fan Speeds and Duty Cycles (up to 3 fans). - Control (PWM): - Pump Mode Control (Quiet/Balanced/Extreme) via pwm1. - Fan Speed Control (0-100% Duty Cycle) via pwm[2-4]. - Initialization logic defaults fans to 50% to prevent startup noise. - Attempted Robustness: - CRC-8 verification on all received reports. - Synchronous transaction logic (Write + Wait for Report) to ensure data validity. - Exposes human-readable model name (e.g., "Corsair iCUE H150i Elite RGB") via standard `label` sysfs attribute. Signed-off-by: Jack Greiner <jack@emoss.org>
Also make sure we pull completion.h explicitly just in case. Signed-off-by: Jack Greiner <jack@emoss.org>
The driver previously used a single shared buffer for both transmitting commands and receiving responses. This may have caused race conditions when the driver was accessed concurrently, or when userspace tools (like OpenRGB or liquidctl) were communicating with the device at the same time as the kernel driver was. This is easily replicated by using the "Effects" plugin for OpenRGB It seems like I didn't quite implement the CRC validation correctly, it will now properly detect and ignore responses not intended for the driver. Additionally rate-limited logging will report CRC check failures instead. This allows the kernel driver to coexist and remain somewhat resiliant when userspace tools are also using the device. Signed-off-by: Jack Greiner <jack@emoss.org>
Previously this was wishy washy and only really done for FEATURE_COOLING_FAN3 commands (I was having particular issues there). Now it is more consistant and covers all fans + pump commands. Signed-off-by: Jack Greiner <jack@emoss.org>
So we can support older kernel versions we need to use dev_warn_ratelimited and dev_err_ratelimited as it turns out hid_err_ratelimited and hid_warn_ratelimited are newer helper macros. Signed-off-by: Jack Greiner <jack@emoss.org>
Signed-off-by: Jack Greiner <jack@emoss.org>
…RC8 is disabled Signed-off-by: Jack Greiner <jack@emoss.org>
Signed-off-by: Jack Greiner <jack@emoss.org>
…version" Signed-off-by: Jack Greiner <jack@emoss.org>
This should be inline with what the other drivers in this repo are doing for documentation. Signed-off-by: Jack Greiner <jack@emoss.org>
Signed-off-by: Jack Greiner <jack@emoss.org>
…CONFIG_CRC8 is disabled" This reverts commit 3f9dd62. Signed-off-by: Jack Greiner <jack@emoss.org>
ProjectSynchro
force-pushed
the
h150i_support
branch
from
b98c054 to
b47509c
Compare
Contributor
Author
|
Rebased from master, should pass checks now. |
ProjectSynchro
force-pushed
the
h150i_support
branch
3 times, most recently
from
856718c to
cca01f5
Compare
The `corsair-hydro-platinum` driver relies on the kernel's `crc8` library. In recent mainline kernels, `CONFIG_CRC8` was converted to a hidden symbol (prompt removed), which causes `make allnoconfig` to silently ignore `CONFIG_CRC8=y` in all.config This resulted in `modpost` failures due to undefined `crc8` symbols. See: torvalds/linux@aa09b32 Hopefully fix this by: 1. Explicitly enabling `CONFIG_CRC8=y` in all.config 2. Patching the kernel's Kconfig in build.yml to restore the "CRC8" prompt. This makes the symbol visible and selectable again during configuration, bypassing the limitation of `allnoconfig` with hidden symbols. This ensures the driver builds correctly on both stable (6.8) and mainline configurations. Signed-off-by: Jack Greiner <jack@emoss.org>
ProjectSynchro
force-pushed
the
h150i_support
branch
from
cca01f5 to
1b28bd5
Compare
Contributor
Author
|
Finally :) Seems like CONFIG_CRC8 has been removed recently for the mainline kernel (as of torvalds/linux@aa09b32) So with a bit of patching we can re-enable this option so the module builds correctly. |
Member
|
Thanks! I'll try to take a look at it next week. (Please ping me if you don't hear back from me). |
Member
jonasmalacofilho
left a comment
jonasmalacofilho
left a comment
There was a problem hiding this comment.
Note: I've tried to be as rigorous as I could, even with minor issues, to prepare this PR for you to mainline.
…it response The comment about waiting for the init response is both misplaced (appears after the call) and redundant, since hydro_platinum_write_cooling already handles the response wait internally via hydro_platinum_transaction. Signed-off-by: Jack Greiner <jack@emoss.org>
Previously, initialization and initial status update failures were logged as warnings but probe continued. This could leave the device in an undefined state. Fail probe entirely if either step fails. Signed-off-by: Jack Greiner <jack@emoss.org>
After a USB reset or resume from suspend, the device needs to be re-initialized with cooling settings. Add a reset_resume callback that re-sends the current cooling configuration, following the pattern used by other drivers in this repository. Signed-off-by: Jack Greiner <jack@emoss.org>
Add mutex_destroy calls in the probe error path (fail_and_stop) and in the remove function to properly clean up the mutex. Signed-off-by: Jack Greiner <jack@emoss.org>
Add spacing before the '#' comment on the corsair-hydro-platinum insmod line to better distinguish the command from the comment. Signed-off-by: Jack Greiner <jack@emoss.org>
Return -ENODATA in the read path when the last successful update is older than STATUS_VALIDITY, aligning with the staleness checks used by nzxt-kraken2, nzxt-kraken3, and nzxt-grid3. Signed-off-by: Jack Greiner <jack@emoss.org>
Increase the staleness threshold from 1000ms to 2000ms, giving headroom for one missed update before data is considered stale. This aligns with nzxt-kraken3 which uses the same 2000ms threshold. Signed-off-by: Jack Greiner <jack@emoss.org>
Replace module_hid_driver() with explicit __init/__exit functions and late_initcall(). This ensures the HID bus is fully initialized before the driver registers, avoiding probe ordering issues when compiled into the kernel. Aligns with the pattern used by all other drivers in this repository. Signed-off-by: Jack Greiner <jack@emoss.org>
Change the firmware version message from hid_info to hid_dbg so probe is silent on success. The firmware version remains available via debugfs and when dynamic debug is enabled. Aligns with the quieter probe style used by the other drivers in this repository. Signed-off-by: Jack Greiner <jack@emoss.org>
Replace dev_err_ratelimited and dev_warn_ratelimited calls with hid_err and hid_warn to match the logging convention used by the other drivers in this repository. The hid_* helpers automatically include the driver name in the output. Signed-off-by: Jack Greiner <jack@emoss.org>
Previously, raw_event would complete on any input report and rely on the CRC check in transaction to detect collisions. This meant that when userspace tools (liquidctl, OpenRGB) were using the device concurrently, we would frequently pick up their response packets, fail CRC or return wrong data, and waste our one chance to receive the correct response. Filter incoming reports in raw_event by comparing the response sequence+feature byte (data[1]) against the expected value from our last command. Responses that do not match are silently ignored, allowing the completion to wait for the correct response or time out gracefully. This makes concurrent userspace access significantly more robust, as transient collisions no longer immediately fail the transaction. Signed-off-by: Jack Greiner <jack@emoss.org>
…nsactions Add a second layer of response validation in hydro_platinum_transaction that checks the sequence+feature byte after CRC verification. This catches edge cases where raw_event sequence filtering is insufficient, such as when userspace tools reuse the same sequence number. Wrap the wait+validate logic in a retry loop (up to 3 attempts) so that a single invalid packet does not immediately fail the transaction. On CRC or sequence mismatch, reinitialize the completion and wait for the next report. Timeouts and signal interruptions are not retried. Signed-off-by: Jack Greiner <jack@emoss.org>
Move first argument of wait_for_completion_interruptible_timeout
onto the same line to avoid ending a line with '('.
Signed-off-by: Jack Greiner <jack@emoss.org>
… transaction raw_event runs in interrupt context and writes to rx_buffer without synchronization. A late response from a timed-out transaction, an unsolicited firmware report, or a HIDRAW request from userspace could overwrite rx_buffer while the transaction code is reading it. Add a spinlock (rx_lock) to protect rx_buffer writes in raw_event and reads in hydro_platinum_transaction. The transaction code copies rx_buffer to a local stack buffer under the lock, validates the copy, then writes it back only on success. Signed-off-by: Jack Greiner <jack@emoss.org>
…ite paths Use mutex_lock_interruptible instead of mutex_lock so that userspace can abort a request (e.g. via signal) if it is blocked waiting for the lock, rather than hanging indefinitely. Signed-off-by: Jack Greiner <jack@emoss.org>
…on pattern Instead of using a separate 'valid' boolean to track whether the first status report has been received, initialize priv->updated to STATUS_VALIDITY in the past. This makes the time_after check in the update function naturally trigger the first update without needing a special case. This follows the same pattern used by nzxt-kraken2, nzxt-kraken3, and nzxt-grid3. Signed-off-by: Jack Greiner <jack@emoss.org>
…from scratch Extract the common cooling payload prefix setup into hydro_platinum_init_cooling_payload() and use it for both the main and secondary (Fan 3) commands. The secondary command payload is now built from scratch rather than copying the main payload and overriding fields. This makes it clear exactly which fields are set in each command without needing to mentally track what survived the copy. Signed-off-by: Jack Greiner <jack@emoss.org>
…SE_LENGTH
Remove the redundant section header comments ('USB Vendor/Product IDs',
'Constants') and the unused RESPONSE_LENGTH define.
Signed-off-by: Jack Greiner <jack@emoss.org>
Add an inline comment to the sequence field explaining it is a protocol sequence number that cycles through 1-31. Signed-off-by: Jack Greiner <jack@emoss.org>
Replace the verbose explanation of the 65-byte buffer layout with a concise comment matching the usbhid convention: byte 0 is the report number, report data starts at byte 1. Signed-off-by: Jack Greiner <jack@emoss.org>
The old comment described the CRC range with confusing arithmetic that did not match the code. Replace with a clear description of the CRC-8 calculation: covers buf[2] through buf[REPORT_LENGTH-1], result stored in buf[REPORT_LENGTH]. Signed-off-by: Jack Greiner <jack@emoss.org>
Remove comments that restate what the code already says: 'Send Report', 'Use HID_REQ_SET_REPORT', 'Report ID', and 'raw_request returns number of bytes written on success'. The function names and parameters are self-documenting. Signed-off-by: Jack Greiner <jack@emoss.org>
Align the second argument of wait_for_completion_interruptible_timeout to match the open parenthesis. Signed-off-by: Jack Greiner <jack@emoss.org>
Replace the vague comment about driver buffer layout expectations with a concise description of what the code actually does: clamp the incoming report payload to the rx_buffer size. Signed-off-by: Jack Greiner <jack@emoss.org>
…load The max data length calculation used REPORT_LENGTH - start_at - 1, which is 59 bytes. The correct limit is REPORT_LENGTH - start_at (60 bytes) -- data occupies bytes 4 through 63, with byte 64 reserved for CRC. The old calculation left one payload byte unused. Signed-off-by: Jack Greiner <jack@emoss.org>
…sing Replace the manually unrolled fan parsing with a loop over an offset table. The fan data offsets (14, 21, 42) are irregular so a simple pattern isn't possible, but a lookup table with a loop is cleaner than three near-identical if blocks. Signed-off-by: Jack Greiner <jack@emoss.org>
… allocations The tx and rx buffers were allocated with 16 bytes of extra padding that was never intended to be accessed. This masks bugs rather than preventing them. Size the buffers to exactly what the protocol requires: - tx_buffer: REPORT_LENGTH + 1 (report ID byte + 64-byte payload) - rx_buffer: REPORT_LENGTH (64-byte input report) Also update the raw_event size clamp and the transaction local copy to match. Signed-off-by: Jack Greiner <jack@emoss.org>
…r build The TRANSACTION_RETRIES define was placed between the kerneldoc comment and hydro_platinum_transaction(), causing the doc parser to associate the comment with the macro instead of the function. With -Werror this becomes a build failure. Move the define to the constants section at the top of the file. Signed-off-by: Jack Greiner <jack@emoss.org>
Contributor
Author
|
I think I've actioned all the comments left, things should be a fair bit more robust as a result. CI should also pass after it runs for 0e07e6a Let me know if there is anything else I should work on! (Or if anything else is needed to at least get this driver merged here) |
Testing on hardware revealed that the device uses its own independent sequence counter in responses rather than echoing the sequence number from the command. This caused raw_event to drop every valid response (sequence mismatch), resulting in transaction timeouts during probe. Remove sequence filtering from raw_event and sequence validation from the transaction retry loop. CRC verification is now the sole response validation mechanism, matching what liquidctl does. The retry loop and spinlock remain to handle concurrent userspace access gracefully. Signed-off-by: Jack Greiner <jack@emoss.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See the commit message for specifics on the implementation (I fed my really bad notes into an LLM to clean them up so they are more readable)
This started off as a hacky project so I apologise for this being a single dump.
This is pretty much based on the implementation in liquidctl, I made sure that the driver still allows for userspace tools to interact with the device (e.g. OpenRGB control) and the CRC checking mechanism is also mirrored here which should verify responses (tried to mirror what liquidctl does)
Ideally I would have this driver not set anything in order to "wake" the device but that seems to be required for the device to react to commands. I decided to set this as a "safe" and relatively quiet 50% duty cycle on fans and the "balanced" preset for the pump. (Feel free to let me know if there's a better way to handle this)
I also tried my best to make the coolers appear with pretty names by setting a
model_name, not all software seems to respect or show model information.Let me know if the implementation can be improved any, it should work with all devices listed in the foreword since it mirrors the liquidctl implementation where possible.
Anyone with:
Nothing should break but some testing would be nice.. I don't know if bricking is possible, so test at your own risk (I can disable the untested devices if wanted)
If the device decides to stall, power cycling the AIO seems to reset it's state.