nzxt-kraken3: delay reading for X53 until first report is received; ensure proper concurrency for Z53

[aleksamagicka]

This should resolve the issue of fancontrol possibly racing with the first report on X-series devices and reading an -ENODATA. Please do roast this approach if something's not right. completion_done() will always return true after a complete_all() call, that's how we know that the first report has been parsed and to skip doing that again.

[aleksamagicka]

I have some changes regarding this, but how do I test the .reset_resume handler? .resume triggers on return from suspend. Surely I'm missing something obvious?

[amezin]

I have some changes regarding this, but how do I test the .reset_resume handler? .resume triggers on return from suspend. Surely I'm missing something obvious?

To trigger .reset_resume, I've unplugged and plugged in back the device while the machine was suspended. You need to fully cut power to the device.

[jonasmalacofilho]

To trigger .reset_resume, I've unplugged and plugged in back the device while the machine was suspended. You need to fully cut power to the device.

It's especially important to unplug the USB connection, something that might not be obvious: however, many of these devices (and all NZXT devices I've personally tested) power the controller from 5V-standby, which may be (but by default usually isn't) turned off during suspend of even soft-off.

[aleksamagicka]

Thank you both, that worked. jiffies doesn't change during suspend, so there's nothing to do there.

[aleksamagicka]

Just tested the following scenarios for the X53:

• Start fancontrol, suspend, resume - everything continues normally
• Start fancontrol, suspend, unplug device, resume - fancontrol crashes, device is not physically connected
• Start fancontrol, suspend, unplug, wait a bit, plug in again, resume - everything continues normally

Z53 also works.

[aleksamagicka]

Just pushed the Z53 fixes from the other PR here. I think I've resolved the issues/TODOs from there, but it needs to be looked over.

[stalkerg]

Is it ready to review?

[aleksamagicka]

Yes, should be - I didn't make any changes to this since the last commit, and it worked well during testing. Appreciate any comments or questions!

[jonasmalacofilho]

Hi Aleksa, sorry for the long, long, delay.

When I looked this over a couple of months ago, there were a few things I wasn't sure about (related to concurrency), but I haven't yet had the time to learn about the kernel concurrency model and APIs in the depth I need. And I don't feel qualified to comment here based on what I know so far.

So, I suggest that you post this on the hwmon mailing list, as is, maybe (but not necessarily) with an RFC tag. And it does look good to me, whatever that's worth.

Sorry again for the delay! It was a mix of repeatedly pushing this to the next week, when I though I would have the time, and forgetting about it for a while too.

[aleksamagicka]

Thanks! I was looking over this a few days ago and also wanted to finally move it out of status quo. The premise of this PR still seems OK to me, but of course Guenter will rip it apart in ways I never thought possible. Will work on posting this to the mailing list, and TBH I'm excited to get things moving, especially with the 2023 models at our heels.

[aleksamagicka]

I've pushed a simplified version of the same logic, the main reader requests the status and others just wait for a signal that it's finished. I think that the previous version had a race condition where the secondary readers would start waiting for a report, but the main reader could still be doing the preparatory work - that's gone now, and only the main reader controls when the secondary ones continue.

Edit: talking about the Z53, of course.

[aleksamagicka]

Pushed the wait_queue version. Very similar as it was before.

Side note: I used the interruptible variants of wait_event and wait_for_completion, will switch other calls in the driver to those in a next PR.

[aleksamagicka]

Thanks!!