[PATCH 1/2] pm: cpupower: bench: print path fopen failed

[PATCH 1/2] pm: cpupower: bench: print path fopen failed

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Print out the config file path when fopen failed. It will be easy
for users to know where to create the file.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 tools/power/cpupower/bench/parse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
index e63dc11fa3a5..366b20f9ddf1 100644
--- a/tools/power/cpupower/bench/parse.c
+++ b/tools/power/cpupower/bench/parse.c
@@ -166,7 +166,7 @@ int prepare_config(const char *path, struct config *config)
 	configfile = fopen(path, "r");
 	if (configfile == NULL) {
 		perror("fopen");
-		fprintf(stderr, "error: unable to read configfile\n");
+		fprintf(stderr, "error: unable to read configfile: %s\n", path);
 		free(config);
 		return 1;
 	}
-- 
2.37.1

[PATCH 2/2] pm: cpupower: Makefile: better support cross-compiling

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Allow overridding the envs, this will be easier to user Yocto
cross-compiler toolchains to build cpupower with only two steps:

source (toolchain path)/environment-setup-armv8a-poky-linux
make

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 tools/power/cpupower/Makefile | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
index 6c02f401069e..e2a48af6fa2a 100644
--- a/tools/power/cpupower/Makefile
+++ b/tools/power/cpupower/Makefile
@@ -86,12 +86,12 @@ INSTALL_SCRIPT = ${INSTALL} -m 644
 # If you are running a cross compiler, you may want to set this
 # to something more interesting, like "arm-linux-".  If you want
 # to compile vs uClibc, that can be done here as well.
-CROSS = #/usr/i386-linux-uclibc/usr/bin/i386-uclibc-
-CC = $(CROSS)gcc
-LD = $(CROSS)gcc
-AR = $(CROSS)ar
-STRIP = $(CROSS)strip
-RANLIB = $(CROSS)ranlib
+CROSS ?= #/usr/i386-linux-uclibc/usr/bin/i386-uclibc-
+CC ?= $(CROSS)gcc
+LD ?= $(CROSS)gcc
+AR ?= $(CROSS)ar
+STRIP ?= $(CROSS)strip
+RANLIB ?= $(CROSS)ranlib
 HOSTCC = gcc
 MKDIR = mkdir
 
-- 
2.37.1

Re: [PATCH 2/2] pm: cpupower: Makefile: better support cross-compiling

[Shuah Khan]

On 9/11/24 19:38, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>

Improve subject line. "better support cross-compiling" doesn't
tell me much. "Allow overriding cross-compiling env params" would
is a better subject line for this patch.

> 
> Allow overridding the envs, this will be easier to user Yocto

"Allow overriding the cross-comple env parameters to make it
easier for Yactor users."

Then add what it is like now and how this change helps.

spelling "overridding"?. Run checkpatch on patches before sending.

> cross-compiler toolchains to build cpupower with only two steps:
> 
> source (toolchain path)/environment-setup-armv8a-poky-linux
> make
> 

What steps do you have to take without this change? Include that
in the changelog

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   tools/power/cpupower/Makefile | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
> index 6c02f401069e..e2a48af6fa2a 100644
> --- a/tools/power/cpupower/Makefile
> +++ b/tools/power/cpupower/Makefile
> @@ -86,12 +86,12 @@ INSTALL_SCRIPT = ${INSTALL} -m 644
>   # If you are running a cross compiler, you may want to set this
>   # to something more interesting, like "arm-linux-".  If you want
>   # to compile vs uClibc, that can be done here as well.
> -CROSS = #/usr/i386-linux-uclibc/usr/bin/i386-uclibc-
> -CC = $(CROSS)gcc
> -LD = $(CROSS)gcc
> -AR = $(CROSS)ar
> -STRIP = $(CROSS)strip
> -RANLIB = $(CROSS)ranlib
> +CROSS ?= #/usr/i386-linux-uclibc/usr/bin/i386-uclibc-
> +CC ?= $(CROSS)gcc
> +LD ?= $(CROSS)gcc
> +AR ?= $(CROSS)ar
> +STRIP ?= $(CROSS)strip
> +RANLIB ?= $(CROSS)ranlib
>   HOSTCC = gcc
>   MKDIR = mkdir
>   

Send v2 with cover-letter to make easier for reviewers.

thanks,
-- Shuah

Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed

[John B. Wyatt IV]

Hello Peng,

These two seem like two separate patches and usually a series has a
cover letter. Did you mean to send them separately or is something missing?

-- 
Sincerely,
John Wyatt
Software Engineer, Core Kernel
Red Hat

RE: [PATCH 1/2] pm: cpupower: bench: print path fopen failed

[Peng Fan]

Hi John,

> Subject: Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed
> 
> Hello Peng,
> 
> These two seem like two separate patches and usually a series has a
> cover letter. Did you mean to send them separately or is something
> missing?

I think the two patches are just small patches, so send them
together and not write cover-letter for them.

I could write the cover-letter in v2 if there are any comments.

Thanks,
Peng.

> 
> --
> Sincerely,
> John Wyatt
> Software Engineer, Core Kernel
> Red Hat

Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed

[Shuah Khan]

On 9/11/24 19:38, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Print out the config file path when fopen failed. It will be easy
> for users to know where to create the file.

Send these two patches as a series with a cover letter.

Also what is changing - you can include what change: use the
same subject line in here.

The subject line can be improved to say more than fopen() failed.
Which file open failed?

The message can be informative about which file:
  about which file.

e.g: pm: cpupower: bench: print config file path when open fails

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   tools/power/cpupower/bench/parse.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
> index e63dc11fa3a5..366b20f9ddf1 100644
> --- a/tools/power/cpupower/bench/parse.c
> +++ b/tools/power/cpupower/bench/parse.c
> @@ -166,7 +166,7 @@ int prepare_config(const char *path, struct config *config)
>   	configfile = fopen(path, "r");
>   	if (configfile == NULL) {
>   		perror("fopen");
> -		fprintf(stderr, "error: unable to read configfile\n");
> +		fprintf(stderr, "error: unable to read configfile: %s\n", path);

While you are at it, fix it to use strerror() instead of calling perror()
followed by fprintf().


>   		free(config);
>   		return 1;
>   	}

thanks,
-- Shuah

RE: [PATCH 1/2] pm: cpupower: bench: print path fopen failed

[Peng Fan]

> Subject: Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed
> 
> On 9/11/24 19:38, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Print out the config file path when fopen failed. It will be easy for
> > users to know where to create the file.
> 
> Send these two patches as a series with a cover letter.
> 
> Also what is changing - you can include what change: use the same
> subject line in here.
> 
> The subject line can be improved to say more than fopen() failed.
> Which file open failed?
> 
> The message can be informative about which file:
>   about which file.
> 
> e.g: pm: cpupower: bench: print config file path when open fails
> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >   tools/power/cpupower/bench/parse.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/power/cpupower/bench/parse.c
> > b/tools/power/cpupower/bench/parse.c
> > index e63dc11fa3a5..366b20f9ddf1 100644
> > --- a/tools/power/cpupower/bench/parse.c
> > +++ b/tools/power/cpupower/bench/parse.c
> > @@ -166,7 +166,7 @@ int prepare_config(const char *path, struct
> config *config)
> >   	configfile = fopen(path, "r");
> >   	if (configfile == NULL) {
> >   		perror("fopen");
> > -		fprintf(stderr, "error: unable to read configfile\n");
> > +		fprintf(stderr, "error: unable to read configfile: %s\n",
> path);
> 
> While you are at it, fix it to use strerror() instead of calling perror()
> followed by fprintf().

Seems the usage of perror is in the whole file. Could the conversion
to sterror() be done in a separate patch?

Thanks,
Peng.

> 
> 
> >   		free(config);
> >   		return 1;
> >   	}
> 
> thanks,
> -- Shuah

Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed

[Shuah Khan]

On 9/17/24 06:37, Peng Fan wrote:
>> Subject: Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed
>>
>> On 9/11/24 19:38, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> Print out the config file path when fopen failed. It will be easy for
>>> users to know where to create the file.
>>
>> Send these two patches as a series with a cover letter.
>>
>> Also what is changing - you can include what change: use the same
>> subject line in here.
>>
>> The subject line can be improved to say more than fopen() failed.
>> Which file open failed?
>>
>> The message can be informative about which file:
>>    about which file.
>>
>> e.g: pm: cpupower: bench: print config file path when open fails
>>
>>>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>    tools/power/cpupower/bench/parse.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/power/cpupower/bench/parse.c
>>> b/tools/power/cpupower/bench/parse.c
>>> index e63dc11fa3a5..366b20f9ddf1 100644
>>> --- a/tools/power/cpupower/bench/parse.c
>>> +++ b/tools/power/cpupower/bench/parse.c
>>> @@ -166,7 +166,7 @@ int prepare_config(const char *path, struct
>> config *config)
>>>    	configfile = fopen(path, "r");
>>>    	if (configfile == NULL) {
>>>    		perror("fopen");
>>> -		fprintf(stderr, "error: unable to read configfile\n");
>>> +		fprintf(stderr, "error: unable to read configfile: %s\n",
>> path);
>>
>> While you are at it, fix it to use strerror() instead of calling perror()
>> followed by fprintf().
> 
> Seems the usage of perror is in the whole file. Could the conversion
> to sterror() be done in a separate patch?
> 

Yes. That can be a separate patch. Please send them together in a patch series.
I will pull those in after the merge window closes.

thanks,
-- Shuah