dominiksalvet / asus-fan-control

Fan control for ASUS devices running Linux
MIT License
320 stars 37 forks source link

asus-fan-control doesn't automatically insert acpi_call module #18

Closed agura-lex closed 4 years ago

agura-lex commented 4 years ago

Environment Arch Linux

Describe the bug After the update of acpi_call package, asus-fan-control stopped working. It seems that it relies on a distribution to load the module (which, in this case, it no longer does on boot), which, I think, is not a very robust solution.

Possible solutions a) Add modprobe acpi_call to asus-fan-control b) Add ExecStartPre=modprobe acpi_call to asus-fan-control.service to make sure it gets started on boot

dominiksalvet commented 4 years ago

Got it and thank you for the report! :heart:

But please wait until I prepare and merge the new-api branch. Then we can investigate the issue together. Is it okay for you?

agura-lex commented 4 years ago

Sure, I remember that you are working on that. Just drafted an issue to not forget about it. Take your time. ❤️

agura-lex commented 4 years ago

BTW, don't think "systemd" label is relevant here unless we decide to fix it in service file. :D Nonetheless, that's what I did for my local version, which I decided to push to AUR since there are some fixes since 2.12.0 and 3.0 has a fair bit of work yet to be done.

agura-lex commented 4 years ago

Patched .service version for Arch looks like that now:

#-------------------------------------------------------------------------------
# Copyright 2019-2020 Alexander Agura
# github.com/dominiksalvet/asus-fan-control
#-------------------------------------------------------------------------------

[Unit]
Description=Fan control for ASUS devices running Linux.
Before=multi-user.target
After=hibernate.target suspend-then-hibernate.target modprobe@acpi_call.service
Wants=modprobe@acpi_call.service

[Service]
Type=oneshot
ExecStart=/usr/bin/asus-fan-control

[Install]
WantedBy=multi-user.target hibernate.target suspend-then-hibernate.target
dominiksalvet commented 4 years ago

Yeah, there is something to explain... I have added the systemd label because it is most likely going to be the solution, at least in my current opinion.

I am definitely against the solution embedded in the asus-fan-control source file itself. The reasons are simple:

BTW, asus-fan-control relies on /etc/modules when it comes to acpi_call module loading. Please take a look at precp file. It also loads acpi_call immediately to apply changes once the installation using GitPack is finished.

You know what? If you want, feel free to create the PR and we can slowly talk about the details. Maybe we can even merge it before 3.0.0 is out.

agura-lex commented 4 years ago

Unnecessary asus-fan-control software dependency (from the asus-fan-control file perspective)

Well, every GNU/Linux system has got kmod package which provides the tools necessary to insert/remove kernel modules: lsmod, modprobe, insmod etc.

  • Do once what you have to do once (and we have a strong tool to do so - systemd)
  • The asus-fan-control itself is intended to be run at boot (again using systemd)

Those are good points.

BTW, asus-fan-control relies on /etc/modules when it comes to acpi_call module loading

I somehow missed that it does it. And that's a not very portable solution, I guess, since /etc/modules is kinda a Ubuntu thing if I'm not mistaken, at least I couldn't find any documentation referencing it except man modules in Ubuntu. Well, that's yet another point for switching to a more cross-distro method of handling it.

To be fair, I'm perfectly fine moving it to a systemd service, and if it fits your vision of the project better, than I'm all for it. :+1:

agura-lex commented 4 years ago

Will draft a PR soon

dominiksalvet commented 4 years ago

Closing issue and we will continue in #19.