nzxt-kraken3: Improve things noticed in #43, always return -ENODATA if device is faulty

aleksamagicka commented 11 months ago

This PR:

improves the things @amezin commented on in #43
adds a few comments to those parts
formats the kraken3_raw_event() signature to be on one line
improves the faultiness logic to return -ENODATA if the device reports it's faulty, for a cycle. Otherwise, stale data would be returned
notes the faultiness reporting in the docs
improves lots of things that Guenter complained about in the gigabyte_waterforce driver, that are directly applicable to this one

aleksamagicka commented 11 months ago

Pushed what I had in mind regarding the spinlocks, please have a look. The calls could be more specific AFAICS (raw event is a softirq, so _bh should suffice), but I went with _irqsave as a starting point.

Edit on 28 Nov: AFAICS, only one softirq can run at a time, so if a spin lock is needed, a pure one is enough. We'll see.

aleksamagicka commented 9 months ago

Status update: the gigabyte_waterforce submission for mainline will help cut down on comments immensely for this driver. Also, Guenter hasn't complained about the _bh spinlock calls in the mutex for getting status there, so I hope that we finally have that one locked down for kraken3 as well.

aleksamagicka commented 9 months ago

This should now be rebased correctly. I've also incorporated lots of Guenter's feedback on the Waterforce driver here as well.

The faultiness handling irks me somewhat, I'll have to look into making it as resettable as possible.

aleksamagicka commented 9 months ago

Fixed the faultiness logic so that it resets upon receiving a normal report and returns -ENODATA for a faulty cycle. I think the PR looks good as a whole (famous last words, though). Please review!

aleksamagicka commented 7 months ago

I'd like to merge this PR this month, if anyone has any comments please say. (The merge window is delayed right now so there's a bit more time.)

jonasmalacofilho commented 7 months ago

Realistically, I won't be able to do an in-depth review of this any time soon. But I have looked it over.

Overall, LGTM (and, as far as I'm concerned, you can proceed and merge this as is). But I have not checked the kernel concurrency primitives being used for corner cases I might have forgotten (or that are otherwise not obvious).

Just one question: how were you able to determinate that the HID raw event handler will run in soft IRQ?

aleksamagicka commented 7 months ago

Realistically, I won't be able to do an in-depth review of this any time soon. But I have looked it over.

Overall, LGTM (and, as far as I'm concerned, you can proceed and merge this as is). But I have not checked the kernel concurrency primitives being used for corner cases I might have forgotten (or that are otherwise not obvious).

Thanks. The concurrency code is now very similar to what is in the Gigabyte driver. Guenter will complain if he sees something anyway.

Just one question: how were you able to determinate that the HID raw event handler will run in soft IRQ?

Using ftrace, seems to be softirq:

# tracer: function
#
# entries-in-buffer/entries-written: 4/4   #P:32
#
#                                _-----=> irqs-off/BH-disabled
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| / _-=> migrate-disable
#                              |||| /     delay
#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
          <idle>-0       [028] ..s2.    75.127663: aqc_raw_event <-hid_input_report
          <idle>-0       [028] ..s2.    75.143658: aqc_raw_event <-hid_input_report
          <idle>-0       [028] ..s2.    76.135680: aqc_raw_event <-hid_input_report
          <idle>-0       [028] ..s2.    76.143685: aqc_raw_event <-hid_input_report

And Guenter didn't say otherwise for one of the commits :)

aleksamagicka commented 7 months ago

@jonasmalacofilho, what was the concensus regarding merging - do we just rebase or do a merge commit?

jonasmalacofilho commented 7 months ago

Well, now that you mention in, I guess consensus has not been explicitly reached.

Initially it seemed like we had settled on:

So let's disable all but normal merge commits, like you suggested. —me, replying to amezin, https://github.com/liquidctl/liquidtux/issues/31#issuecomment-1376667065, 2023-01-10

But afterwards rebase came back:

Also, is anything wrong with "Rebase and merge" method? It keeps the original commit message —amezin, https://github.com/liquidctl/liquidtux/pull/39#issuecomment-1399253107, 2023-01-21

After the latter exchange I re-enabled "Rebase and merge", and we've been using it since without issues, as far as I'm aware.

So think you can use it too, assuming you agree that a linear history would be preferable in this case. (And I don't think it's necessary to ping amezin to re-hash this).

aleksamagicka commented 7 months ago

So let's disable all but normal merge commits, like you suggested.

Yes, that part confused me initially.

I think that a rebase is OK here, thanks!

liquidctl / liquidtux

nzxt-kraken3: Improve things noticed in #43, always return -ENODATA if device is faulty #56