Add ITE 8613 so that superio attaches to that.
Add driver for its HWM to read temperature and fan speed.
Details
- Reviewers
avg
I have a mainboard here with this chip on it. Gives reasonable numbers.
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
sys/dev/superio/superio.c | ||
---|---|---|
273 | i only know of the hardware monitor for my board. |
sys/dev/superio/superio.c | ||
---|---|---|
556–558 | is this still true? see line 448. |
$ devinfo -r [...] isab0 isa0 superio0 ACPI I/O ports: 0x2e-0x2f it8613hwm0 ACPI I/O ports: 0xa35-0xa36 [...] $ tail /var/log/messages kernel: it8613hwm0: it8613hwm_probe kernel: it8613hwm0: it8613hwm_probe kernel: it8613hwm0: <Hardware monitor on ITE SuperIO> at HWM ldn 0x04 on superio0 kernel: it8613hwm0: it8613hwm_attach kernel: it8613hwm0: it8613hwm iobase: 2608 $ sysctl dev.it8613hwm dev.it8613hwm.0.temperature2: 27C dev.it8613hwm.0.temperature1: 41C dev.it8613hwm.0.temperature0: 51C dev.it8613hwm.0.%parent: superio0 dev.it8613hwm.0.%pnpinfo: type=HWM dev.it8613hwm.0.%location: ldn=0x04 dev.it8613hwm.0.%driver: it8613hwm dev.it8613hwm.0.%desc: Hardware monitor on ITE SuperIO dev.it8613hwm.%parent:
Is this the right thing to do?
I wrote a sbin tool that opens /dev/io and does raw io to talk to the chip. Now turning that into a driver that exposes the system info as sysctl seems reasonable to me... but maybe this should go somewhere else? I'm looking for opinions.
The superio.c change looks mostly good, but I think that you can use ite_devices for your chip as well.
Most likely it does have the watchdog component.
Also, you were correct about the comment and assert on the possible vendor IDs. Those were not updated when fintek support was added.
Regarding the new HWM driver, I haven't thoroughly reviewed it yet, but from a quick look it has many deviations from FreeBSD code.
You can read style(9) man page and look at the existing code.
Things like:
- too long lines
- indentation for continuation lines
- placement of opening braces { in conditional blocks
- variable definitions as a separate block
- debugging printf-s should be removed or conditional
thanks for taking a look.
yeah lots of style issues. i'll fix those.
but otherwise no objection to this being a kmod and putting the temp/fan/etc into sysctls?
$ sysctl dev.it8613hwm dev.it8613hwm.0.fan2: 1470 dev.it8613hwm.0.fan1: 648 dev.it8613hwm.0.fan0: 0 dev.it8613hwm.0.temperature2: 35C dev.it8613hwm.0.temperature1: 40C dev.it8613hwm.0.temperature0: 49C dev.it8613hwm.0.%parent: superio0 dev.it8613hwm.0.%pnpinfo: type=HWM dev.it8613hwm.0.%location: ldn=0x04 dev.it8613hwm.0.%driver: it8613hwm dev.it8613hwm.0.%desc: Hardware monitor on ITE SuperIO dev.it8613hwm.%parent: $ sysctl -a | grep temp [...] hw.acpi.thermal.tz0.temperature: 25.1C dev.it8613hwm.0.temperature2: 35C dev.it8613hwm.0.temperature1: 40C dev.it8613hwm.0.temperature0: 49C dev.jedec_dimm.1.temp: 37.6C dev.jedec_dimm.0.temp: 37.2C dev.amdtemp.0.ccd0: 34.6C dev.amdtemp.0.core0.sensor0: 35.6C dev.amdtemp.0.sensor_offset: 0 [...] dev.cpu.0.temperature: 35.6C [...]
Notice that acpi says the temp is 10 deg lower. Looks like it does not take into account the sensor offset. (Or the BIOS is botched... not unlikely tbh...)
So checking my boards ASL and acpi_thermal, looks like access to those hardware monitor registers are completely unsynchronised. That can be a problem: if we allow an unpriviledged user to initiate a read here that could glitch a concurrently running acpi_thermal update. What exactly will happen depends on the BIOS (in the case of my board a user could cause some gpio functions to be triggered, no idea whats hanging off those, maybe just emergency shutdown).
works pretty well, here's a grafana screenshot of the numbers that this driver spits out.