Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grid v3 upstreaming #19

Open
amezin opened this issue Dec 28, 2021 · 4 comments
Open

Grid v3 upstreaming #19

amezin opened this issue Dec 28, 2021 · 4 comments

Comments

@amezin
Copy link
Member

amezin commented Dec 28, 2021

We have two almost complete drivers for grid v3, and none upstreamed. That's not good.

@jonasmalacofilho Could you explain what's missing/wrong with nzxt-grid3.c in this repository?

And I'll try to recall the state of my implementation...

@jonasmalacofilho
Copy link
Member

@jonasmalacofilho Could you explain what's missing/wrong with nzxt-grid3.c in this repository?

I'll look into it next week.

@jonasmalacofilho
Copy link
Member

Hey @amezin,

The short answer is: not much, at least for a v1 patch, but there are scenarios where pwmconfig or fancontrol will panic due to the additional assumptions those tools make.

In the past I tried adding more and more hacks to fix all instances where either tool would crash or complain, but it was never enough. For example, when resuming from a suspended state (or if the device simply hasn't sent any reports in a while), we may transiently return -ENODATA; that (IIRC) will cause a running fancontrol instance to abort.

Therefore my last significant commit (00bcb93) actually removed most of these hacks, only keeping the few needed for what I think are the reasoable user-space expectations (that, unfortunately, the device doesn't natively provide). And that brought the driver back to something I think Guenter may eventually (= after any necessary fixes) accept.

But, again, I don't recommend using fancontrol with it,12 and that's why I haven't submitted the driver yet.

To clarify: I don't think pwmconfig and fancontrol can't make additional assumptions, and I'm sure they make sense for most Super I/O drivers generally used with them. Usually I would simply take on the task of fixing those tools to work just as well with these new HID drivers, but the fact that both tools are bash scripts (which makes the more nuanced error handling that's required considerably annoying to write), and that the maintenance of the lm-sensors project has been spotty in last the years, has demotivated me to do so...

Footnotes

  1. Or with any driver/device that doesn't perfectly match fancontrol's assumptions.

  2. Personally I've only been using the driver for monitoring, and relying on liquidctl to set up static fan speeds during startup/resume; for dynamic fan control I would probably still use liquidctl (with the yoda script we ship with it), or try one of the modern alternatives to fancontrol.

@amezin
Copy link
Member Author

amezin commented Jan 23, 2022

The only discussion about Grid v3 on linux-hwmon that I found: https://www.spinics.net/lists/linux-hwmon/msg11045.html (please add other links if I missed something)

@amezin
Copy link
Member Author

amezin commented Jan 23, 2022

For example, when resuming from a suspended state (or if the device simply hasn't sent any reports in a while), we may transiently return -ENODATA; that (IIRC) will cause a running fancontrol instance to abort.

Yep, that's why in smart2 driver I block until the data is available (the locks/synchronization Guenter initially disliked).

And yes, fancontrol/pwmconfig are somewhat broken (for example, writability checks that don't work when these scripts are run as root - i. e. always), so maybe 100% compatibility isn't necessary (I don't use them too). However, if compatibility can be achieved with just a few tweaks, I think it's reasonable to do. And, for example, I think blocking until the data is available is a sane thing to do (regardless of fancontrol/pwmconfig expectations and behavior).

Even when not using fancontrol/pwmconfig, the ability to control fans by simple sysfs writes, without any additional software, is a good thing, I think.

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

No branches or pull requests

2 participants