search

liquidctl / liquidtux

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

Add nzxt-kraken3 patch to patches/ #43

Closed aleksamagicka closed 5 months ago

aleksamagicka commented 1 year ago

As requested in #21, adding the patch file to patches/ (currently v1, not yet sent anywhere).

amezin commented 1 year ago

@jonasmalacofilho @stalkerg I propose continuing the discussion from https://github.com/liquidctl/liquidtux/issues/21 here - now all usual code review tools are available

aleksamagicka commented 1 year ago

I'll create a new PR for any fixes I come up with, it's probably easier to review there and look at the final patch here once we decide it's good to go.

On Sat, Jan 21, 2023, 3:38 PM Aleksandr Mezin @.***> wrote:

@jonasmalacofilho https://github.com/jonasmalacofilho @stalkerg https://github.com/stalkerg I propose continuing the discussion from #21 https://github.com/liquidctl/liquidtux/issues/21 here - now all usual code review tools are available

— Reply to this email directly, view it on GitHub https://github.com/liquidctl/liquidtux/pull/43#issuecomment-1399263398, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQGRIL65XDGCGL7SIJVFJDWTPYHNANCNFSM6AAAAAAUCHT34A . You are receiving this because you authored the thread.Message ID: @.***>

stalkerg commented 11 months ago

For me, it looks ok after #56 it can be polished a little bit more but I suppose it will be anyway during the mail list discussion.

aleksamagicka commented 7 months ago

Here is the v1 for LKML as a commit and as a patch file: v1_nzxt-kraken3.txt after #56.

Tested on both X53 and Z53 and works fine on my end.

@jonasmalacofilho and @stalkerg, I've included your tags, please have a look.

@amezin, do you want to be credited as well?

stalkerg commented 7 months ago

@aleksamagicka thanks, LGTM.

jonasmalacofilho commented 7 months ago

LGTM as well, thanks!

aleksamagicka commented 7 months ago

I still notice some issues, but everybody is likely already too tired of this :) So feel free to ignore. At least I can't say "it's incorrect/broken" now.

Awesome! Can be improved >>> not broken :)

Perhaps we can revisit the stuff you mentioned for eventual v2 (although they are small, thankfully).

As far as I understand, "Reviewed-By:" won't be a correct tag here, so I'm not sure how you can credit me in the patch.

Why wouldn't it be? I've seen that before on new patches (for example someone from Nvidia signed off and someone else, also from Nvidia, reviewed internally).

A Reviewed-by tag is a statement of opinion that the patch is an appropriate modification of the kernel without any remaining serious technical issues. Any interested reviewer (who has done the work) can offer a Reviewed-by tag for a patch. This tag serves to give credit to reviewers and to inform maintainers of the degree of review which has been done on the patch.

amezin commented 7 months ago

A Reviewed-by tag is a statement of opinion that the patch is an appropriate modification of the kernel without any remaining serious technical issues.

Well, I'm not so sure about that. Before submitting the patch, I'd definitely remove the unnecessary buffer_lock and fix the comments - because I'd expect to receive complaints from Guenter about "too complex synchronization". Good comments and smaller number of synchronization primitives reduce chances of that.

I'm sorry for holding you back by being slow to respond. This week I'm on a bug review/triage duty on my primary job, and in the remaining time I'm moving to another apartment, in a foreign country. So there's not much time left for GitHub activities :)

jonasmalacofilho commented 7 months ago

I'm not sure I see removing buffer_lock and allocating a new buffer every time it's needed as necessarily better (or worse).

On one hand, that specific synchronization isn't that tricky. On the other, the buffers are small and should be fairly cheap to allocate, and it can be argued that the expected frequency of their allocation is low to zero. At the same time, keeping a persistent buffer [of that size], while not useful for all users and use cases, is also cheap.

aleksamagicka commented 7 months ago

I'd definitely remove the unnecessary buffer_lock and fix the comments - because I'd expect to receive complaints from Guenter about "too complex synchronization". Good comments and smaller number of synchronization primitives reduce chances of that.

