search

liquidctl / liquidtux

Linux kernel hwmon drivers for AIO liquid coolers and other devices
Other
80 stars 15 forks source link

nzxt-kraken3: Ensure that only one of the readers requests Z53 status #53

Closed aleksamagicka closed 1 year ago

aleksamagicka commented 1 year ago

In kraken3_read(), multiple readers could execute the same commands for Z53 in parallel, if it's time to refresh the sensor data.

This PR introduces a mutex (z53_status_request_lock) that's used to let a single reader send the command for requesting a status report, while others (if they exist), only wait until the report is parsed.

jonasmalacofilho commented 1 year ago

This is where not working in isolated patches makes sense.

We're clearly still figuring out what needs to be done, and we're in synchronization code, which requires keeing a more global model of the code.

So I propose we work on the entire kraken3_read/kraken3_raw_event pair in a single PR.

As for what's in this PR, I agree that we do need the mutex now, but I also think that reinit_completion needs to be moved into the critical section:

The re-initialization function, reinit_completion(), simply resets the ->done field to 0 (“not done”), without touching the waitqueue. Callers of this function must make sure that there are no racy wait_for_completion() calls going on in parallel. —https://www.kernel.org/doc/html/latest/scheduler/completion.html?highlight=racy

That said, I haven't looked into this PR in much detail yet, as it's hard to keep track of all possible states in two PRs + concurrency + an unsafe C API (where I keep discovering new preconditions, like the one above).

aleksamagicka commented 1 year ago

Ah, will move it into the earlier one. Thought that it would be better to separate.

jonasmalacofilho commented 1 year ago

Thanks!

(But if you @amezin disagree and actually prefer to always work in isolated patches, even in this sorts of cases, then please do it that way. I just might have a harder time keeping up with the changes).

aleksamagicka commented 1 year ago

I'm not tied into either way particularly, I'll follow whatever is easier for you and others to look at.