i/builtin/power-control: add paths for battery charging thresholds to power-control interface #13722
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Everyone contributing to this PR have now signed the CLA. Thanks! |
|
PR failed on CLA check, because I didn’t fill out the Contributor agreement form. Which I did do afterwards as individual contributor and since I didn’t know what to fill out in “Please add the Canonical Project Manager or contact” field I put your name here @alexmurray. I see @kenvandine volunteered himself for this field in another (same) issue someone encountered, and could be someone I could refer to but I went with Alex :) If I did this step right, is there to re-run this step or I need to re-create this PR? |
alexmurray
requested changes
Mar 17, 2024
There was a problem hiding this comment.
@AdnanHodzic Thanks for this but it is the wrong interface - hardware-observe should not be granting write permission - instead can this please be added to the power-control interface as I suggested previously? https://forum.snapcraft.io/t/request-for-kernel-module-control-interface-for-auto-cpufreq/38953/12 Thanks.
AdnanHodzic
force-pushed
the
battery-limit-thresholds
branch
from
88bd067
to
a1a47db
Compare
@alexmurray I apologize for misunderstanding, changes amended and pushed. P.S: unfortunately, CLA check step is still failing. |
AdnanHodzic
changed the title
|
I re-ran the CLA check but it failed again. I don't quite know how that works (how soon is the check able to see the data). Let's wait 24 hours, rebase on master and push - we've landed a bunch of fixes ahead of release so this will both give us another chance to see the data and give us a view of any impact / failures. |
alexmurray
reviewed
Mar 18, 2024
interfaces/builtin/power_control.go
Outdated
| @@ -49,6 +49,10 @@ const powerControlConnectedPlugAppArmor = ` | |||
|
|
|||
| # for android kernels, see https://android.googlesource.com/kernel/msm/+/android-msm-bullhead-3.10-marshmallow-dr/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt | |||
| /sys/module/lpm_levels/parameters/sleep_disabled rw, | |||
|
|
|||
| # Needed for ACPI modules to read and set values for battery charging thresholds | |||
| /sys/class/power_supply/BAT{,*}/charge_start_threshold rw, | |||
There was a problem hiding this comment.
$ realpath /sys/class/power_supply/BAT0/charge_stop_threshold
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1b/PNP0C09:00/PNP0C0A:00/power_supply/BAT0/charge_stop_threshold
AppArmor evaluates files based on their canonical name, so I think instead you might need something like:
/sys/devices/**/power_supply/BAT[0-9]*/charge_stop_threshold rw,
You can test this by editing the existing snap profile for auto-cpufreq which should be at /var/lib/snapd/apparmor/profiles/snap.auto-cpufreq
And then reloading it via:
sudo apparmor_parser -r /var/lib/snapd/apparmor/profiles/snap.auto-cpufreq
Let me know what you find.
There was a problem hiding this comment.
Made and pushed suggested changes as:
/sys/devices/**/power_supply/BAT[0-9]*/charge_stop_threshold rw, definitely makes more sense.
You can test this by editing the existing snap profile for auto-cpufreq which should be at /var/lib/snapd/apparmor/profiles/snap.auto-cpufreq
for auto-cpufreq under /var/lib/snapd/apparmor/profiles there are a few files:
snap.auto-cpufreq.auto-cpufreq
snap.auto-cpufreq.auto-cpufreq-gtk
snap.auto-cpufreq.hook.configure
snap.auto-cpufreq.service
snap-update-ns.auto-cpufreq
What I'm looking for exactly am I looking to test as part of these files as some of them are over 1000 lines.
There was a problem hiding this comment.
So you should add that line:
/sys/devices/**/power_supply/BAT[0-9]*/charge_stop_threshold rw,
to all of the snap.auto-cpufreq.* profiles, then reload them via sudo apparmor_parser -r /var/lib/snapd/apparmor/profiles/snap.auto-cpufreq.xxx and check that things work as you expect.
There was a problem hiding this comment.
I just rebuilt snap package locally, using the latest snapcraft.yaml contents on master branch.
I installed it using --dangerous flag, i.e sudo snap install auto-cpufreq_2.2.0_amd64.snap --dangerous
I've added:
/sys/devices/**/power_supply/BAT[0-9]*/charge_start_threshold rw,
/sys/devices/**/power_supply/BAT[0-9]*/charge_stop_threshold rw,
right before } to every file that had mention of auto-cpufreq under /var/lib/snapd/apparmor/profiles except snap-update-ns.auto-cpufreq
Completely list I added these lines to:
snap.auto-cpufreq.auto-cpufreq
snap.auto-cpufreq.auto-cpufreq-gtk
snap.auto-cpufreq.hook.configure
snap.auto-cpufreq.service
Reloaded each one of the above files with sudo apparmor_parser -r but running auto-cpufreq still failed, i.e:
sudo auto-cpufreq --live
Using settings defined in /etc/auto-cpufreq.conf file
Note: You can quit live mode by pressing "ctrl+c"
----------------------------------- Warning -----------------------------------
Due to Snap package confinement limitations please consider installing auto-cpufreq using
auto-cpufreq-installer: https://github.com/AdnanHodzic/auto-cpufreq/#auto-cpufreq-installer
Unable to detect state of GNOME Power Profiles daemon service!
This daemon might interfere with auto-cpufreq and should be disabled.
Steps to perform this action using auto-cpufreq: power_helper script:
git clone https://github.com/AdnanHodzic/auto-cpufreq.git
cd auto-cpufreq/auto_cpufreq
python3 power_helper.py --gnome_power_disable
Reference: https://github.com/AdnanHodzic/auto-cpufreq#configuring-auto-cpufreq
----------------------------------- Warning -----------------------------------
Unable to detect if you are using a TLP service!
This daemon might interfere with auto-cpufreq which can lead to unexpected results.
We strongly encourage you not to use TLP unless you really know what you are doing.
-------------------------------------------------------------------------------
PermissionError(13, 'Permission denied')
Linux distro: UNKNOWN distro UNKNOWN version
Linux kernel: 6.5.0-25-generic
Processor: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
Cores: 8
Architecture: x86_64
Driver: intel_pstate
------------------------------ Current CPU stats ------------------------------
CPU max frequency: 4600 MHz
CPU min frequency: 400 MHz
Core Usage Temperature Frequency
CPU0 0.0% nan °C 4096 MHz
CPU1 2.0% nan °C 4542 MHz
CPU2 1.0% nan °C 4380 MHz
CPU3 4.0% nan °C 400 MHz
CPU4 2.0% nan °C 4475 MHz
CPU5 1.0% nan °C 400 MHz
CPU6 0.0% nan °C 400 MHz
CPU7 0.0% nan °C 4292 MHz
---------------------------- CPU frequency scaling ----------------------------
Traceback (most recent call last):
File "/snap/auto-cpufreq/x2/bin/auto-cpufreq", line 8, in <module>
sys.exit(main())
File "/snap/auto-cpufreq/x2/lib/python3.10/site-packages/click/core.py", line 1157, in __call__
return self.main(*args, **kwargs)
File "/snap/auto-cpufreq/x2/lib/python3.10/site-packages/click/core.py", line 1078, in main
rv = self.invoke(ctx)
File "/snap/auto-cpufreq/x2/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/snap/auto-cpufreq/x2/lib/python3.10/site-packages/click/core.py", line 783, in invoke
return __callback(*args, **kwargs)
File "/snap/auto-cpufreq/x2/lib/python3.10/site-packages/auto_cpufreq/bin/auto_cpufreq.py", line 139, in main
set_autofreq()
File "/snap/auto-cpufreq/x2/lib/python3.10/site-packages/auto_cpufreq/core.py", line 1146, in set_autofreq
elif charging():
File "/snap/auto-cpufreq/x2/lib/python3.10/site-packages/auto_cpufreq/core.py", line 331, in charging
with open(Path(power_supply_path + supply + "/type")) as f:
PermissionError: [Errno 13] Permission denied: '/sys/class/power_supply/AC/type'
Which seems to be the same problem that I was mentioning in forum post.
What I did then is I uploaded the latest snap build to beta channel snapcraft upload auto-cpufreq_2.2.0_amd64.snap --release beta and re-installed auto-cpufreq from beta channel sudo snap install auto-cpufreq --channel=beta and re-ran the same command:
sudo auto-cpufreq --live
Using settings defined in /etc/auto-cpufreq.conf file
Note: You can quit live mode by pressing "ctrl+c"
Traceback (most recent call last):
File "/snap/auto-cpufreq/150/bin/auto-cpufreq", line 8, in <module>
sys.exit(main())
File "/snap/auto-cpufreq/150/lib/python3.10/site-packages/click/core.py", line 1157, in __call__
return self.main(*args, **kwargs)
File "/snap/auto-cpufreq/150/lib/python3.10/site-packages/click/core.py", line 1078, in main
rv = self.invoke(ctx)
File "/snap/auto-cpufreq/150/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/snap/auto-cpufreq/150/lib/python3.10/site-packages/click/core.py", line 783, in invoke
return __callback(*args, **kwargs)
File "/snap/auto-cpufreq/150/lib/python3.10/site-packages/auto_cpufreq/bin/auto_cpufreq.py", line 122, in main
battery_setup()
File "/snap/auto-cpufreq/150/lib/python3.10/site-packages/auto_cpufreq/battery_scripts/battery.py", line 41, in battery_setup
if lsmod("thinkpad_acpi"):
File "/snap/auto-cpufreq/150/lib/python3.10/site-packages/auto_cpufreq/battery_scripts/battery.py", line 10, in lsmod
output = subprocess.run(
File "/usr/lib/python3.10/subprocess.py", line 503, in run
with Popen(*popenargs, **kwargs) as process:
File "/usr/lib/python3.10/subprocess.py", line 971, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "/usr/lib/python3.10/subprocess.py", line 1863, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: 'lsmod'
After I moved config file, sudo mv /etc/auto-cpufreq.conf /tmp and re-ran auto-cpufreq, its run looked lot more successful then before:
sudo auto-cpufreq --live
Note: You can quit live mode by pressing "ctrl+c"
----------------------------------- Warning -----------------------------------
Due to Snap package confinement limitations please consider installing auto-cpufreq using
auto-cpufreq-installer: https://github.com/AdnanHodzic/auto-cpufreq/#auto-cpufreq-installer
Unable to detect state of GNOME Power Profiles daemon service!
This daemon might interfere with auto-cpufreq and should be disabled.
Steps to perform this action using auto-cpufreq: power_helper script:
git clone https://github.com/AdnanHodzic/auto-cpufreq.git
cd auto-cpufreq/auto_cpufreq
python3 power_helper.py --gnome_power_disable
Reference: https://github.com/AdnanHodzic/auto-cpufreq#configuring-auto-cpufreq
----------------------------------- Warning -----------------------------------
Unable to detect if you are using a TLP service!
This daemon might interfere with auto-cpufreq which can lead to unexpected results.
We strongly encourage you not to use TLP unless you really know what you are doing.
-------------------------------------------------------------------------------
Linux distro: Ubuntu 23.10 (Mantic Minotaur)
Linux kernel: 6.5.0-25-generic
Processor: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
Cores: 8
Architecture: x86_64
Driver: intel_pstate
------------------------------ Current CPU stats ------------------------------
CPU max frequency: 800 MHz
CPU min frequency: 400 MHz
Core Usage Temperature Frequency
CPU0 10.0% 49 °C 800 MHz
CPU1 20.4% 51 °C 800 MHz
CPU2 9.3% 49 °C 800 MHz
CPU3 10.3% 51 °C 800 MHz
CPU4 11.3% 49 °C 801 MHz
CPU5 16.5% 51 °C 800 MHz
CPU6 8.2% 49 °C 800 MHz
CPU7 10.1% 51 °C 800 MHz
CPU fan speed: 0 RPM
---------------------------- CPU frequency scaling ----------------------------
Battery is: charging
Setting to use: "performance" governor
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
/snap/auto-cpufreq/150/usr/bin/cpufreqctl.auto-cpufreq: line 105: echo: write error: Device or resource busy
Setting to use: "balance_performance" EPP
Setting maximum CPU frequency to 4600 Mhz
Total CPU usage: 11.8 %
Total system load: 1.83
Average temp. of all cores: 50.00 °C
High CPU load (load average: 1.83, 1.15, 1.05)
setting turbo boost: on
-------------------------------------------------------------------------------
"auto-cpufreq" is about to refresh ..
But as I said in forum post, if I install previous auto-cpufreq 2.0.0 version from (stable) release, it'll run just fine without any errors using this same config file. As also mentioned in blog, I think the only big change that was added to config file since v2.2.0 release is ability to limit battery charging threshold.
Meulengracht
changed the title
@zyga today around 12CET I got an email that I got added to "contributor-agreement-canonical" and it gets past CLA check now. Only thing that remains is a change @alexmurray requested, which I did. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #13722 +/- ##
==========================================
- Coverage 78.90% 78.90% -0.01%
==========================================
Files 1043 1043
Lines 134337 134345 +8
==========================================
+ Hits 106004 106010 +6
Misses 21721 21721
- Partials 6612 6614 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
The details above still don't necessarily convince me that this is working as expected - I think we really need to see the following:
|
Agreed, something is still not right. I'm at KubeCon ATM and don't have access to my private laptop, hence it'll take me some time to check this myself. In meantime I'll ask one of the project contributors if they can check this. Just as heads up if any other names then mine start making comments to the PR. |
|
@alexmurray I finally got to what was requested of me:
Example denial looks like: You can see the contents of whole file here.
Example denial looks like: Which looks the same to me as before apparmor profile changes? Hence I'm either doing something wrong or changes aren't applied accordingly. You can see the contents of whole file here. Hope this helps ... |
|
Hi @AdnanHodzic This line indicates that your application also need (at least read) access to which is not granted by the proposed apparmor rules Could you give it a try also including (in addition to previous ones) following apparmor rule to see if anything changes? Thanks! |
ernestl
added
the
Needs security review
Hi @jslarraz! I apologize for such delayed reply, I was travelling for work last few weeks and didn't have access to my private laptop. Regardless, I did what you suggested here, I've appended Which I fixed by appending that I made to: However, while now the app doesn't crash, I'm still facing permission issues, i.e: and "Temperatures" are being reported as Hence looking forward to hear if you have any other suggestions how to resolve this issue. |
|
Could you please share DENIALS captured via snappy-debug when you running auto-cpufreq with the new set of app armor rules? |
Most of the denials revolved around: Which is weird since system-observe is already part of snapcraft.yaml. |
|
So, I think that Those are certainly unexpected as it should be allowed by the template. Could you please confirm that this rule effectively ended up in your application profile? Output should contain The only thing that comes to my mind already is that the denial may be related to running the application as sudo. Could you please add the following rule (note it does not contain the
Thanks |
I've added following block: to following files: Reloaded each file with |
|
So it seems we are still missing at least the following rule |
besides this one I also needed to add Final list of all rules added is now: and with it I'll still end up getting a lot of "Permission denied" List of of few snappy denials: |
|
Is your snap plugging Note that connecting a new interface will make snapd to recreate the apparmor profile, so you will need to add the rules in discussion afterward again |
Yes it is, current snapcraft.yaml looks like: |
|
Could you also please confirm that the interface is effectively connect by issuing I'm asking because |
Yes |
|
@AdnanHodzic actually that output shows the interface is not connected - if it was it would look like Can you please try running: |
My bad, I misinterpreted it and I just went through all plugs that were in snapcraft.yaml and connected them manually: Unfortunately, when I run |
…art of auto-cpufreq
|
Hey @alexmurray @jslarraz, good news! Today I made an auto-cpufreq v2.3.0 release and along with it pulled latest updates on my system (Ubuntu 24.04) that included some snapd changes. Rebuilt the .snap package, made necessary plug connections that are in my snapcraft.yaml, i.e: Along with proposed changes: to all apparmor profile files: and after reloading the changes with I've pushed the changes to power_control interface, please let me know if any additional changes are needed, otherwise I hope this PR is ready to be merged. |
alexmurray
requested changes
May 10, 2024
interfaces/builtin/power_control.go
Outdated
| /sys/devices/**/power_supply/BAT[0-9]*/status r, | ||
| /sys/devices/**/power_supply/AC/type r, | ||
| /sys/devices/**/power_supply/AC/online r, | ||
| @{PROC}/@{pid}/cmdline r, |
There was a problem hiding this comment.
@{PROC}/@{pid}/cmdline r, is not really appropriate in the power-control interface - can this be removed and instead if this is really required, your snap should plug the system-observe interface which already provides access to this data.
There was a problem hiding this comment.
Line removed, changes pushed
jslarraz
reviewed
May 10, 2024
alexmurray
approved these changes
May 14, 2024
alexmurray
removed
the
Needs security review
@alexmurray I see merge is still blocked, is there anything else I can do to have it merged? |
|
@AdnanHodzic I've just approved the GitHub actions to run, once they are complete then one of the snapd maintainers can choose to merge it (unfortunately I don't have that permission). |
Thanks for the clarification, now that Github are complete I'll wait for one of snapd maintainers to merge it then. |
|
Just did a last pass on this. The rules added to the |
jslarraz
approved these changes
Jun 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
auto-cpufreq project has had snap package for few years now, and with release of 2.2.0 which adds functionality to set battery charging thresholds, its snap package is in broken state since.
Reason for this is because
hardware-observeinterface doesn't have ability to write to/sys/class/power_supply/. This was discussion and this PR was suggested as part of Snapcraft forum: Request for kernel-module-control interface for auto-cpufreq by @alexmurrayCurrently only 2 locations auto-cpufreq will need to write to are:
but this list might extended in future.
Since this is my first contribution to snapd, I hope I've made the right changes, as this is how I interpreted where & how changes should be made. If not please let me know what I need to fix/improve.
Thanks!
Adnan