Re: [PATCH v2] cpupower: Implement powercap enabled setters

[Shuah Khan <skhan@linuxfoundation.org>]

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