diff mbox series

[v2,3/7] intel-speed-select: Add check for CascadeLake-N models

Message ID 20191003121112.25870-4-prarit@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series Add CascadeLake-N Support | expand

Commit Message

Prarit Bhargava Oct. 3, 2019, 12:11 p.m. UTC 
Three CascadeLake-N models (6252N, 6230N, and 5218N) have SST-PBF support.

Return an error if the CascadeLake processor is not one of these specific
models.

v2: Add is_clx_n_platform()

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 .../x86/intel-speed-select/isst-config.c      | 44 ++++++++++++++++++-
 1 file changed, 42 insertions(+), 2 deletions(-)

Comments

srinivas pandruvada Oct. 4, 2019, 5:15 p.m. UTC | #1 
On Thu, 2019-10-03 at 08:11 -0400, Prarit Bhargava wrote:
> Three CascadeLake-N models (6252N, 6230N, and 5218N) have SST-PBF
> support.
> 
> Return an error if the CascadeLake processor is not one of these
> specific
> models.
> 
This patch sigfaults immediately on CLX.

> v2: Add is_clx_n_platform()
> 
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  .../x86/intel-speed-select/isst-config.c      | 44
> ++++++++++++++++++-
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/power/x86/intel-speed-select/isst-config.c
> b/tools/power/x86/intel-speed-select/isst-config.c
> index f4a23678416e..734a7960458c 100644
> --- a/tools/power/x86/intel-speed-select/isst-config.c
> +++ b/tools/power/x86/intel-speed-select/isst-config.c
> @@ -23,6 +23,7 @@ static int debug_flag;
>  static FILE *outf;
>  
>  static int cpu_model;
> +static int cpu_stepping;
>  
>  #define MAX_CPUS_IN_ONE_REQ 64
>  static short max_target_cpus;
> @@ -72,7 +73,16 @@ void debug_printf(const char *format, ...)
>  	va_end(args);
>  }
>  
> -static void update_cpu_model(void)
> +
> +int is_clx_n_platform(void)
> +{
> +	if (cpu_model == 0x55)
> +		if (cpu_stepping == 0x6 || cpu_stepping == 0x7)
> +			return 1;
> +	return 0;
> +}
> +
> +static int update_cpu_model(void)
>  {
>  	unsigned int ebx, ecx, edx;
>  	unsigned int fms, family;
> @@ -82,6 +92,34 @@ static void update_cpu_model(void)
>  	cpu_model = (fms >> 4) & 0xf;
>  	if (family == 6 || family == 0xf)
>  		cpu_model += ((fms >> 16) & 0xf) << 4;
> +
> +	cpu_stepping = fms & 0xf;
> +
> +	/* only three CascadeLake-N models are supported */
> +	if (is_clx_n_platform()) {
> +		FILE *fp;
> +		size_t n;
> +		char *line;
Need n = 0 and *line = NULL here as getline() will require if it has to
allocate.

Anyway I will update the patchset and post after test.

Thanks,
Srinivas
> +		int ret = 1;
> +
> +		fp = fopen("/proc/cpuinfo", "r");
> +		if (!fp)
> +			err(-1, "cannot open /proc/cpuinfo\n");
> +
> +		while (getline(&line, &n, fp) > 0) {
> +			if (strstr(line, "model name")) {
> +				if (strstr(line, "6252N") ||
> +				    strstr(line, "6230N") ||
> +				    strstr(line, "5218N"))
> +					ret = 0;
> +				break;
> +			}
> +		}
> +		free(line);
> +		fclose(fp);
> +		return ret;
> +	}
> +	return 0;
>  }
>  
>  /* Open a file, and exit on failure */
> @@ -1889,7 +1927,9 @@ static void cmdline(int argc, char **argv)
>  		fprintf(stderr, "Feature name and|or command not
> specified\n");
>  		exit(0);
>  	}
> -	update_cpu_model();
> +	ret = update_cpu_model();
> +	if (ret)
> +		err(-1, "Invalid CPU model (%d)\n", cpu_model);
>  	printf("Intel(R) Speed Select Technology\n");
>  	printf("Executing on CPU model:%d[0x%x]\n", cpu_model,
> cpu_model);
>  	set_max_cpu_num();
Prarit Bhargava Oct. 4, 2019, 5:18 p.m. UTC | #2 
On 10/4/19 1:15 PM, Srinivas Pandruvada wrote:
> On Thu, 2019-10-03 at 08:11 -0400, Prarit Bhargava wrote:
>> Three CascadeLake-N models (6252N, 6230N, and 5218N) have SST-PBF
>> support.
>>
>> Return an error if the CascadeLake processor is not one of these
>> specific
>> models.
>>
> This patch sigfaults immediately on CLX.>
>> v2: Add is_clx_n_platform()
>>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> ---
>>  .../x86/intel-speed-select/isst-config.c      | 44
>> ++++++++++++++++++-
>>  1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/power/x86/intel-speed-select/isst-config.c
>> b/tools/power/x86/intel-speed-select/isst-config.c
>> index f4a23678416e..734a7960458c 100644
>> --- a/tools/power/x86/intel-speed-select/isst-config.c
>> +++ b/tools/power/x86/intel-speed-select/isst-config.c
>> @@ -23,6 +23,7 @@ static int debug_flag;
>>  static FILE *outf;
>>  
>>  static int cpu_model;
>> +static int cpu_stepping;
>>  
>>  #define MAX_CPUS_IN_ONE_REQ 64
>>  static short max_target_cpus;
>> @@ -72,7 +73,16 @@ void debug_printf(const char *format, ...)
>>  	va_end(args);
>>  }
>>  
>> -static void update_cpu_model(void)
>> +
>> +int is_clx_n_platform(void)
>> +{
>> +	if (cpu_model == 0x55)
>> +		if (cpu_stepping == 0x6 || cpu_stepping == 0x7)
>> +			return 1;
>> +	return 0;
>> +}
>> +
>> +static int update_cpu_model(void)
>>  {
>>  	unsigned int ebx, ecx, edx;
>>  	unsigned int fms, family;
>> @@ -82,6 +92,34 @@ static void update_cpu_model(void)
>>  	cpu_model = (fms >> 4) & 0xf;
>>  	if (family == 6 || family == 0xf)
>>  		cpu_model += ((fms >> 16) & 0xf) << 4;
>> +
>> +	cpu_stepping = fms & 0xf;
>> +
>> +	/* only three CascadeLake-N models are supported */
>> +	if (is_clx_n_platform()) {
>> +		FILE *fp;
>> +		size_t n;
>> +		char *line;
> Need n = 0 and *line = NULL here as getline() will require if it has to
> allocate.

Odd, I ran multiple tests and never hit the segfault :(.  Thanks for fixing.

P.

> 
> Anyway I will update the patchset and post after test.
> 

> Thanks,
> Srinivas
>> +		int ret = 1;
>> +
>> +		fp = fopen("/proc/cpuinfo", "r");
>> +		if (!fp)
>> +			err(-1, "cannot open /proc/cpuinfo\n");
>> +
>> +		while (getline(&line, &n, fp) > 0) {
>> +			if (strstr(line, "model name")) {
>> +				if (strstr(line, "6252N") ||
>> +				    strstr(line, "6230N") ||
>> +				    strstr(line, "5218N"))
>> +					ret = 0;
>> +				break;
>> +			}
>> +		}
>> +		free(line);
>> +		fclose(fp);
>> +		return ret;
>> +	}
>> +	return 0;
>>  }
>>  
>>  /* Open a file, and exit on failure */
>> @@ -1889,7 +1927,9 @@ static void cmdline(int argc, char **argv)
>>  		fprintf(stderr, "Feature name and|or command not
>> specified\n");
>>  		exit(0);
>>  	}
>> -	update_cpu_model();
>> +	ret = update_cpu_model();
>> +	if (ret)
>> +		err(-1, "Invalid CPU model (%d)\n", cpu_model);
>>  	printf("Intel(R) Speed Select Technology\n");
>>  	printf("Executing on CPU model:%d[0x%x]\n", cpu_model,
>> cpu_model);
>>  	set_max_cpu_num();
>
Andy Shevchenko Oct. 7, 2019, 10:03 a.m. UTC | #3 
On Fri, Oct 04, 2019 at 10:15:21AM -0700, Srinivas Pandruvada wrote:
> On Thu, 2019-10-03 at 08:11 -0400, Prarit Bhargava wrote:

> > +	/* only three CascadeLake-N models are supported */
> > +	if (is_clx_n_platform()) {
> > +		FILE *fp;
> > +		size_t n;
> > +		char *line;
> Need n = 0 and *line = NULL here as getline() will require if it has to
> allocate.

Good catch and thus...

> > +		int ret = 1;
> > +
> > +		fp = fopen("/proc/cpuinfo", "r");
> > +		if (!fp)
> > +			err(-1, "cannot open /proc/cpuinfo\n");
> > +
> > +		while (getline(&line, &n, fp) > 0) {

> > +			if (strstr(line, "model name")) {
> > +				if (strstr(line, "6252N") ||
> > +				    strstr(line, "6230N") ||
> > +				    strstr(line, "5218N"))
> > +					ret = 0;
> > +				break;
> > +			}

Missed free(line) here.

> > +		}
> > +		free(line);
> > +		fclose(fp);
> > +		return ret;
> > +	}
> > +	return 0;
> >  }
> >  
> >  /* Open a file, and exit on failure */
> > @@ -1889,7 +1927,9 @@ static void cmdline(int argc, char **argv)
> >  		fprintf(stderr, "Feature name and|or command not
> > specified\n");
> >  		exit(0);
> >  	}
> > -	update_cpu_model();
> > +	ret = update_cpu_model();
> > +	if (ret)
> > +		err(-1, "Invalid CPU model (%d)\n", cpu_model);
> >  	printf("Intel(R) Speed Select Technology\n");
> >  	printf("Executing on CPU model:%d[0x%x]\n", cpu_model,
> > cpu_model);
> >  	set_max_cpu_num();
>
srinivas pandruvada Oct. 7, 2019, 7:18 p.m. UTC | #4 
On Mon, 2019-10-07 at 13:03 +0300, Andy Shevchenko wrote:
> On Fri, Oct 04, 2019 at 10:15:21AM -0700, Srinivas Pandruvada wrote:
> > On Thu, 2019-10-03 at 08:11 -0400, Prarit Bhargava wrote:
> > > +	/* only three CascadeLake-N models are supported */
> > > +	if (is_clx_n_platform()) {
> > > +		FILE *fp;
> > > +		size_t n;
> > > +		char *line;
> > 
> > Need n = 0 and *line = NULL here as getline() will require if it
> > has to
> > allocate.
> 
> Good catch and thus...
> 
> > > +		int ret = 1;
> > > +
> > > +		fp = fopen("/proc/cpuinfo", "r");
> > > +		if (!fp)
> > > +			err(-1, "cannot open /proc/cpuinfo\n");
> > > +
> > > +		while (getline(&line, &n, fp) > 0) {
> > > +			if (strstr(line, "model name")) {
> > > +				if (strstr(line, "6252N") ||
> > > +				    strstr(line, "6230N") ||
> > > +				    strstr(line, "5218N"))
> > > +					ret = 0;
> > > +				break;
> > > +			}
> 
> Missed free(line) here.
This may not be required. After the first call geline() allocated a
buffer and will reuse it during next call. If it is not enough it will
realloc even if the buffer is passed by user via malloc().

From man page:

"
If *lineptr is set to NULL and *n is set 0 before the call, then
       getline() will allocate a buffer for storing the line.  This
buffer
       should be freed by the user program even if getline() failed.

       Alternatively, before calling getline(), *lineptr can contain a
       pointer to a malloc(3)-allocated buffer *n bytes in size.  If
the
       buffer is not large enough to hold the line, getline() resizes
it
       with realloc(3), updating *lineptr and *n as necessary.

       In either case, on a successful call, *lineptr and *n will be
updated
       to reflect the buffer address and allocated size respectively.
"

> 
> > > +		}
> > > +		free(line);
> > > +		fclose(fp);
> > > +		return ret;
> > > +	}
> > > +	return 0;
> > >  }
> > >  
> > >  /* Open a file, and exit on failure */
> > > @@ -1889,7 +1927,9 @@ static void cmdline(int argc, char **argv)
> > >  		fprintf(stderr, "Feature name and|or command not
> > > specified\n");
> > >  		exit(0);
> > >  	}
> > > -	update_cpu_model();
> > > +	ret = update_cpu_model();
> > > +	if (ret)
> > > +		err(-1, "Invalid CPU model (%d)\n", cpu_model);
> > >  	printf("Intel(R) Speed Select Technology\n");
> > >  	printf("Executing on CPU model:%d[0x%x]\n", cpu_model,
> > > cpu_model);
> > >  	set_max_cpu_num();
> 
>
diff mbox series

Patch

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index f4a23678416e..734a7960458c 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -23,6 +23,7 @@  static int debug_flag;
 static FILE *outf;
 
 static int cpu_model;
+static int cpu_stepping;
 
 #define MAX_CPUS_IN_ONE_REQ 64
 static short max_target_cpus;
@@ -72,7 +73,16 @@  void debug_printf(const char *format, ...)
 	va_end(args);
 }
 
-static void update_cpu_model(void)
+
+int is_clx_n_platform(void)
+{
+	if (cpu_model == 0x55)
+		if (cpu_stepping == 0x6 || cpu_stepping == 0x7)
+			return 1;
+	return 0;
+}
+
+static int update_cpu_model(void)
 {
 	unsigned int ebx, ecx, edx;
 	unsigned int fms, family;
@@ -82,6 +92,34 @@  static void update_cpu_model(void)
 	cpu_model = (fms >> 4) & 0xf;
 	if (family == 6 || family == 0xf)
 		cpu_model += ((fms >> 16) & 0xf) << 4;
+
+	cpu_stepping = fms & 0xf;
+
+	/* only three CascadeLake-N models are supported */
+	if (is_clx_n_platform()) {
+		FILE *fp;
+		size_t n;
+		char *line;
+		int ret = 1;
+
+		fp = fopen("/proc/cpuinfo", "r");
+		if (!fp)
+			err(-1, "cannot open /proc/cpuinfo\n");
+
+		while (getline(&line, &n, fp) > 0) {
+			if (strstr(line, "model name")) {
+				if (strstr(line, "6252N") ||
+				    strstr(line, "6230N") ||
+				    strstr(line, "5218N"))
+					ret = 0;
+				break;
+			}
+		}
+		free(line);
+		fclose(fp);
+		return ret;
+	}
+	return 0;
 }
 
 /* Open a file, and exit on failure */
@@ -1889,7 +1927,9 @@  static void cmdline(int argc, char **argv)
 		fprintf(stderr, "Feature name and|or command not specified\n");
 		exit(0);
 	}
-	update_cpu_model();
+	ret = update_cpu_model();
+	if (ret)
+		err(-1, "Invalid CPU model (%d)\n", cpu_model);
 	printf("Intel(R) Speed Select Technology\n");
 	printf("Executing on CPU model:%d[0x%x]\n", cpu_model, cpu_model);
 	set_max_cpu_num();