Re: Issue with toshiba fan sensors & missing feedback on patch

[Hans de Goede <hdegoede@redhat.com>]

Hi Arvid,

On 10/7/22 14:51, Arvid Norlander wrote:
> On 2022-10-07 13:22, Hans de Goede wrote:
>> 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".
> 
> After testing, thankfully it seems to be the case that it is mostly
> sleeping!

That is good, still unfortunate that it takes so long though.

> 
>>
>> 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.
> 
> I will try to look into this, hopefully this weekend, though I'm currently
> down with a nasty cold, so I can't promise anything.
> 
>> 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?
> 
> These are separate: A shared one for AC current and AC voltage, and two
> separate ones for battery current and battery voltage. There is also
> more data in the AC reply one that I can't quite decode, but it seems to
> be thermally related (but I have not managed to decode it as a temperature
> that matches anything else). Annoyingly the windows software displays
> almost everything in percentages on dial gauges without any units or even
> scales, making actually figuring out the interpretations rather difficult
> at times.
> 
> For reading the power a call is can be issued to set the time resolution,
> in powers of two between 1 and 16 (i.e. 1, 2, 4, 8 and 16 are valid values),
> where 1 is slightly more than one new reading per second, while 16 is about
> one reading every 20ish seconds.
> 
> This suggests that the EC would be updating some periodic registers
> internally and we are just querying them, however, these are still just as
> slow to read as the fan.

Hmm, I'm not sure what is the best thing to do here, if we end up making
4 calls of each 50 ms here, then that is going to block the caller for
200 ms which I guess might be just acceptable if we do it only once
per x seconds, for some value of x...

I guess you could use power_supply_is_system_supplied() to skip
the AC readings when not charging, but that ends up calling _PSR
on the ACPI AC device, which might also be slow.


> 
>>
>> 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.
> 
> I do seem to have voltage_now and power_now. So current could be computed
> by measuring the delta of energy_now over time and then using the voltage
> to compute the current.
> 
> However, the voltage as reported in /sys/class/power_supply/BAT1 does not
> quite match the voltage I get from the HCI calls. The HCI call consistently
> reads higher, but how much higher depends on if the AC is connected or not.
> Also voltage_now seems to update rather more slowly than the HCI calls.
> 
>>
>> 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.
> 
> Yes they are slow, but they might be interesting in order to compute
> system power usage while on AC (AC power - battery power) as it reports
> the battery charging current while charging and the discharging current
> while discharging.
> 
> At the very least I will be documenting them when I get time.

Yes that sounds like a good idea. I guess you could also add support
for them in the driver, but then maybe activated with some "extended_hwmon"
kernel-module option. Which then should default to off I guess?

Regards,

Hans