search

Fred78290 / nct6687d

Linux kernel module for Nuvoton NCT6687-R
GNU General Public License v2.0
203 stars 40 forks source link

[RFC] Update cached pwm values on pwm store #91

Closed desowin closed 6 months ago

desowin commented 6 months ago

Userspace application fan2go expects written pwm value to be returned on read after 5 ms. If the value read back then fan2go assumes that the pwm is resolution limited and was clamped by the driver.

The nct6687d driver returns cached pwm values on reads. The cache is updated at most once per second (unless resumed from suspend) and therefore it leads for fan2go to falsely analyzing the pwm output as resolution limited.

Avoid the issue by waiting for pwm value to be reported back on each write and updating the cache immediately. On MSI PRO Z790-P WIFI (MS-7E06) it takes around 500 ms for the pwm value (with DC mode fan connected, so it is not a real PWM but a voltage) to be read back after write.

I am not sure where this issue should be fixed, I'd tend towards fixing it in fan2go. This PR is opened mostly to try to reach to a wider audience. The biggest takeaway from this was determining that it takes around 500 ms for the set value to update.

Another possibility would be to simply update cached value after write and not wait for it be read back. Unfortunately doing so would be prone to race conditions if the write happens when the time to next update is less than the "hardware update interval" (500 ms on my setup).

xaizek commented 6 months ago

I am not sure where this issue should be fixed, I'd tend towards fixing it in fan2go.

I also think that working around behaviour of a tool in the driver is not the best idea. Although, being a workaround, it could be enabled via module's parameter.

The biggest takeaway from this was determining that it takes around 500 ms for the set value to update.

Seems to take effect immediately for me with fans in PWM mode.

xaizek commented 6 months ago

Well, now, everyone will get 50ms delay even if the value is changed immediately (like in PWM mode).

desowin commented 6 months ago

@xaizek the 50 ms delay was there already.

SergeyMy commented 6 months ago

Well, now, everyone will get 50ms delay even if the value is changed immediately (like in PWM mode).

everyone will get 500ms latency if NCT6687 REG_PWM! measured value. Sadness. inside mutex_lock!!!

xaizek commented 6 months ago

everyone will get 500ms latency if NCT6687 REG_PWM! measured value. Sadness.

Those who have DC fans might (I haven't tested this change on PWM though). As mentioned above, adding a module parameter to control this behaviour could help.

inside mutex_lock!!!

Unlock before msleep() and lock after it? Don't really know practical impact of it other than blocking from setting/reading values in parallel.

SergeyMy commented 6 months ago

Those who have DC fans might (I haven't tested this change on PWM though). As mentioned above, adding a module parameter to control this behaviour could help.

good decision

I am not sure where this issue should be fixed, I'd tend towards fixing it in fan2go.

and this is perfect

Unlock before msleep() and lock after it? Don't really know practical impact of it other than blocking from setting/reading values in parallel.

after that https://github.com/Fred78290/nct6687d/pull/83 it seems there is no need for delays at all, but more tests are needed