On Wed, Apr 21, 2021 at 05:15:37PM -0700, Guenter Roeck wrote: > On Wed, Apr 21, 2021 at 08:31:27PM -0300, Jonas Malaco wrote: > > On Wed, Apr 21, 2021 at 10:21:36AM -0700, Guenter Roeck wrote: > > > On Wed, Apr 21, 2021 at 01:48:03PM -0300, Jonas Malaco wrote: > > > > On Tue, Apr 13, 2021 at 09:45:29AM -0300, Jonas Malaco wrote: > > > > > Guenter (and others on this list), > > > > > > > > Very gentle ping. > > > > > > > > I also thought posting these questions first would be less disruptive > > > > than a RFC patch, but please let me know if you prefer the latter. > > > > > > > > > > It is a difficult subject, and I am struggling myself with the situations > > > you are presenting. > > > > I am somewhat relieved that these issues are not so silly. And I really > > appropriate your comments. > > > > Please take a look at a few more comments bellow. > > > > > > > > > Thanks again, > > > > Jonas > > > > > > > > > > > > > > I am getting ready to submit a driver for NZXT Grid+ V3 and Smart Device > > > > > (V1) fan controllers, but I am having trouble deciding how to expose > > > > > their PWM control due to some device limitations. > > > > > > > > > > Before getting into those, let me first give some very basic context... > > > > > > > > > > These devices are USB HIDs, and asynchronously send "status" reports > > > > > every 200 ms to communicate speed, current, voltage and control mode for > > > > > their channels (one channel per report). > > > > > > > > > > Fans can be controlled by sending HID output reports to the device, and > > > > > both DC and PWM modes are supported. The device features a special > > > > > initialization routine (that must be requested during probe) which > > > > > automatically detects the appropriate control mode for each channel. > > > > > > > > > > Back to the device limitations... > > > > > > > > > > The first is that PWM values can be set, but not read back. And neither > > > > > hwmon[1] nor lm-sensors' pwmconfig/fancontrol expect pmw* attributes to > > > > > be WO. One solution is to have the driver track the PWM values that are > > > > > set through it and return those, but is this acceptable? > > > > > > I have seen a couple of those recently. I think returning -ENODATA > > > if the value isn't known (yet) is the best possible solution. I thought > > > about adding that to the ABI, actually. > > > > We can never read the pwm[1-*] attributes, not even for detected and > > controllable fans after the initialization procedure. > > > > And returning -ENODATA for pwm[1-*] reads makes pwmconfig/fancontrol > > unhappy: > > > > Seems to me that pwmconfig is then maybe not appropriate to use, > and maybe there should be no driver for this device in the kernel > in the first place. I see your point. Although, apart from this rather serious quirk, these devices work really well with pwmconfig/fancontrol. > > Returning a random value after setting the pwm value to 255, removing, > and re-inserting the driver is, in my opinion, even worse than > returning -ENODATA. After all, the driver doesn't know the pwm value, > and it is really a bad idea to report data which doesn't reflect > reality. Well, in this particular case, the driver must reinitialize the device anyway, since it cannot know whether the device was already initialized. Maybe the driver was removed because the device itself was disconnected and powered off (it's a USB device, not a chip on a board). So in the work-in-progress driver I take this opportunity to also make sure that its state matches the device's, and force all fans to their default PWM values after powering on. Specifically, besides requesting the device to initialize itself, I also (re)set all fans to 40% PWM. (This does not require waiting for the initialization to be complete). Is this egregious? Thanks, Jonas > > Guenter > > > # pwmconfig > > [...] > > Found the following PWM controls: > > cat: hwmon0/pwm1: No data available > > hwmon0/pwm1 current value: > > cat: hwmon0/pwm1: No data available > > /bin/pwmconfig: line 201: [: : integer expression expected > > cat: hwmon0/pwm1: No data available > > hwmon0/pwm1 stuck to > > Manual control mode not supported, skipping hwmon0/pwm1. > > [...] > > > > # fancontrol > > [...] > > Enabling PWM on fans... > > cat: hwmon0/pwm1: No data available > > Starting automatic fan control... > > /bin/fancontrol: line 551: read: read error: 0: No data available > > Error reading PWM value from /sys/class/hwmon/hwmon0/pwm1 > > Aborting, restoring fans... > > cat: hwmon0/pwm1: No data available > > /bin/fancontrol: line 458: [: : integer expression expected > > hwmon0/pwm1_enable stuck to 1 > > Verify fans have returned to full speed > > > > > > > > > > > > > > > The other starts with PWM control being disabled for channels that the > > > > > device identifies as unconnected. This is not in itself a problem, but > > > > > the initialization routine (where the detection happens) is > > > > > asynchronous, takes somewhere around 5 seconds, and we do not have any > > > > > way of directly querying its result. We only know the control mode of > > > > > each channel (be it DC, PWM or disabled) from the regular status > > > > > reports. > > > > > > Again, I think the best solution is to return -ENODATA until the value > > > is known. > > > > Ok. > > > > > > > > > > > > > > > These limitations make it complicated to simply use is_visible() to hide > > > > > pwm attributes of unconnected channels. We would need to register with > > > > > the hwmon subsystem only after getting enough post-initialization status > > > > > reports for all channels, and this would essentially mean to sleep for > > > > > 6+ seconds. We would also need to unregister and re-register when going > > > > > through a suspend-reset_resume cycle, because the device may have its > > > > > state wiped, requiring reinitialization.[2] > > > > > > > > I think the above should resolve that. > > > > Yes, as well as your suggestion bellow. > > > > > > > > > > A different approach to handle this, which I have preferred _so far,_ is > > > > > to use pwm*_enable = 0 to report the unconnected channels to user-space, > > > > > while keeping the other pwm attributes visible. But this comes with > > > > > other problems. > > > > > > > > > > First, lm-sensors' pwmconfig expects to be able to write to a > > > > > pwm*_enable attribute if it exists, but the device does not support that > > > > > operation. The hwmon documentation states that RW values may be RO, but > > > > > pwmconfig is out there and in use. So far I simply return 0 to attempts > > > > > at those writes, silently ignoring them; functional, but certainly a > > > > > hack. > > > > > > It is a bad idea to return 0 if the value is not accepted. You could check > > > if the written value matches the current value and return 0 if it does, > > > and an error such as -EOPNOTSUPP or -EINVAL otherwise. > > > > It worked really well, thanks! > > > > > > > > > > > > > > > Second, if PWM control is disabled for a channel, but its pwm* and > > > > > pwm*_mode attributes are still visible, what should we return when > > > > > user-space attempts to write to them? The practical answer may simply > > > > > be to return -EOPNOTSUPP, but this makes me wonder if the whole approach > > > > > (of handling these cases with pwm*_enable instead of is_visible()) is > > > > > not doomed. > > > > > > > > Mode isn't really writeable either, isn't it ? If so, use the same trick as > > > with the _enable attribute. > > > > You're right, but setting its visibility to 0444 didn't cause issues for > > pwmconfig or fancontrol, so I don't think the trick is necessary here. > > > > > > > > The same is effectively true for the pwm value itself: Since both _enable > > > and _mode are effectively read-only, you can accept a write only if > > > fan control is enabled, and return an error if it isn't. > > > > Ok. > > > > > > > > > > A final minor problem is that channels detected as unconnected run at > > > > > 40% PWM, but the documentation for pwm*_enable == 0 is a bit too > > > > > specific: "no fan speed control (i.e. fan at *full* speed)" (emphasis > > > > > mine). > > > > > > Just document the difference. Reality doesn't always match our expectations. > > > > Ok. > > > > Thanks, > > Jonas > > > > > > > > Thanks, > > > Guenter > > > > > > > > > > > > > Do you have any suggestions and/or recommendations? > > > > > > > > > > If it helps, a pre-RFC (but functional and mostly clean) version of the > > > > > driver can be found at: > > > > > > > > > > https://github.com/jonasmalacofilho/linux/blob/p-hwmon-add-nzxt-smartdevice-gridplus3/drivers/hwmon/nzxt-smartdevice.c > > > > > > > > > > Thanks, > > > > > Jonas > > > > > > > > > > [1] According to Documentation/hwmon/sysfs-interface.rst. > > > > > [2] The device also does not respond to HID Get_Report, so it is not > > > > > trivial to check whether it really needs to be reinitialized, since > > > > > the only symptom of that being necessary is the absence of the > > > > > asynchronous status reports.