Yeah, but we've pretty much had a test run with gigabyte_waterforce (which is small enough to be boilerplate), which resolved the qualms he had with this approach (not many IIRC). And he didn't say anything about buffer_lock (which the waterforce has), and TBH I'm a bit hesitant to just remove it as I'm not sure how the requests are serialized at USB level (what I want to say is, what happens if multiple hid_hw_output_report fire in parallel).

I'm sorry for holding you back by being slow to respond. This week I'm on a bug review/triage duty on my primary job, and in the remaining time I'm moving to another apartment, in a foreign country. So there's not much time left for GitHub activities :)

Heh, I remember not touching this for months at a time last year because of college, no need to apologize. That time is long gone now, thankfully.

amezin commented 7 months ago

I'm just sticking to a trivial, obvious metric - the number of synchronization primitives.

and TBH I'm a bit hesitant to just remove it as I'm not sure how the requests are serialized at USB level (what I want to say is, what happens if multiple hid_hw_output_report fire in parallel).

It should be safe to do, both in hid and usb layers. At least I can't find a reason why it won't. I've been doing it in nzxt-smart2, until I found out that I need a mutex to make sure sending the message and committing the result into the driver state won't be interleaved with similar concurrent requests.

BTW, if you insist on using the mutex - you could merge the buffer into driver data struct - again, like in nzxt-smart2. To get rid of the devm_kzalloc.

stalkerg commented 7 months ago

I don't think sending the patch in its current shape will be a problem. Updates from @amezin can be in the second version or as a separate patch. I just worry that we miss something more important than an unnecessary lock; this is why I suppose showing a patch to LKML ASAP would be better. But it's up to you.

aleksamagicka commented 7 months ago

BTW, if you insist on using the mutex - you could merge the buffer into driver data struct - again, like in nzxt-smart2. To get rid of the devm_kzalloc.

That seems like a sidegrade at best, so I don't see a point. No one has ever complained about devm_kzalloc().

I don't think sending the patch in its current shape will be a problem. Updates from @amezin can be in the second version or as a separate patch. I just worry that we miss something more important than an unnecessary lock; this is why I suppose showing a patch to LKML ASAP would be better. But it's up to you.

As I said, I agree with this. We've already fixed the outstanding issues (and I've brought the code in line to what Guenter has previously accepted, as much as possible). I'm pretty sure there's going to be a v2 down the line, and then I can make that one comment lenghtier, as I'd rather not force Jonas and Yury to look over the patch again.

So, the next step is to send this to LKML. Do I have your Reviewed-by for it in the current state, @amezin? (buffer_lock and devm_kzalloc() are already commonplace in hwmon.)

amezin commented 7 months ago

Please don't add me as Reviewed-by. TBH I don't understand what all these tags exactly mean, and currently have no time to figure that out. But I'm sure adding my Reviewed-by has no benefit for the patch, i.e. it won't be accepted faster/easier because of that.

Other than that - I've already approved the pull request.

aleksamagicka commented 7 months ago

Thanks. Just sent v1 to LKML.

aleksamagicka commented 7 months ago

Well, everyone... this just got merged without any comments.

Thank you all for working on this driver with me, and especially to Jonas for starting it in the first place - I sure learned a lot during this journey. Next stop, adding support for 2023 Krakens.

amezin commented 7 months ago

Maybe merge the PR then? It likely doesn't really matter what happens with patches at this point, but "merged" status seems to better match the reality than just "closed".

jonasmalacofilho commented 7 months ago

@aleksamagicka, you did most of the work, so thank you! And thanks everyone that collaborated too.

aleksamagicka commented 7 months ago

Maybe merge the PR then? It likely doesn't really matter what happens with patches at this point, but "merged" status seems to better match the reality than just "closed".

Does it make sense to merge at all? The patch file can now be extracted from the upstream commit anytime and it's immutable now. I know that the initial idea was to store it in repo, but iterating by providing a link to a real Linux repo with the actual commit is better, IMO, and allows me/whoever to force push without notifications, have a Github UI for it, etc.

My line of thought is if I merge this, I don't see how it's useful to anyone in this form? As for github status, it would be best to close it as completed, but this is not an Issue so it's not possible. Anyway, if someone's looking, I also have to update the README which I'll do in a next PR.

Thoughts?

aleksamagicka commented 5 months ago

Closing as the patch can always be retrieved from the main repos.