[PATCH] cpufreq/intel_pstate: Do not issue the not supported message on !Intel

[PATCH] cpufreq/intel_pstate: Do not issue the not supported message on !Intel

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Issue the CPU-not-supported message only on Intel machines as this
driver is Intel-only. Which means, the print statement can remain
KERN_INFO for ease of debugging (no need to enable it first in dynamic
debug).

While at it, correct it to say CPU "model" which is what that test does.

Fixes: 076b862c7e44 ("cpufreq: intel_pstate: Add reasons for failure and debug messages")
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Erwan Velu <e.velu@criteo.com>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
CC: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/intel_pstate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index ea62e3f02d56..19854f01e2fa 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2608,7 +2608,9 @@ static int __init intel_pstate_init(void)
 	} else {
 		id = x86_match_cpu(intel_pstate_cpu_ids);
 		if (!id) {
-			pr_info("CPU ID not supported\n");
+			if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+				pr_info("CPU model not supported\n");
+
 			return -ENODEV;
 		}
 
-- 
2.21.0

Re: [PATCH] cpufreq/intel_pstate: Do not issue the not supported message on !Intel

[Erwan Velu]

> index ea62e3f02d56..19854f01e2fa 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2608,7 +2608,9 @@ static int __init intel_pstate_init(void)
>   	} else {
>   		id = x86_match_cpu(intel_pstate_cpu_ids);
>   		if (!id) {
> -			pr_info("CPU ID not supported\n");
> +			if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +				pr_info("CPU model not supported\n");
> +
>   			return -ENODEV;
>   		}

That's a good catch but I was wondering why not putting this vendor 
condition at the initial "if (noload)" statement.

I mean, if we don't run an intel CPU there is no need of making the 
x86_match_cpu().

This commit is also killing the case of reporting an unsupported intel 
processor.


I'd suggest something like this and keep the 'CPUID not supported' part 
untouched.

     if (no_load) || (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)

         return -ENODEV

Re: [PATCH] cpufreq/intel_pstate: Do not issue the not supported message on !Intel

[Borislav Petkov]

On Mon, Apr 01, 2019 at 08:11:51AM +0000, Erwan Velu wrote:
> I'd suggest something like this and keep the 'CPUID not supported' part 
> untouched.
> 
>      if (no_load) || (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)

Yeah, that's the usual thing we do in such cases and the better idea,
I'll do that in v2.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH] cpufreq/intel_pstate: Load only on Intel hardware

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

This driver is Intel-only so loading on anything which is not Intel is
pointless. Prevent it from doing so.

While at it, correct the "not supported" print statement to say CPU
"model" which is what that test does.

Suggested-by: Erwan Velu <e.velu@criteo.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
CC: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/intel_pstate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index b599c7318aab..2986119dd31f 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2596,6 +2596,9 @@ static int __init intel_pstate_init(void)
 	const struct x86_cpu_id *id;
 	int rc;
 
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+		return -ENODEV;
+
 	if (no_load)
 		return -ENODEV;
 
@@ -2611,7 +2614,7 @@ static int __init intel_pstate_init(void)
 	} else {
 		id = x86_match_cpu(intel_pstate_cpu_ids);
 		if (!id) {
-			pr_info("CPU ID not supported\n");
+			pr_info("CPU model not supported\n");
 			return -ENODEV;
 		}
 
-- 
2.21.0


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] cpufreq/intel_pstate: Load only on Intel hardware

[Erwan Velu]

Le 01/04/2019 à 17:03, Borislav Petkov a écrit :
> From: Borislav Petkov <bp@suse.de>
>
> This driver is Intel-only so loading on anything which is not Intel is
> pointless. Prevent it from doing so.
>
> While at it, correct the "not supported" print statement to say CPU
> "model" which is what that test does.
>
Looks good to me !

Thanks !

Erwan,

Re: [PATCH] cpufreq/intel_pstate: Load only on Intel hardware

[Thomas Renninger]

On Monday, April 1, 2019 5:03:45 PM CEST Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> This driver is Intel-only so loading on anything which is not Intel is
> pointless. Prevent it from doing so.

Nice.
I wondered whether there are more of these to find by review, instead
of waiting for the next message to show up.

I ended up in the "not so straight forward" IOMMU init macros... and
continued with daily work again.

Anyway there are a lot files showing up when grepping the kernel for
intel files/drivers, maybe someone who is involved in the one or other comes
up with something similar...

       Thomas

FWIW:
Reviewed-by: Thomas Renninger <trenn@suse.de>

 
> While at it, correct the "not supported" print statement to say CPU
> "model" which is what that test does.
> 
> Suggested-by: Erwan Velu <e.velu@criteo.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> CC: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/intel_pstate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index b599c7318aab..2986119dd31f 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2596,6 +2596,9 @@ static int __init intel_pstate_init(void)
>  	const struct x86_cpu_id *id;
>  	int rc;
> 
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> +		return -ENODEV;
> +
...

Re: [PATCH] cpufreq/intel_pstate: Load only on Intel hardware

[Rafael J. Wysocki]

On Monday, April 1, 2019 5:03:45 PM CEST Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> This driver is Intel-only so loading on anything which is not Intel is
> pointless. Prevent it from doing so.
> 
> While at it, correct the "not supported" print statement to say CPU
> "model" which is what that test does.
> 
> Suggested-by: Erwan Velu <e.velu@criteo.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> CC: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/intel_pstate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index b599c7318aab..2986119dd31f 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2596,6 +2596,9 @@ static int __init intel_pstate_init(void)
>  	const struct x86_cpu_id *id;
>  	int rc;
>  
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> +		return -ENODEV;
> +
>  	if (no_load)
>  		return -ENODEV;
>  
> @@ -2611,7 +2614,7 @@ static int __init intel_pstate_init(void)
>  	} else {
>  		id = x86_match_cpu(intel_pstate_cpu_ids);
>  		if (!id) {
> -			pr_info("CPU ID not supported\n");
> +			pr_info("CPU model not supported\n");
>  			return -ENODEV;
>  		}
>  
> 

Applied, thanks!