Skip to content

Conversation

dripsnek
Copy link

@dripsnek dripsnek commented Mar 31, 2025

This is a hwmon driver I've been writing for the Razer Hanbo Chroma AIO cooler. Specifically the 360mm version, though if the 240mm uses the same protocol minus one fan then that should work as well.

The nzxt-kraken3 and gigabyte_waterforce drivers served as inspiration for this. I have been using this driver on my own Hanbo 360mm under Linux 6.13.7.

I believe the documentation pretty much sums up the exposed functionality though I have some queries.

  • Naming conventions - does anything need to change?
  • In addition to the fan curves I have made a custom attribute for PWM setpoints so that one could compare the target vs reported PWM. Both values come from HID reports and are reflective of the active profile. Is this an appropriate use of a custom attribute? (yes)
  • I can acquire the serial number for the AIO in firmware, is this worth exposing somewhere in the driver? (incorporated)
  • Are packet captures need to form part of the pull request?
  • Is there anything required liquidctl side to use this? (to be addressed later)
  • Is it worth while implementing some kind of online reset without reloading the module? (removed)

In terms of TODOs:

  • I'd like some packet dumps from models running older firmware to check compatibility and get a definitive format of the firmware version field. The current firmware version dissection is based off fuzzing the firmware updater.
  • There is a PWM scaling hack on the sysfs side to make lm-sensors show a value closer to what is reported over the wire. Not sure of the best way to tidy that up. For Kernel 6.14 it seems hwmon no longer needs this workaround. Removed.
  • Test for compatibility with userland RGB control software. OpenRGB support has been implemented along with a proof of concept Liquidctl patchset. Interop tests are ongoing. Currently no crash scenarios encountered, adverse effects typically result in timeouts reported to userspace. Sysfs values retain previous values in this circumstance.

Copy link
Member

@aleksamagicka aleksamagicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an initial review, thanks.

case FAN_CHANNEL:
switch (val) {
case 1:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify this by storing 0x14, 0x32 etc. from the switch in a variable and then just call

				ret = hanbo_hid_profile_send(priv, channel, val & 0xFF, magic_value);
					if (ret < 0)
						goto unlock_and_return;
					break;

at the end.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in 1159c87

@aleksamagicka
Copy link
Member

In addition to the fan curves I have made a custom attribute for PWM setpoints so that one could compare the target vs reported PWM. Both values come from HID reports and are reflective of the active profile. Is this an appropriate use of a custom attribute?

I think it is.

I can acquire the serial number for the AIO in firmware, is this worth exposing somewhere in the driver?

Yes, through debugfs, like fw version.

Is there anything required liquidctl side to use this?

You'll need to implement a 'driver' there as well.

Is it worth while implementing some kind of online reset without reloading the module?

Not sure what this means?

* Refactor hanbo_hid_validate_header to use less arguments.
* Refactor hanbo_hid_profile_send to use less arguments.
* Include device serial number in DebugFS.
* Removed power management placeholder code.
* 'ret' used as return from more functions.
* Replaced a number of values with appropriate error codes or constants.
* mutex_unlocks occur after spin_unlocks.
* Removed redundant goto statements in hanbo_hwmon_write.
* Minor formatting updates and changes to constants for clarity.

Signed-off-by: Joseph East <[email protected]>
@dripsnek
Copy link
Author

dripsnek commented Apr 1, 2025

Yes, through debugfs, like fw version.

Added in 1159c87

You'll need to implement a 'driver' there as well.

I'll have a poke around then.

Is it worth while implementing some kind of online reset without reloading the module?

Not sure what this means?

There was a power management placeholder there for HID reset/resume, I was tossing up whether to use it or not but it has been removed as of 1159c87.

Comment on lines 419 to 434
switch (val) {
case 1:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
case 2:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
case 3:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
case 4:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
default:
ret = -EINVAL;
}
Copy link
Member

@aleksamagicka aleksamagicka Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
switch (val) {
case 1:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
case 2:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
case 3:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
case 4:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;
default:
ret = -EINVAL;
}
if (val < 1 || val > 4) {
ret = -EINVAL;
goto unlock_and_return;
}
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF);
break;

or something like that would be better as to not repeat the same code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in 8a21201. I chose to sanitise the remaining input arguments in hanbo_hid_profile_send allowing for less switch statements.

dripsnek added 2 commits April 2, 2025 20:30
* Documentation updated (serial number & sysfs table)
* hanbo_hwmon_write simplification by arg checking in hanbo_profile_send
* All hwmon_ops return 0 on success as per hwmon.h definition
* CPU reference temperature only updated if HID transfer succeeds
* Driver will emit hid_fail and bail out if init sequence fails.

Signed-off-by: Joseph East <[email protected]>
Signed-off-by: Joseph East <[email protected]>
dripsnek added 2 commits April 2, 2025 20:59
* Include a profile_sticky property to prevent active_profile updates
  when curve mode is known to be running.
* Passively detect curve mode by processing its ack report.
* Remove stickiness when a profile ack report is processed.
* Remove scaling hack, appears to be no longer needed in Linux 6.14?
* Documentation updates

Signed-off-by: Joseph East <[email protected]>
Signed-off-by: Joseph East <[email protected]>
@dripsnek
Copy link
Author

Removing draft tag - I think this is as complete as it is going to get. Liquidctl implementation hasn't revealed any missing functions. Still looking for others running older firmware though.

@dripsnek dripsnek marked this pull request as ready for review April 23, 2025 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants