The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH] cpupower: Implement powercap enabled setters
@ 2026-05-08 22:35 Mateusz Jaśkiewicz
  2026-05-11 21:01 ` Shuah Khan
  0 siblings, 1 reply; 10+ messages in thread
From: Mateusz Jaśkiewicz @ 2026-05-08 22:35 UTC (permalink / raw)
  To: Thomas Renninger, Shuah Khan, John B . Wyatt IV, John Kacur
  Cc: linux-pm, linux-kernel, Mateusz Jaśkiewicz

powercap_set_enabled() and powercap_zone_set_enabled() are part of the
public libcpupower API, but both currently return success without
updating sysfs.

Write the requested value to the matching enabled attribute so callers
can actually enable or disable the powercap control type or zone, and
report write failures back to the caller.

Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
---
 tools/power/cpupower/lib/powercap.c | 43 +++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
index 94a0c69e5..6d8b83428 100644
--- a/tools/power/cpupower/lib/powercap.c
+++ b/tools/power/cpupower/lib/powercap.c
@@ -70,6 +70,27 @@ static int sysfs_get_enabled(char *path, int *mode)
 	return ret;
 }
 
+static int sysfs_set_enabled(const char *path, int mode)
+{
+	char yes_no;
+	int fd;
+	ssize_t ret;
+
+	if (mode != 0 && mode != 1)
+		return -1;
+
+	yes_no = mode ? '1' : '0';
+	fd = open(path, O_WRONLY);
+	if (-1 == fd)
+		return -1;
+	ret = write(fd, &yes_no, 1);
+	if (ret != 1) {
+		close(fd);
+		return -1;
+	}
+	return close(fd);
+}
+
 int powercap_get_enabled(int *mode)
 {
 	char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
@@ -77,17 +98,13 @@ int powercap_get_enabled(int *mode)
 	return sysfs_get_enabled(path, mode);
 }
 
-/*
- * TODO: implement function. Returns dummy 0 for now.
- */
 int powercap_set_enabled(int mode)
 {
-	return 0;
+	return sysfs_set_enabled(PATH_TO_RAPL "/enabled", mode);
 }
-
 /*
  * Hardcoded, because rapl is the only powercap implementation
-- * this needs to get more generic if more powercap implementations
+ * this needs to get more generic if more powercap implementations
  * should show up
  */
 int powercap_get_driver(char *driver, int buflen)
@@ -180,8 +197,18 @@ int powercap_zone_get_enabled(struct powercap_zone *zone, int *mode)
 
 int powercap_zone_set_enabled(struct powercap_zone *zone, int mode)
 {
-	/* To be done if needed */
-	return 0;
+	char path[SYSFS_PATH_MAX];
+	int ret;
+
+	if (!zone)
+		return -1;
+
+	ret = snprintf(path, sizeof(path), "%s/%s/enabled",
+		       PATH_TO_POWERCAP, zone->sys_name);
+	if (ret < 0 || ret >= (int)sizeof(path))
+		return -1;
+
+	return sysfs_set_enabled(path, mode);
 }
 
 
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] cpupower: Implement powercap enabled setters
  2026-05-08 22:35 [PATCH] cpupower: Implement powercap enabled setters Mateusz Jaśkiewicz
@ 2026-05-11 21:01 ` Shuah Khan
  2026-05-12 16:11   ` [PATCH v2] " Mateusz Jaśkiewicz
  0 siblings, 1 reply; 10+ messages in thread
From: Shuah Khan @ 2026-05-11 21:01 UTC (permalink / raw)
  To: Mateusz Jaśkiewicz, Thomas Renninger, Shuah Khan,
	John B . Wyatt IV, John Kacur
  Cc: linux-pm, linux-kernel, Shuah Khan

On 5/8/26 16:35, Mateusz Jaśkiewicz wrote:
> powercap_set_enabled() and powercap_zone_set_enabled() are part of the
> public libcpupower API, but both currently return success without
> updating sysfs.
> 
> Write the requested value to the matching enabled attribute so callers
> can actually enable or disable the powercap control type or zone, and
> report write failures back to the caller.
> 
> Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
> ---
>   tools/power/cpupower/lib/powercap.c | 43 +++++++++++++++++++++++------
>   1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
> index 94a0c69e5..6d8b83428 100644
> --- a/tools/power/cpupower/lib/powercap.c
> +++ b/tools/power/cpupower/lib/powercap.c
> @@ -70,6 +70,27 @@ static int sysfs_get_enabled(char *path, int *mode)
>   	return ret;
>   }
>   
> +static int sysfs_set_enabled(const char *path, int mode)
> +{
> +	char yes_no;
> +	int fd;
> +	ssize_t ret;
> +
> +	if (mode != 0 && mode != 1)
> +		return -1;

The default is enabled. You could check the current value
and then do a write.

> +
> +	yes_no = mode ? '1' : '0';
> +	fd = open(path, O_WRONLY);
> +	if (-1 == fd)

I prefer fd == value convention. Also checking fd < 0 is
better than == -1 and printing error message calling perror()



> +		return -1;
> +	ret = write(fd, &yes_no, 1);
> +	if (ret != 1) {


> +		close(fd);
> +		return -1;
> +	}
> +	return close(fd);
> +}
> +
>   int powercap_get_enabled(int *mode)
>   {
>   	char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
> @@ -77,17 +98,13 @@ int powercap_get_enabled(int *mode)
>   	return sysfs_get_enabled(path, mode);
>   }
>   
> -/*
> - * TODO: implement function. Returns dummy 0 for now.
> - */
>   int powercap_set_enabled(int mode)
>   {
> -	return 0;
> +	return sysfs_set_enabled(PATH_TO_RAPL "/enabled", mode);
>   }
> -
>   /*
>    * Hardcoded, because rapl is the only powercap implementation
> -- * this needs to get more generic if more powercap implementations
> + * this needs to get more generic if more powercap implementations
>    * should show up
>    */
>   int powercap_get_driver(char *driver, int buflen)
> @@ -180,8 +197,18 @@ int powercap_zone_get_enabled(struct powercap_zone *zone, int *mode)
>   
>   int powercap_zone_set_enabled(struct powercap_zone *zone, int mode)
>   {
> -	/* To be done if needed */
> -	return 0;
> +	char path[SYSFS_PATH_MAX];
> +	int ret;
> +
> +	if (!zone)
> +		return -1;
> +
> +	ret = snprintf(path, sizeof(path), "%s/%s/enabled",
> +		       PATH_TO_POWERCAP, zone->sys_name);
> +	if (ret < 0 || ret >= (int)sizeof(path))

Drop the typecast

> +		return -1;
> +
> +	return sysfs_set_enabled(path, mode);
>   }
>   
>   

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] cpupower: Implement powercap enabled setters
  2026-05-11 21:01 ` Shuah Khan
@ 2026-05-12 16:11   ` Mateusz Jaśkiewicz
  2026-05-14 21:28     ` Shuah Khan
  2026-05-26 19:53     ` Shuah Khan
  0 siblings, 2 replies; 10+ messages in thread
From: Mateusz Jaśkiewicz @ 2026-05-12 16:11 UTC (permalink / raw)
  To: skhan
  Cc: Thomas Renninger, Shuah Khan, John B. Wyatt IV, John Kacur,
	Mateusz Jaśkiewicz, linux-pm, linux-kernel

powercap_set_enabled() and powercap_zone_set_enabled() are part of the
public libcpupower API, but both currently return success without
updating sysfs.

Write the requested value to the matching enabled attribute so callers
can actually enable or disable the powercap control type or zone, and
report write failures back to the caller.

---
Changes in v2:
- Check current enabled value before writing.
- Use fd < 0 style for open() failures.
- Print sysfs open/write failures with perror().
- Drop the sizeof(path) typecast.

Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
---
 tools/power/cpupower/lib/powercap.c | 58 +++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
index 94a0c69e5..80af65287 100644
--- a/tools/power/cpupower/lib/powercap.c
+++ b/tools/power/cpupower/lib/powercap.c
@@ -21,7 +21,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
 	ssize_t numread;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1)
+	if (fd < 0)
 		return 0;
 
 	numread = read(fd, buf, buflen - 1);
@@ -36,7 +36,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
 	return (unsigned int) numread;
 }
 
-static int sysfs_get_enabled(char *path, int *mode)
+static int sysfs_get_enabled(const char *path, int *mode)
 {
 	int fd;
 	char yes_no;
@@ -45,7 +45,7 @@ static int sysfs_get_enabled(char *path, int *mode)
 	*mode = 0;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1) {
+	if (fd < 0) {
 		ret = -1;
 		goto out;
 	}
@@ -70,6 +70,36 @@ static int sysfs_get_enabled(char *path, int *mode)
 	return ret;
 }
 
+static int sysfs_set_enabled(const char *path, int mode)
+{
+	char yes_no;
+	int fd;
+	int current_mode;
+	ssize_t ret;
+
+	if (mode != 0 && mode != 1)
+		return -1;
+
+	if (sysfs_get_enabled(path, &current_mode) == 0 &&
+	    current_mode == mode)
+		return 0;
+
+	yes_no = mode ? '1' : '0';
+	fd = open(path, O_WRONLY);
+	if (fd < 0) {
+		perror(path);
+		return -1;
+	}
+	ret = write(fd, &yes_no, 1);
+	if (ret != 1) {
+		if (ret < 0)
+			perror(path);
+		close(fd);
+		return -1;
+	}
+	return close(fd);
+}
+
 int powercap_get_enabled(int *mode)
 {
 	char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
@@ -77,17 +107,13 @@ int powercap_get_enabled(int *mode)
 	return sysfs_get_enabled(path, mode);
 }
 
-/*
- * TODO: implement function. Returns dummy 0 for now.
- */
 int powercap_set_enabled(int mode)
 {
-	return 0;
+	return sysfs_set_enabled(PATH_TO_RAPL "/enabled", mode);
 }
-
 /*
  * Hardcoded, because rapl is the only powercap implementation
-- * this needs to get more generic if more powercap implementations
+ * this needs to get more generic if more powercap implementations
  * should show up
  */
 int powercap_get_driver(char *driver, int buflen)
@@ -180,8 +206,18 @@ int powercap_zone_get_enabled(struct powercap_zone *zone, int *mode)
 
 int powercap_zone_set_enabled(struct powercap_zone *zone, int mode)
 {
-	/* To be done if needed */
-	return 0;
+	char path[SYSFS_PATH_MAX];
+	int ret;
+
+	if (!zone)
+		return -1;
+
+	ret = snprintf(path, sizeof(path), "%s/%s/enabled",
+		       PATH_TO_POWERCAP, zone->sys_name);
+	if (ret < 0 || ret >= sizeof(path))
+		return -1;
+
+	return sysfs_set_enabled(path, mode);
 }
 
 
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] cpupower: Implement powercap enabled setters
  2026-05-12 16:11   ` [PATCH v2] " Mateusz Jaśkiewicz
@ 2026-05-14 21:28     ` Shuah Khan
  2026-05-26 19:53     ` Shuah Khan
  1 sibling, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2026-05-14 21:28 UTC (permalink / raw)
  To: Mateusz Jaśkiewicz, Thomas Renninger
  Cc: Shuah Khan, John B. Wyatt IV, John Kacur, linux-pm, linux-kernel

On 5/12/26 10:11, Mateusz Jaśkiewicz wrote:
> powercap_set_enabled() and powercap_zone_set_enabled() are part of the
> public libcpupower API, but both currently return success without
> updating sysfs.
> 
> Write the requested value to the matching enabled attribute so callers
> can actually enable or disable the powercap control type or zone, and
> report write failures back to the caller.
> 
> ---
> Changes in v2:
> - Check current enabled value before writing.
> - Use fd < 0 style for open() failures.
> - Print sysfs open/write failures with perror().
> - Drop the sizeof(path) typecast.
> 
> Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
> ---
>   tools/power/cpupower/lib/powercap.c | 58 +++++++++++++++++++++++------
>   1 file changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
> index 94a0c69e5..80af65287 100644
> --- a/tools/power/cpupower/lib/powercap.c
> +++ b/tools/power/cpupower/lib/powercap.c
> @@ -21,7 +21,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
>   	ssize_t numread;
>   
>   	fd = open(path, O_RDONLY);
> -	if (fd == -1)
> +	if (fd < 0)
>   		return 0;
>   
>   	numread = read(fd, buf, buflen - 1);
> @@ -36,7 +36,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
>   	return (unsigned int) numread;
>   }
>   
> -static int sysfs_get_enabled(char *path, int *mode)
> +static int sysfs_get_enabled(const char *path, int *mode)
>   {
>   	int fd;
>   	char yes_no;
> @@ -45,7 +45,7 @@ static int sysfs_get_enabled(char *path, int *mode)
>   	*mode = 0;
>   
>   	fd = open(path, O_RDONLY);
> -	if (fd == -1) {
> +	if (fd < 0) {
>   		ret = -1;
>   		goto out;
>   	}
> @@ -70,6 +70,36 @@ static int sysfs_get_enabled(char *path, int *mode)
>   	return ret;
>   }
>   
> +static int sysfs_set_enabled(const char *path, int mode)
> +{
> +	char yes_no;
> +	int fd;
> +	int current_mode;
> +	ssize_t ret;
> +
> +	if (mode != 0 && mode != 1)
> +		return -1;
> +
> +	if (sysfs_get_enabled(path, &current_mode) == 0 &&
> +	    current_mode == mode)
> +		return 0;
> +
> +	yes_no = mode ? '1' : '0';
> +	fd = open(path, O_WRONLY);
> +	if (fd < 0) {
> +		perror(path);
> +		return -1;
> +	}
> +	ret = write(fd, &yes_no, 1);
> +	if (ret != 1) {
> +		if (ret < 0)
> +			perror(path);
> +		close(fd);
> +		return -1;
> +	}
> +	return close(fd);
> +}
> +
>   int powercap_get_enabled(int *mode)
>   {
>   	char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
> @@ -77,17 +107,13 @@ int powercap_get_enabled(int *mode)
>   	return sysfs_get_enabled(path, mode);
>   }
>   
> -/*
> - * TODO: implement function. Returns dummy 0 for now.
> - */
>   int powercap_set_enabled(int mode)
>   {
> -	return 0;
> +	return sysfs_set_enabled(PATH_TO_RAPL "/enabled", mode);
>   }
> -
>   /*
>    * Hardcoded, because rapl is the only powercap implementation
> -- * this needs to get more generic if more powercap implementations
> + * this needs to get more generic if more powercap implementations
>    * should show up
>    */
>   int powercap_get_driver(char *driver, int buflen)
> @@ -180,8 +206,18 @@ int powercap_zone_get_enabled(struct powercap_zone *zone, int *mode)
>   
>   int powercap_zone_set_enabled(struct powercap_zone *zone, int mode)
>   {
> -	/* To be done if needed */
> -	return 0;
> +	char path[SYSFS_PATH_MAX];
> +	int ret;
> +
> +	if (!zone)
> +		return -1;
> +
> +	ret = snprintf(path, sizeof(path), "%s/%s/enabled",
> +		       PATH_TO_POWERCAP, zone->sys_name);
> +	if (ret < 0 || ret >= sizeof(path))
> +		return -1;
> +
> +	return sysfs_set_enabled(path, mode);
>   }
>   
>   

Thomas,

Please take a look at the _set_enable() addition and let me
know if it is okay to apply this patch.

Are there any tests you would like to see run to verify this
set API?

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] cpupower: Implement powercap enabled setters
  2026-05-12 16:11   ` [PATCH v2] " Mateusz Jaśkiewicz
  2026-05-14 21:28     ` Shuah Khan
@ 2026-05-26 19:53     ` Shuah Khan
  2026-05-27  8:12       ` [PATCH v3] " Mateusz Jaśkiewicz
  1 sibling, 1 reply; 10+ messages in thread
From: Shuah Khan @ 2026-05-26 19:53 UTC (permalink / raw)
  To: Mateusz Jaśkiewicz
  Cc: Thomas Renninger, Shuah Khan, John B. Wyatt IV, John Kacur,
	linux-pm, linux-kernel, Shuah Khan

On 5/12/26 10:11, Mateusz Jaśkiewicz wrote:
> powercap_set_enabled() and powercap_zone_set_enabled() are part of the
> public libcpupower API, but both currently return success without
> updating sysfs.
> 
> Write the requested value to the matching enabled attribute so callers
> can actually enable or disable the powercap control type or zone, and
> report write failures back to the caller.
> 
> ---

Everything after the the 3 dots gets thrown away when patch is
applied. Patch version changes should be added after the
Signed-off-by
s
> Changes in v2:
> - Check current enabled value before writing.
> - Use fd < 0 style for open() failures.
> - Print sysfs open/write failures with perror().
> - Drop the sizeof(path) typecast.
> 
> Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
> ---

Here

>   tools/power/cpupower/lib/powercap.c | 58 +++++++++++++++++++++++------
>   1 file changed, 47 insertions(+), 11 deletions(-)
> 

Please fix this and send me v3.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3] cpupower: Implement powercap enabled setters
  2026-05-26 19:53     ` Shuah Khan
@ 2026-05-27  8:12       ` Mateusz Jaśkiewicz
  2026-05-27 15:26         ` Shuah Khan
  0 siblings, 1 reply; 10+ messages in thread
From: Mateusz Jaśkiewicz @ 2026-05-27  8:12 UTC (permalink / raw)
  To: Thomas Renninger, Shuah Khan, John B. Wyatt IV, John Kacur
  Cc: Mateusz Jaśkiewicz, linux-pm, linux-kernel

powercap_set_enabled() and powercap_zone_set_enabled() are part of the
public libcpupower API, but both currently return success without
updating sysfs.

Write the requested value to the matching enabled attribute so callers
can actually enable or disable the powercap control type or zone, and
report write failures back to the caller.

Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
---
Changes in v3:
- Move the patch changelog below the Signed-off-by trailer so the
  sign-off is kept when the patch is applied.

Changes in v2:
- Check current enabled value before writing.
- Use fd < 0 style for open() failures.
- Print sysfs open/write failures with perror().
- Drop the sizeof(path) typecast.

 tools/power/cpupower/lib/powercap.c | 58 +++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
index 94a0c69e5..80af65287 100644
--- a/tools/power/cpupower/lib/powercap.c
+++ b/tools/power/cpupower/lib/powercap.c
@@ -21,7 +21,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
 	ssize_t numread;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1)
+	if (fd < 0)
 		return 0;
 
 	numread = read(fd, buf, buflen - 1);
@@ -36,7 +36,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
 	return (unsigned int) numread;
 }
 
-static int sysfs_get_enabled(char *path, int *mode)
+static int sysfs_get_enabled(const char *path, int *mode)
 {
 	int fd;
 	char yes_no;
@@ -45,7 +45,7 @@ static int sysfs_get_enabled(char *path, int *mode)
 	*mode = 0;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1) {
+	if (fd < 0) {
 		ret = -1;
 		goto out;
 	}
@@ -70,6 +70,36 @@ static int sysfs_get_enabled(char *path, int *mode)
 	return ret;
 }
 
+static int sysfs_set_enabled(const char *path, int mode)
+{
+	char yes_no;
+	int fd;
+	int current_mode;
+	ssize_t ret;
+
+	if (mode != 0 && mode != 1)
+		return -1;
+
+	if (sysfs_get_enabled(path, &current_mode) == 0 &&
+	    current_mode == mode)
+		return 0;
+
+	yes_no = mode ? '1' : '0';
+	fd = open(path, O_WRONLY);
+	if (fd < 0) {
+		perror(path);
+		return -1;
+	}
+	ret = write(fd, &yes_no, 1);
+	if (ret != 1) {
+		if (ret < 0)
+			perror(path);
+		close(fd);
+		return -1;
+	}
+	return close(fd);
+}
+
 int powercap_get_enabled(int *mode)
 {
 	char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
@@ -77,17 +107,13 @@ int powercap_get_enabled(int *mode)
 	return sysfs_get_enabled(path, mode);
 }
 
-/*
- * TODO: implement function. Returns dummy 0 for now.
- */
 int powercap_set_enabled(int mode)
 {
-	return 0;
+	return sysfs_set_enabled(PATH_TO_RAPL "/enabled", mode);
 }
-
 /*
  * Hardcoded, because rapl is the only powercap implementation
-- * this needs to get more generic if more powercap implementations
+ * this needs to get more generic if more powercap implementations
  * should show up
  */
 int powercap_get_driver(char *driver, int buflen)
@@ -180,8 +206,18 @@ int powercap_zone_get_enabled(struct powercap_zone *zone, int *mode)
 
 int powercap_zone_set_enabled(struct powercap_zone *zone, int mode)
 {
-	/* To be done if needed */
-	return 0;
+	char path[SYSFS_PATH_MAX];
+	int ret;
+
+	if (!zone)
+		return -1;
+
+	ret = snprintf(path, sizeof(path), "%s/%s/enabled",
+		       PATH_TO_POWERCAP, zone->sys_name);
+	if (ret < 0 || ret >= sizeof(path))
+		return -1;
+
+	return sysfs_set_enabled(path, mode);
 }
 
 
-- 
2.54.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] cpupower: Implement powercap enabled setters
  2026-05-27  8:12       ` [PATCH v3] " Mateusz Jaśkiewicz
@ 2026-05-27 15:26         ` Shuah Khan
  2026-05-28 15:32           ` [PATCH v4] " Mateusz Jaśkiewicz
  0 siblings, 1 reply; 10+ messages in thread
From: Shuah Khan @ 2026-05-27 15:26 UTC (permalink / raw)
  To: Mateusz Jaśkiewicz, Thomas Renninger, Shuah Khan,
	John B. Wyatt IV, John Kacur
  Cc: linux-pm, linux-kernel, Shuah Khan

On 5/27/26 02:12, Mateusz Jaśkiewicz wrote:
> powercap_set_enabled() and powercap_zone_set_enabled() are part of the
> public libcpupower API, but both currently return success without
> updating sysfs.
> 
> Write the requested value to the matching enabled attribute so callers
> can actually enable or disable the powercap control type or zone, and
> report write failures back to the caller.
> 
> Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
> ---
> Changes in v3:
> - Move the patch changelog below the Signed-off-by trailer so the
>    sign-off is kept when the patch is applied.
> 
> Changes in v2:
> - Check current enabled value before writing.
> - Use fd < 0 style for open() failures.
> - Print sysfs open/write failures with perror().
> - Drop the sizeof(path) typecast.
> 
>   tools/power/cpupower/lib/powercap.c | 58 +++++++++++++++++++++++------
>   1 file changed, 47 insertions(+), 11 deletions(-)
> 

Something to fix here:

lib/powercap.c: In function ‘powercap_zone_set_enabled’:
lib/powercap.c:217:28: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare]
   217 |         if (ret < 0 || ret >= sizeof(path))
       |                            ^~

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4] cpupower: Implement powercap enabled setters
  2026-05-27 15:26         ` Shuah Khan
@ 2026-05-28 15:32           ` Mateusz Jaśkiewicz
  2026-06-03 18:17             ` Shuah Khan
  0 siblings, 1 reply; 10+ messages in thread
From: Mateusz Jaśkiewicz @ 2026-05-28 15:32 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Thomas Renninger, John B. Wyatt IV, John Kacur, linux-pm,
	linux-kernel, Mateusz Jaśkiewicz

powercap_set_enabled() and powercap_zone_set_enabled() are part of the
public libcpupower API, but both currently return success without
updating sysfs.

Write the requested value to the matching enabled attribute so callers
can actually enable or disable the powercap control type or zone, and
report write failures back to the caller.

Signed-off-by: Mateusz Jaśkiewicz <jaskiewiczteo@gmail.com>
---
Changes in v4:
- Compare the snprintf() return value against SYSFS_PATH_MAX to avoid
  the signedness warning without adding a typecast.

Changes in v3:
- Move the patch changelog below the Signed-off-by trailer so the
  sign-off is kept when the patch is applied.

Changes in v2:
- Check current enabled value before writing.
- Use fd < 0 style for open() failures.
- Print sysfs open/write failures with perror().
- Drop the sizeof(path) typecast.

 tools/power/cpupower/lib/powercap.c | 58 +++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
index 94a0c69e5..cbbfc4736 100644
--- a/tools/power/cpupower/lib/powercap.c
+++ b/tools/power/cpupower/lib/powercap.c
@@ -21,7 +21,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
 	ssize_t numread;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1)
+	if (fd < 0)
 		return 0;
 
 	numread = read(fd, buf, buflen - 1);
@@ -36,7 +36,7 @@ static unsigned int sysfs_read_file(const char *path, char *buf, size_t buflen)
 	return (unsigned int) numread;
 }
 
-static int sysfs_get_enabled(char *path, int *mode)
+static int sysfs_get_enabled(const char *path, int *mode)
 {
 	int fd;
 	char yes_no;
@@ -45,7 +45,7 @@ static int sysfs_get_enabled(char *path, int *mode)
 	*mode = 0;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1) {
+	if (fd < 0) {
 		ret = -1;
 		goto out;
 	}
@@ -70,6 +70,36 @@ static int sysfs_get_enabled(char *path, int *mode)
 	return ret;
 }
 
+static int sysfs_set_enabled(const char *path, int mode)
+{
+	char yes_no;
+	int fd;
+	int current_mode;
+	ssize_t ret;
+
+	if (mode != 0 && mode != 1)
+		return -1;
+
+	if (sysfs_get_enabled(path, &current_mode) == 0 &&
+	    current_mode == mode)
+		return 0;
+
+	yes_no = mode ? '1' : '0';
+	fd = open(path, O_WRONLY);
+	if (fd < 0) {
+		perror(path);
+		return -1;
+	}
+	ret = write(fd, &yes_no, 1);
+	if (ret != 1) {
+		if (ret < 0)
+			perror(path);
+		close(fd);
+		return -1;
+	}
+	return close(fd);
+}
+
 int powercap_get_enabled(int *mode)
 {
 	char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
@@ -77,17 +107,13 @@ int powercap_get_enabled(int *mode)
 	return sysfs_get_enabled(path, mode);
 }
 
-/*
- * TODO: implement function. Returns dummy 0 for now.
- */
 int powercap_set_enabled(int mode)
 {
-	return 0;
+	return sysfs_set_enabled(PATH_TO_RAPL "/enabled", mode);
 }
-
 /*
  * Hardcoded, because rapl is the only powercap implementation
-- * this needs to get more generic if more powercap implementations
+ * this needs to get more generic if more powercap implementations
  * should show up
  */
 int powercap_get_driver(char *driver, int buflen)
@@ -180,8 +206,18 @@ int powercap_zone_get_enabled(struct powercap_zone *zone, int *mode)
 
 int powercap_zone_set_enabled(struct powercap_zone *zone, int mode)
 {
-	/* To be done if needed */
-	return 0;
+	char path[SYSFS_PATH_MAX];
+	int ret;
+
+	if (!zone)
+		return -1;
+
+	ret = snprintf(path, sizeof(path), "%s/%s/enabled",
+		       PATH_TO_POWERCAP, zone->sys_name);
+	if (ret < 0 || ret >= SYSFS_PATH_MAX)
+		return -1;
+
+	return sysfs_set_enabled(path, mode);
 }
 
 
-- 
2.54.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] cpupower: Implement powercap enabled setters
  2026-05-28 15:32           ` [PATCH v4] " Mateusz Jaśkiewicz
@ 2026-06-03 18:17             ` Shuah Khan
       [not found]               ` <CA+Z_OmTMGTib=eCEGNhCbd=7pmR8Zq7cResyRb=v_wWyRoEdfg@mail.gmail.com>
  0 siblings, 1 reply; 10+ messages in thread
From: Shuah Khan @ 2026-06-03 18:17 UTC (permalink / raw)
  To: Mateusz Jaśkiewicz, Shuah Khan
  Cc: Thomas Renninger, John B. Wyatt IV, John Kacur, linux-pm,
	linux-kernel, Shuah Khan

On 5/28/26 09:32, Mateusz Jaśkiewicz wrote:
> powercap_set_enabled() and powercap_zone_set_enabled() are part of the
> public libcpupower API, but both currently return success without
> updating sysfs.

You are right that cpupower set_enable API simply returns success.

Does intel-rapl:0 implement store function on your system?
It doesn't on mine.

cd /sys/devices/virtual/powercap/intel-rapl/intel-rapl:0
echo 1 > enabled
bash: echo: write error: Function not implemented

> 
> Write the requested value to the matching enabled attribute so callers
> can actually enable or disable the powercap control type or zone, and
> report write failures back to the caller.
> 

The patch loos good. Is there an existing use-case for this?

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] cpupower: Implement powercap enabled setters
       [not found]                   ` <CA+Z_OmRN0bFXCxRg4SVNP-M0v6G2cME0HszPNyHhP5w7ti2Usw@mail.gmail.com>
@ 2026-06-18 15:23                     ` Shuah Khan
  0 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2026-06-18 15:23 UTC (permalink / raw)
  To: Mateusz Jaśkiewicz, John B . Wyatt IV, John Kacur,
	Thomas Renninger
  Cc: linux-kernel, linux-pm, Shuah Khan

On 6/17/26 22:18, Mateusz Jaśkiewicz wrote:
>  > So when you tried to write it worked looks like. The new
> 

Please follow the kernel mailing list convention of posting
at the bottom of the message you are replying you instead
adding your response at the top. You can find the details
in the kernel documentation.

> What I meant by "silent failure" is the situation, when we call a function and it just returns success without doing anything (the situation with current implementation).



> 
> 
>  > value is 1 - what was it before you ran echo 1?
> 
> It was 0 before. That being said there are zones that after writing 0 to them remained 1. Should I add check for this and fail if the value remains unchanged despite intention?

Okay so in this case it worked the way it should and the
value changed. When writing 0 (echo 0) to the file, did
you see a message that says "not supported"

I would like to see an concrete use-case for adding the
set enable API. We added this stub when SWIG support was
added.

powercap_set_enabled() could return ENOTSUPP instead of
0 if we don't have a concrete use-case that uses this
API.

Also if you are looking to improve things cpupower,
looks like sysfs_read_file() could be improved. The
return values are inconsistent and confusing. In some
return values aren't checked.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-06-18 15:23 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-08 22:35 [PATCH] cpupower: Implement powercap enabled setters Mateusz Jaśkiewicz
2026-05-11 21:01 ` Shuah Khan
2026-05-12 16:11   ` [PATCH v2] " Mateusz Jaśkiewicz
2026-05-14 21:28     ` Shuah Khan
2026-05-26 19:53     ` Shuah Khan
2026-05-27  8:12       ` [PATCH v3] " Mateusz Jaśkiewicz
2026-05-27 15:26         ` Shuah Khan
2026-05-28 15:32           ` [PATCH v4] " Mateusz Jaśkiewicz
2026-06-03 18:17             ` Shuah Khan
     [not found]               ` <CA+Z_OmTMGTib=eCEGNhCbd=7pmR8Zq7cResyRb=v_wWyRoEdfg@mail.gmail.com>
     [not found]                 ` <e056d7f2-935f-4efd-bdef-d0f696026d7b@linuxfoundation.org>
     [not found]                   ` <CA+Z_OmRN0bFXCxRg4SVNP-M0v6G2cME0HszPNyHhP5w7ti2Usw@mail.gmail.com>
2026-06-18 15:23                     ` Shuah Khan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox