Rework the CPUGovernors enum a bit.

Change to CPUScalingGovernors and use strum
crate to remove some cruft.

Cargo.lock

@@ -920,6 +920,12 @@ dependencies = [
  "windows-sys 0.52.0",
 ]
 
+[[package]]
+name = "rustversion"
+version = "1.0.17"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "955d28af4278de8121b7ebeb796b6a45735dc01436d898801014aced2773a3d6"
+
 [[package]]
 name = "serde"
 version = "1.0.201"
@@ -1027,6 +1033,7 @@ dependencies = [
  "libc",
  "nix",
  "serde",
+ "strum",
  "tempfile",
  "tokio",
  "tokio-stream",
@@ -1040,6 +1047,28 @@ dependencies = [
  "zbus_xml",
 ]
 
+[[package]]
+name = "strum"
+version = "0.26.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5d8cec3501a5194c432b2b7976db6b7d10ec95c253208b45f83f7136aa985e29"
+dependencies = [
+ "strum_macros",
+]
+
+[[package]]
+name = "strum_macros"
+version = "0.26.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4c6bee85a5a24955dc440386795aa378cd9cf82acd5f764469152d2270e581be"
+dependencies = [
+ "heck",
+ "proc-macro2",
+ "quote",
+ "rustversion",
+ "syn 2.0.63",
+]
+
 [[package]]
 name = "syn"
 version = "1.0.109"

Cargo.toml

@@ -29,3 +29,4 @@ tracing-subscriber = { version = "0.3", default-features = false, features = ["e
 udev = "0.8"
 xdg = "2.5"
 zbus = { version = "4", default-features = false, features = ["tokio"] }
+strum = { version = "0.26", features = ["derive"] }

com.steampowered.SteamOSManager1.xml

@@ -312,10 +312,10 @@
 
         Enumerate the supported cpu governors on the system.
 
-        A list of supported governors (a dictionary of values to names)
+        A list of supported governor names
         Version available: 9
     -->
-    <property name="CpuGovernors" type="a{us}" access="read"/>
+    <property name="AvailableCpuScalingGovernors" type="as" access="read"/>
 
     <!--
         CpuGovernor:
@@ -326,7 +326,7 @@
 
         Version available: 9
     -->
-    <property name="CpuGovernor" type="u" access="readwrite"/>
+    <property name="CpuScalingGovernor" type="s" access="readwrite"/>
 
   </interface>
 

src/bin/steamosctl.rs

@@ -11,7 +11,7 @@ use itertools::Itertools;
 use std::ops::Deref;
 use steamos_manager::cec::HdmiCecState;
 use steamos_manager::hardware::FanControlState;
-use steamos_manager::power::{CPUGovernor, GPUPerformanceLevel, GPUPowerProfile};
+use steamos_manager::power::{CPUScalingGovernor, GPUPerformanceLevel, GPUPowerProfile};
 use steamos_manager::proxy::ManagerProxy;
 use steamos_manager::wifi::{WifiBackend, WifiDebugMode, WifiPowerManagement};
 use zbus::fdo::PropertiesProxy;
@@ -45,16 +45,16 @@ enum Commands {
     /// Get the fan control state
     GetFanControlState,
 
-    /// Get the CPU governors supported on this device
+    /// Get the available CPU scaling governors supported on this device
-    GetCpuGovernors,
+    GetAvailableCpuScalingGovernors,
 
     /// Get the current CPU governor
-    GetCpuGovernor,
+    GetCpuScalingGovernor,
 
-    /// Set the current CPU governor
+    /// Set the current CPU Scaling governor
-    SetCpuGovernor {
+    SetCpuScalingGovernor {
         /// Valid governors are get-cpu-governors.
-        governor: CPUGovernor,
+        governor: CPUScalingGovernor,
     },
 
     /// Get the GPU power profiles supported on this device
@@ -215,29 +215,27 @@ async fn main() -> Result<()> {
                 Err(_) => println!("Got unknown value {state} from backend"),
             }
         }
-        Commands::GetCpuGovernors => {
+        Commands::GetAvailableCpuScalingGovernors => {
-            let governors = proxy.cpu_governors().await?;
+            let governors = proxy.available_cpu_scaling_governors().await?;
             println!("Governors:\n");
-            for key in governors.keys().sorted() {
+            for name in governors {
-                let name = &governors[key];
+                println!("{name}");
-                println!("{key}: {name}");
             }
         }
-        Commands::GetCpuGovernor => {
+        Commands::GetCpuScalingGovernor => {
-            let governor = proxy.cpu_governor().await?;
+            let governor = proxy.cpu_scaling_governor().await?;
-            let governor_type = CPUGovernor::try_from(governor);
+            let governor_type = CPUScalingGovernor::try_from(governor.as_str());
             match governor_type {
-                Ok(t) => {
+                Ok(_) => {
-                    let name = t.to_string();
+                    println!("CPU Governor: {governor}");
-                    println!("CPU Governor: {governor} {name}");
                 }
                 Err(_) => {
                     println!("Unknown CPU governor or unable to get type from {governor}");
                 }
             }
         }
-        Commands::SetCpuGovernor { governor } => {
+        Commands::SetCpuScalingGovernor { governor } => {
-            proxy.set_cpu_governor(*governor as u32).await?;
+            proxy.set_cpu_scaling_governor(governor.to_string()).await?;
         }
         Commands::GetGPUPowerProfiles => {
             let profiles = proxy.gpu_power_profiles().await?;

src/manager/root.rs

@@ -20,8 +20,8 @@ use crate::daemon::DaemonCommand;
 use crate::error::{to_zbus_error, to_zbus_fdo_error};
 use crate::hardware::{variant, FanControl, FanControlState, HardwareVariant};
 use crate::power::{
-    set_cpu_governor, set_gpu_clocks, set_gpu_performance_level, set_gpu_power_profile,
+    set_cpu_scaling_governor, set_gpu_clocks, set_gpu_performance_level, set_gpu_power_profile,
-    set_tdp_limit, CPUGovernor, GPUPerformanceLevel, GPUPowerProfile,
+    set_tdp_limit, CPUScalingGovernor, GPUPerformanceLevel, GPUPowerProfile,
 };
 use crate::process::{run_script, script_output, ProcessManager};
 use crate::wifi::{
@@ -191,11 +191,11 @@ impl SteamOSManager {
             .map_err(to_zbus_fdo_error)
     }
 
-    async fn set_cpu_governor(&self, governor: u32) -> fdo::Result<()> {
+    async fn set_cpu_scaling_governor(&self, governor: String) -> fdo::Result<()> {
-        let governor = CPUGovernor::try_from(governor).map_err(to_zbus_fdo_error)?;
+        let g = CPUScalingGovernor::try_from(governor.as_str()).map_err(to_zbus_fdo_error)?;
-        set_cpu_governor(governor)
+        set_cpu_scaling_governor(g)
             .await
-            .inspect_err(|message| error!("Error setting CPU governor: {message}"))
+            .inspect_err(|message| error!("Error setting CPU scaling governor: {message}"))
             .map_err(to_zbus_fdo_error)
     }
 

src/manager/user.rs

@@ -20,8 +20,8 @@ use crate::daemon::DaemonCommand;
 use crate::error::{to_zbus_error, to_zbus_fdo_error, zbus_to_zbus_fdo};
 use crate::hardware::check_support;
 use crate::power::{
-    get_cpu_governor, get_cpu_governors, get_gpu_clocks, get_gpu_performance_level,
+    get_available_cpu_scaling_governors, get_cpu_scaling_governor, get_gpu_clocks,
-    get_gpu_power_profile, get_gpu_power_profiles, get_tdp_limit,
+    get_gpu_performance_level, get_gpu_power_profile, get_gpu_power_profiles, get_tdp_limit,
 };
 use crate::wifi::{get_wifi_backend, get_wifi_power_management_state};
 use crate::API_VERSION;
@@ -192,18 +192,31 @@ impl SteamOSManager {
     }
 
     #[zbus(property(emits_changed_signal = "false"))]
-    async fn cpu_governors(&self) -> fdo::Result<HashMap<u32, String>> {
+    async fn available_cpu_scaling_governors(&self) -> fdo::Result<Vec<String>> {
-        get_cpu_governors().await.map_err(to_zbus_fdo_error)
+        let governors = get_available_cpu_scaling_governors()
+            .await
+            .map_err(to_zbus_fdo_error)?;
+        let mut result = Vec::new();
+        for g in governors {
+            result.push(g.to_string());
+        }
+        Ok(result)
     }
 
     #[zbus(property(emits_changed_signal = "false"))]
-    async fn cpu_governor(&self) -> fdo::Result<u32> {
+    async fn cpu_scaling_governor(&self) -> fdo::Result<String> {
-        get_cpu_governor().await.map_err(to_zbus_fdo_error)
+        let governor = get_cpu_scaling_governor()
+            .await
+            .map_err(to_zbus_fdo_error)?;
+        Ok(governor.to_string())
     }
 
     #[zbus(property)]
-    async fn set_cpu_governor(&self, governor: u32) -> zbus::Result<()> {
+    async fn set_cpu_scaling_governor(&self, governor: String) -> zbus::Result<()> {
-        setter!(self, "SetCpuGovernor", &(governor))
+        self.proxy
+            .call("SetCpuScalingGovernor", &(governor))
+            .await
+            .map_err(to_zbus_error)
     }
 
     #[zbus(property(emits_changed_signal = "false"))]

src/power.rs

@@ -10,9 +10,10 @@ use std::collections::HashMap;
 use std::fmt;
 use std::path::PathBuf;
 use std::str::FromStr;
+use strum::{Display, EnumString};
 use tokio::fs::{self, File};
 use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
-use tracing::error;
+use tracing::{error, warn};
 
 use crate::hardware::is_deck;
 use crate::{path, write_synced};
@@ -21,7 +22,7 @@ const GPU_HWMON_PREFIX: &str = "/sys/class/hwmon";
 const GPU_HWMON_NAME: &str = "amdgpu";
 const GPU_DRM_PREFIX: &str = "/sys/class/drm";
 const GPU_VENDOR: &str = "0x1002";
-const CPU_GOVERNOR_PREFIX: &str = "/sys/devices/system/cpu/cpufreq";
+const CPU_PREFIX: &str = "/sys/devices/system/cpu/cpufreq";
 
 const CPU0_NAME: &str = "policy0";
 const CPU_POLICY_NAME: &str = "policy";
@@ -29,8 +30,8 @@ const CPU_POLICY_NAME: &str = "policy";
 const GPU_POWER_PROFILE_SUFFIX: &str = "device/pp_power_profile_mode";
 const GPU_PERFORMANCE_LEVEL_SUFFIX: &str = "device/power_dpm_force_performance_level";
 const GPU_CLOCKS_SUFFIX: &str = "device/pp_od_clk_voltage";
-const CPU_GOVERNOR_SUFFIX: &str = "scaling_governor";
+const CPU_SCALING_GOVERNOR_SUFFIX: &str = "scaling_governor";
-const CPU_GOVERNORS_SUFFIX: &str = "scaling_available_governors";
+const CPU_SCALING_AVAILABLE_GOVERNORS_SUFFIX: &str = "scaling_available_governors";
 
 const TDP_LIMIT1: &str = "power1_cap";
 const TDP_LIMIT2: &str = "power2_cap";
@@ -149,9 +150,9 @@ impl fmt::Display for GPUPerformanceLevel {
     }
 }
 
-#[derive(Hash, Eq, PartialEq, Debug, Copy, Clone)]
+#[derive(Display, EnumString, Hash, Eq, PartialEq, Debug, Copy, Clone)]
-#[repr(u32)]
+#[strum(serialize_all = "lowercase")]
-pub enum CPUGovernor {
+pub enum CPUScalingGovernor {
     Conservative,
     OnDemand,
     UserSpace,
@@ -160,49 +161,6 @@ pub enum CPUGovernor {
     SchedUtil,
 }
 
-impl TryFrom<u32> for CPUGovernor {
-    type Error = &'static str;
-    fn try_from(v: u32) -> Result<Self, Self::Error> {
-        match v {
-            x if x == CPUGovernor::Conservative as u32 => Ok(CPUGovernor::Conservative),
-            x if x == CPUGovernor::OnDemand as u32 => Ok(CPUGovernor::OnDemand),
-            x if x == CPUGovernor::UserSpace as u32 => Ok(CPUGovernor::UserSpace),
-            x if x == CPUGovernor::PowerSave as u32 => Ok(CPUGovernor::PowerSave),
-            x if x == CPUGovernor::Performance as u32 => Ok(CPUGovernor::Performance),
-            x if x == CPUGovernor::SchedUtil as u32 => Ok(CPUGovernor::SchedUtil),
-            _ => Err("No enum match for value {v}"),
-        }
-    }
-}
-
-impl FromStr for CPUGovernor {
-    type Err = Error;
-    fn from_str(input: &str) -> Result<CPUGovernor, Self::Err> {
-        Ok(match input {
-            "conservative" => CPUGovernor::Conservative,
-            "ondemand" => CPUGovernor::OnDemand,
-            "userspace" => CPUGovernor::UserSpace,
-            "powersave" => CPUGovernor::PowerSave,
-            "performance" => CPUGovernor::Performance,
-            "schedutil" => CPUGovernor::SchedUtil,
-            v => bail!("No enum match for value {v}"),
-        })
-    }
-}
-
-impl fmt::Display for CPUGovernor {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            CPUGovernor::Conservative => write!(f, "conservative"),
-            CPUGovernor::OnDemand => write!(f, "ondemand"),
-            CPUGovernor::UserSpace => write!(f, "userspace"),
-            CPUGovernor::PowerSave => write!(f, "powersave"),
-            CPUGovernor::Performance => write!(f, "performance"),
-            CPUGovernor::SchedUtil => write!(f, "schedutil"),
-        }
-    }
-}
-
 async fn read_gpu_sysfs_contents() -> Result<String> {
     // check which profile is current and return if possible
     let base = find_gpu_prefix().await?;
@@ -212,24 +170,24 @@ async fn read_gpu_sysfs_contents() -> Result<String> {
 }
 
 async fn read_cpu_governor_sysfs_available_contents() -> Result<String> {
-    let base = path(CPU_GOVERNOR_PREFIX);
+    let base = path(CPU_PREFIX);
-    fs::read_to_string(base.join(CPU0_NAME).join(CPU_GOVERNORS_SUFFIX))
+    Ok(fs::read_to_string(
-        .await
+        base.join(CPU0_NAME)
-        .map_err(|message| anyhow!("Error opening sysfs file for reading {message}"))
+            .join(CPU_SCALING_AVAILABLE_GOVERNORS_SUFFIX)
+    )
+    .await?)
 }
 
 async fn read_cpu_governor_sysfs_contents() -> Result<String> {
     // Read contents of policy0 path
-    let base = path(CPU_GOVERNOR_PREFIX);
+    let base = path(CPU_PREFIX);
-    let full_path = base.join(CPU0_NAME).join(CPU_GOVERNOR_SUFFIX);
+    let full_path = base.join(CPU0_NAME).join(CPU_SCALING_GOVERNOR_SUFFIX);
-    fs::read_to_string(full_path)
+    Ok(fs::read_to_string(full_path).await?)
-        .await
-        .map_err(|message| anyhow!("Error opening sysfs file for reading {message}"))
 }
 
 async fn write_cpu_governor_sysfs_contents(contents: String) -> Result<()> {
-    // Iterate over all cpuX paths
+    // Iterate over all policyX paths
-    let mut dir = fs::read_dir(path(CPU_GOVERNOR_PREFIX)).await?;
+    let mut dir = fs::read_dir(path(CPU_PREFIX)).await?;
     let mut wrote_stuff = false;
     loop {
         let base = match dir.next_entry().await? {
@@ -238,34 +196,24 @@ async fn write_cpu_governor_sysfs_contents(contents: String) -> Result<()> {
                     .file_name()
                     .into_string()
                     .map_err(|_| anyhow!("Unable to convert path to string"))?;
-                if file_name.starts_with(CPU_POLICY_NAME) {
+                if !file_name.starts_with(CPU_POLICY_NAME) {
-                    entry.path()
-                } else {
-                    // Not a policy path, so move on
                     continue;
                 }
+                entry.path()
             }
             None => {
-                if wrote_stuff {
+                ensure!(
-                    return Ok(());
+                    wrote_stuff,
-                } else {
+                    "No data written, unable to find any policyX sysfs paths"
-                    bail!("No data written, unable to find any policyX sysfs paths")
+                );
-                }
+                return Ok(());
             }
         };
-        let file_name = base.join(CPU_GOVERNOR_SUFFIX);
-        println!(
-            "Trying to write to file at path: {:?}",
-            file_name.as_os_str()
-        );
-        let mut myfile = File::create(file_name)
-            .await
-            .inspect_err(|message| error!("Error opening sysfs file for writing: {message}"))?;
-
         // Write contents to each one
         wrote_stuff = true;
-        myfile.write_all(contents.as_bytes()).await?;
+        write_synced(base.join(CPU_SCALING_GOVERNOR_SUFFIX), contents.as_bytes())
-        myfile.sync_data().await?;
+            .await
+            .inspect_err(|message| error!("Error writing to sysfs file: {message}"))?
     }
 }
 
@@ -360,41 +308,35 @@ pub(crate) async fn set_gpu_performance_level(level: GPUPerformanceLevel) -> Res
         .inspect_err(|message| error!("Error writing to sysfs file: {message}"))
 }
 
-pub(crate) async fn get_cpu_governors() -> Result<HashMap<u32, String>> {
+pub(crate) async fn get_available_cpu_scaling_governors() -> Result<Vec<CPUScalingGovernor>> {
     let contents = read_cpu_governor_sysfs_available_contents().await?;
     // Get the list of supported governors from cpu0
-    let mut result = HashMap::new();
+    let mut result = Vec::new();
 
     let words = contents.split_whitespace();
     for word in words {
-        match CPUGovernor::from_str(word) {
+        match CPUScalingGovernor::from_str(word) {
-            Ok(v) => {
+            Ok(governor) => result.push(governor),
-                result.insert(v as u32, word.to_string());
+            Err(message) => warn!("Error parsing governor {message}"),
-            }
+        }
-            Err(message) => bail!("Error parsing governor {message}"),
-        };
     }
 
     Ok(result)
 }
 
-pub(crate) async fn get_cpu_governor() -> Result<u32> {
+pub(crate) async fn get_cpu_scaling_governor() -> Result<CPUScalingGovernor> {
     // get the current governor from cpu0 (assume all others are the same)
     let contents = read_cpu_governor_sysfs_contents().await?;
 
-    let governor = match CPUGovernor::from_str(contents.trim()) {
+    let contents = contents.trim();
-        Ok(v) => v,
+    CPUScalingGovernor::from_str(contents)
-        Err(_) => bail!("Error converting CPU governor sysfs file contents to enumeration"),
+        .map_err(|message| anyhow!("Error converting CPU scaling governor sysfs file contents to enumeration: {message}"))
-    };
-
-    Ok(governor as u32)
 }
 
-pub(crate) async fn set_cpu_governor(governor: CPUGovernor) -> Result<()> {
+pub(crate) async fn set_cpu_scaling_governor(governor: CPUScalingGovernor) -> Result<()> {
     // Set the given governor on all cpus
     let name = governor.to_string();
-    write_cpu_governor_sysfs_contents(name).await?;
+    write_cpu_governor_sysfs_contents(name).await
-    Ok(())
 }
 
 pub(crate) async fn set_gpu_clocks(clocks: u32) -> Result<()> {
@@ -803,13 +745,7 @@ CCLK_RANGE in Core0:
 
     #[test]
     fn cpu_governor_roundtrip() {
-        enum_roundtrip!(CPUGovernor {
+        enum_roundtrip!(CPUScalingGovernor {
-            0: u32 = Conservative,
-            1: u32 = OnDemand,
-            2: u32 = UserSpace,
-            3: u32 = PowerSave,
-            4: u32 = Performance,
-            5: u32 = SchedUtil,
             "conservative": str = Conservative,
             "ondemand": str = OnDemand,
             "userspace": str = UserSpace,
@@ -817,8 +753,7 @@ CCLK_RANGE in Core0:
             "performance": str = Performance,
             "schedutil": str = SchedUtil,
         });
-        assert!(CPUGovernor::try_from(6).is_err());
+        assert!(CPUScalingGovernor::from_str("usersave").is_err());
-        assert!(CPUGovernor::from_str("usersave").is_err());
     }
 
     #[test]

src/proxy.rs

@@ -65,15 +65,15 @@ trait Manager {
     #[zbus(property)]
     fn set_gpu_performance_level(&self, value: u32) -> zbus::Result<()>;
 
-    /// CpuGovernor property
+    /// CpuScalingGovernor property
     #[zbus(property)]
-    fn cpu_governor(&self) -> zbus::Result<u32>;
+    fn cpu_scaling_governor(&self) -> zbus::Result<String>;
     #[zbus(property)]
-    fn set_cpu_governor(&self, value: u32) -> zbus::Result<()>;
+    fn set_cpu_scaling_governor(&self, value: String) -> zbus::Result<()>;
 
-    /// CpuGovernors property
+    /// AvailableCpuScalingGovernors property
     #[zbus(property)]
-    fn cpu_governors(&self) -> zbus::Result<std::collections::HashMap<u32, String>>;
+    fn available_cpu_scaling_governors(&self) -> zbus::Result<Vec<String>>;
 
     /// GpuPowerProfile property
     #[zbus(property)]