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

interfaces/{cpu,power}-control: add more accesses for commercial device tuning #11404

Merged

Conversation

anonymouse64
Copy link
Member

See the init_post_boot.sh[1] script for a summary of these accesses. There are
other accesses we are still evaluating where to provide them, they likely
belong in either custom-devices or in system-files for specific devices
however.

See also salesforce ticket 00320679.

[1] https://source.codeaurora.org/quic/le/snap/yaml/tree/sa8155p-gadget/postboot/init_post_boot.sh?h=UC.UM.1.0.r1-00600-sa8155.0

…ce tuning

See the init_post_boot.sh[1] script for a summary of these accesses. There are
other accesses we are still evaluating where to provide them, they likely
belong in either custom-devices or in system-files for specific devices
however.

See also salesforce ticket 00320679.

[1] https://source.codeaurora.org/quic/le/snap/yaml/tree/sa8155p-gadget/postboot/init_post_boot.sh?h=UC.UM.1.0.r1-00600-sa8155.0

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
@anonymouse64 anonymouse64 added Simple 😃 A small PR which can be reviewed quickly Needs security review Can only be merged once security gave a :+1: labels Feb 17, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #11404 (45f2a0e) into master (f3f669d) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11404   +/-   ##
=======================================
  Coverage   78.34%   78.35%           
=======================================
  Files         931      931           
  Lines      107005   107005           
=======================================
+ Hits        83838    83840    +2     
+ Misses      17951    17949    -2     
  Partials     5216     5216           
Flag Coverage Δ
unittests 78.35% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
interfaces/builtin/cpu_control.go 100.00% <ø> (ø)
interfaces/builtin/power_control.go 100.00% <ø> (ø)
cmd/snap/cmd_debug_state.go 70.64% <0.00%> (+0.45%) ⬆️
daemon/api_connections.go 93.58% <0.00%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3f669d...45f2a0e. Read the comment docs.

Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

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

LGTM! That script has a few other accesses that are missing from here (polling_interval, mem_latency/*, bw_hwmon/*); I guess we don't need them then?

@anonymouse64
Copy link
Member Author

LGTM! That script has a few other accesses that are missing from here (polling_interval, mem_latency/, bw_hwmon/); I guess we don't need them then?

These are being analyzed separately, I'm actually unsure if the for loop in the script with

    for device in /sys/devices/platform/soc

is supposed to actually be

    for device in /sys/devices/platform/soc/*

which will change the actual files we grant permissions for. I wanted to get these proposed while waiting for confirmation about the others

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Collaborator

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@alexmurray alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Feb 22, 2022
@pedronis pedronis merged commit 97c2620 into snapcore:master Feb 22, 2022
@anonymouse64 anonymouse64 deleted the feature/optimization-script-accesses branch May 6, 2022 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
6 participants