Re: [PATCH] tools/powercap: Implement powercap_set_enabled()

[Shuah Khan <skhan@linuxfoundation.org>]

On 5/13/25 19:20, Suchit K wrote:
> Thanks for the suggestion, Shuah. And I did compile the kernel after
> making the changes.
> 
> 

Please don't top post in kernel patch responses.

> On Wed, 14 May 2025 at 03:33, Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 5/10/25 12:47, Suchit Karunakaran wrote:
>>> The powercap_set_enabled() function previously returned a dummy value
>>> and was marked with a TODO comment. This patch implements the function
>>> by writing the desired mode (0 or 1) to /sys/class/powercap/intel-rapl/enabled
>>
>> The short summary should say cpupower: Implement powercap_set_enabled()

I am not clear on how this can be used. Can you elaborate on the use-case
for the set?

Send me v2 fixing the change log.

>>>
>>> Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>
>>> ---
>>>    tools/power/cpupower/lib/powercap.c | 22 ++++++++++++++++++----
>>>    1 file changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
>>> index 94a0c69e55ef..7947b9809239 100644
>>> --- a/tools/power/cpupower/lib/powercap.c
>>> +++ b/tools/power/cpupower/lib/powercap.c
>>> @@ -70,6 +70,22 @@ static int sysfs_get_enabled(char *path, int *mode)
>>>        return ret;
>>>    }
>>>
>>> +static int sysfs_set_enabled(const char *path, int mode)
>>> +{
>>> +     int fd;
>>> +     char buf[2] = { mode ? '1' : '0', '\n' };
>>> +     ssize_t ret;
>>> +
>>> +     fd = open(path, O_WRONLY);
>>> +     if (fd == -1)
>>> +             return -1;
>>> +
>>> +     ret = write(fd, buf, sizeof(buf));
>>> +     close(fd);
>>> +
>>> +     return ret == sizeof(buf) ? 0 : -1;
>>> +}
>>> +
>>>    int powercap_get_enabled(int *mode)
>>>    {
>>>        char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
>>> @@ -77,12 +93,10 @@ 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;
>>> +     char path[SYSFS_PATH_MAX] = PATH_TO_POWERCAP "/intel-rapl/enabled";
>>> +     return sysfs_set_enabled(path, mode);
>>
>> Did you compile this?
>>
>> thanks,
>> -- Shuah