add Thinkpad fan module#2009
Conversation
📝 WalkthroughSummaryAdds a Linux-only ThinkPad fan-speed meter that reads /proc/acpi/ibm/fan and exposes a meter usable in text and LED modes. On success it displays " RPM"; on failure or non‑ThinkPad systems it displays "N/A". Changes
Implementation quality
Commit structure and flowLogical, cohesive splits: build wiring, platform API, and meter implementation are separated into expected files. Commits appear focused and in‑scope for a single hardware‑specific feature. Notes / considerations
WalkthroughThis PR adds ThinkPad fan speed monitoring on Linux: a header declaring ThinkpadFanMeter_class, a Platform_getThinkpadFan() reader that parses /proc/acpi/ibm/fan for "speed:" and returns RPM or -1, a meter implementation that formats RPM or "N/A", registration of the meter in Platform_meterTypes, and Makefile.am updates to include the new source and header. Poem
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9c00b940-7e90-49e5-9c2b-b34ddf374a82
📒 Files selected for processing (5)
Makefile.amlinux/Platform.clinux/Platform.hlinux/ThinkpadFan.clinux/ThinkpadFan.h
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fee7903a-ae1c-4382-9366-7931b3891250
📒 Files selected for processing (5)
Makefile.amlinux/Platform.clinux/Platform.hlinux/ThinkpadFan.clinux/ThinkpadFan.h
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb50cb71-b20f-4ce9-be52-fb1c6e08254e
📒 Files selected for processing (5)
Makefile.amlinux/Platform.clinux/Platform.hlinux/ThinkpadFan.clinux/ThinkpadFan.h
|
@CodeRabbit Are there similar endpoints for non-Thinkpad hardware and on other OS that can be easily be tapped into (e.g. sysctl on *BSD)? |
|
Tip For best results, initiate chat on the files or code changes.
Linux (non-Thinkpad) — hwmon sysfs: OpenBSD — Multiple drivers expose fans this way ( FreeBSD — A full port of the OpenBSD Implications for this PR: |
|
@foxpy Is any information you need in this PR missing from the general hwmon interface? Any other reasons to prioritize this vendor-specific interface over a more general approach? Also, what do you think of implementing a more cross-platform interface covering the other two interfaces mentioned on FreeBSD+OpenBSD too? On a minor stylistic note: I think "rpm" should be lowercase. |
For some reason I always thought that it is the only interface for a Thinkpad lmao. |
No, feel free to close this PR. I will look into using hwmon, however, I will not touch BSD interfaces as I have no machines which run BSD and I have no experience working with BSD. |
BenBE
left a comment
There was a problem hiding this comment.
The general code is fine, if you solely take into account the Thinkpad Fan interface as a source of data.
What this lacks and which I'd prefer to see, is some planning for a cross-platform interface, where a common interface (cur/min/max speed as available perhaps) could be provided by the platform and displayed by a common FanSpeedMeter on platforms where the source data is available.
|
@BenBE from a quick glance, it seems that libsensors actually provides all fan data we need, but we can also obtain it manually using hwmon. I think relying on libsensors would be better because it is already a dependency we have, but what do you think? Speaking about the generic meter interface, I am not sure if min/max is exposed anywhere (usually platforms only define either some "level" or maybe PWM duty cycle). What I am sure is that there can be more fans than one, and that means I will have to make a meter interface similar to CPU meter, except a CPU meter doesn't have to worry about there being no CPUs at all :p |
|
On Linux, libsensors is a dependency, but optional. Thus having hwmon as a backup source doesn't hurt. I think going the full "clone the CPU meter" route is overkill, thus while it would be nice to have multiple fans read out, going for only the first one suffices as a first step. We can still expand from there. Similar for the min/max values: Plan for the in the interface and use sentinels if they aren't available or not available in a way you can fit in the API. It's similar to what I do with the extension of the BatteryMeter. @coderabbitai Do Linux (libsensors, hwmon), FreeBSD, and OpenBSD expose only the current speed, or is min/max speed commonly available to query? |
|
Tip For best results, initiate chat on the files or code changes.
Linux — hwmon / libsensors The sysfs spec defines both FreeBSD Entirely driver-dependent — there is no unified interface. The OpenBSD
Practical implication for the API design: The sentinel approach you described is appropriate. Current RPM is reliably available across all three platforms. Min is sometimes available on Linux (hwmon) and on specific FreeBSD drivers. Max is essentially unavailable except on those same specific FreeBSD drivers. Designing the interface to treat missing min/max as sentinel values (e.g., -1 or 0) keeps the abstraction clean without blocking the initial implementation. |
Then why does htop not support reading CPU temperature without libsensors? If I understand correctly, all data provided by libsensors is available within hwmon interface. I think that since libsensors is commonly packaged in distros and we already rely on it, it is a good idea to use it for fans, too? Let's not invent a wheel for fans, if we don't invent for thermals :) |
Because libsensors does some heavy lifting to sort and structure the information we need for the temperature information (to assign it to the proper topology). Could be done with hwmon alone, but given the [spares the rant] that libsensors is at times, we decided against having two equivalent implementations (due to the size of duplicated code this would encompass).
There's a small but important distinction here: It's regularly packaged by distros but not always installed. And htop tries to be conservative with its hard requirements: The only hard requirement is ncurses and terminfo, while everything else is pulled in at runtime as available. Everything else (lm-sensors, lsof, strace, libunwind, libsystemd, …) is optional and only used if available.
AFAICT, the hwmon part of the implementation isn't too bad. Compared to what would be needed to read all the CPU temperature information using hwmon, the code for reading the fan information is kinda tame. And fallback doesn't even require to be full-featured: Getting the base information is sufficient; if more advanced interfaces are available (like libsensors) they should be preferred. And finally: You already wrote the hwmon code for the fan speed stuff: Why not use it (and be it as backup)? ;-) |
Thinkpad laptops running on Linux usually provide a
/proc/acpi/ibm/fanfile which allows users to read or control fan speed. I implemented a tiny module which exposes a Thinkpad fan speed meter.What I tested manually:
Some important things to consider:
linux, because, I think, Linux is the only platform which exposes this data using/proc/acpi/ibm/faninterface.