Hi Arvid, On 10/6/22 23:12, Arvid Norlander wrote: > Hi Hans, > > Two things: > * I have discovered that reading the fan RPM in toshiba_acpi is slow, > around 50 ms. I didn't notice it at first, but after adding some more > sensors I found (current and voltage for AC and battery) it started to > make running "sensors" visibly slow. > > I don't know what proper fix to this would be. Feel free to revert the > fan RPM for now if it is not acceptable for reading sensors to block for > ~50 ms (as opposed to the 100s of micro-seconds that other sensors such > as coretemp and acpitz take to read on that laptop). Hmm, so apparently the single ACPI call this ends up making takes along time. I wonder what happens with the CPU load if you cat the file from a "while true; do cat /sys/class/hwmon/...; done" loop in the shell. With some luck most of that 50 ms is sleeping, so we won't see say 25% load on a quad core CPU (so 100% load on 1 core) in "top". Regardless we also want any desktop-environment sensor applets which poll more then once/second to block on this all the time. What most hwmon drivers with slow HW access do is update the readings once per second and then return cached values for a second see e.g. : https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/f71882fg.c and then specifically the "bool valid;" and "unsigned long last_updated; /* In jiffies */" members of "struct f71882fg_data" and how those are used. I believe that keeping the fan rpm reading, which IMHO is the most useful one is fine when adding the caching; or alternatively you can submit a revert. As for the AC + bat voltage/current can those be / are those retreived with the same tci_raw() call or do those require separate calls ? And if they require separate calls do those calls also all take 50 ms? The battery values should already be available in some form under /sys/class/power_supply/BAT* although you may only have the multiplied value of the 2 there in the form of energy_now. And the AC values are nice to have but not super interesting, so if they require another slow tci_raw() call then I'm not sure if they are worth adding. > * I never got any feedback from you on the cover letter of this patch > series: https://www.spinics.net/lists/platform-driver-x86/msg34818.html > > In a reply to the one of the patches in the series you alluded to that > you would write a reply to the cover letter as well. Instead you sent > the response to patch 1/2 twice. Sorry besides the double reply to 1/2 I did send another reply to 1/2: https://lore.kernel.org/platform-driver-x86/36cc9c55-bc8c-ed9e-3467-8be0aa450167@xxxxxxxxxx/ Where I ended up answering the userspace API question (or at least I intended to answer it there, that may not have been clear). Which is why I ended up not reply-ing to the cover-letter. I will take another look at the cover-letter and answer any other questions you may have asked there. As for the userspace API question, see my linked reply above. To summarize / clarify: - I'm fine with the suggested wakeup_cause + button_id sysfs-attributes. - For the wakeup_cause I would like to see the format be a standard kernel bool fmt as also used by module options. This mostly means using kstrtobool() in the store function - As mentioned in my reply please add a Documentation file documenting both sysfs attributes > We are already halfway through the merge > window soon, so I would appreciate getting that feedback soon. Generally speaking patches must be ready no later then around rc6 to get merged into the next release. So to get this merged you have about 7 weeks until 6.1-rc6 is released to get this ready and then I'll merge it into my pdx86/for-next branch for the 6.2 cycle. Anything feature work which is not ready around rc6 of the previous cycle will not make the current cycle (instead it gets delayed to the next cycle). I will go and answer the cover-letter now, for real this time... Regards, Hans
