Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add installation with dkms #31

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Diman119
Copy link

@Diman119 Diman119 commented Aug 29, 2023

Added a persistent installation script, that does 2 things - installs the module into the kernel with DKMS (and makes it survive kernel upgrades) and sets it to load at boot.
Added an uninstallation script, that reverts these actions.
Tested on Ubuntu 23.04

@anzigo
Copy link

anzigo commented Sep 3, 2023

Tested this on an Acer Swift XSFX14-41G running Fedora 38. Module continued to work and was persistent through a kernel update.

For Fedora, you'd need to install the following at a minimum:
sudo dnf group install "C Development Tools and Libraries" "Development Tools"
sudo dnf install kernel-devel dkms

@albertovito
Copy link

Tested on Acer Aspire 7 A715-42G and Ubuntu 23.04. It seems work flawless.

@brenobaptista
Copy link

brenobaptista commented Dec 16, 2023

Working on Acer Nitro AN515-57, thank you for this PR!

@brenobaptista
Copy link

@Diman119

The PR repo doesn't accept issues, so putting it here.
Somehow the module didn't work 4 days after I installed it in DKMS. Using lsmod it seems to be loaded:

image

I tried uninstalling and installing it again and got this (in Pop!_OS):

image

@brenobaptista
Copy link

brenobaptista commented Dec 22, 2023

The value went back to 0 after 2 days for no apparent reason.

image

No problem with DKMS:

image

Also no problem in lsmod:

image

I don't think this problem comes from the PR, maybe the base repository has some bug that reverts this value back to 0.

@brenobaptista
Copy link

Just remembered to update on comments above. I fixed it by adding a systemd service to force health_mode to be 1 on startup. I have no experience with building drivers but maybe to fix this it would be needed to persist this value in a file and get that file on reboot/kernel updates.

@Diman119
Copy link
Author

Diman119 commented Mar 2, 2024

@Diman119

The PR repo doesn't accept issues, so putting it here. Somehow the module didn't work 4 days after I installed it in DKMS. Using lsmod it seems to be loaded:

image

I tried uninstalling and installing it again and got this (in Pop!_OS):

image

I do not know how the driver actually works, so I can't help you there, but there indeed was a problem with the uninstall script. The script is now fixed, pull the changes and use the new one if you want a cleaner uninstallation.

Copy link
Owner

@frederik-h frederik-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The PR looks good to me and I would like to merge it after the review changes have been addressed.

1) Install DKMS and generic kernel headers (this will always get you the latest headers), on Debian or Ubuntu it can be done with:

```
sudo apt install dkms linux-headers-generic
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked what linux-headers-generic would install on my system and you are right, it depends on the package for the latest kernel headers. But what if you are not running the latest kernel version - as I am. Since the build instructions already describe how to install the headers for the current kernel, I would remove the "linux-headers-generic" from this apt invocation.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this file and the other dkms related files to a new directory contrib/dkms, move your instructions to contrib/dkms/README.md and link to this file from README.md?

Copy link
Owner

@frederik-h frederik-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some further remarks.

@@ -2,7 +2,7 @@ obj-m += acer-wmi-battery.o
PWD := $(CURDIR)

all:
make -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules
make -C /lib/modules/$(KERNELVERSION)/build M=$(PWD) modules
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need to be able to pass a kernel version to the Makefile, it would be better to make this optional, i.e. specify a default value for KERNELVERSION. This can be accomplished by defining the variable at the top of the Makefile like this:
KERNELVERSION ?= "$(shell uname -r)".

sudo ./install.sh
```

The driver will now automatically load at boot and be recompiled after a kernel upgrade. Reboot to use it.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reboot to use it.

modprobe should also work?

@@ -19,13 +19,13 @@ Any feedback on how it works on other Acer laptops would be appreciated.
## Building

Make sure that you have the kernel headers for your kernel installed
and type `make` in the cloned project directory. In more detail,
and type `make KERNELVERSION=$(uname -r)` in the cloned project directory. In more detail,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build instructions should not need to change once you adapt the Makefile as described above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants