Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Eduardo Valentin]

Hello,

On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> On a system with X86_FEATURE_ARCH_PERFMON disabled
> and with a model not known by family PMU drivers,
> user gets a kernel message log like the following:
> [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> 
> The "unsupported .. CPU" part may be confusing for some
> users leading to wrong understanding that the kernel
> does not support the CPU model.
> 
> This patch rewords the messages on the failure path to:
> [ 0.667154] Performance Events: CPU does not support PMU: no PMU driver, software events only.
> 


Gentle ping on this patch, any further comments?

> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Cc: Eduardo Valentin <eduval@xxxxxxxxxx>
> Cc: Jia Zhang <qianyue.zj@xxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Eduardo Valentin <eduval@xxxxxxxxxx>
> ---
> 
> Changes from V1->V2:
> - As per initial review, the propose messaging was even
> more confusing. Simplified it by only saying that
> the CPU does not support PMU.
> 
> 
>  arch/x86/events/intel/core.c | 15 +++++++++++----
>  arch/x86/events/intel/p4.c   |  5 +----
>  arch/x86/events/intel/p6.c   |  1 -
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 035c37481f57..2ddb97f03f4a 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3889,15 +3889,22 @@ __init int intel_pmu_init(void)
>  	char *name;
>  
>  	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON)) {
> +		int ret = -ENODEV;
> +
>  		switch (boot_cpu_data.x86) {
>  		case 0x6:
> -			return p6_pmu_init();
> +			ret = p6_pmu_init();
> +			break;
>  		case 0xb:
> -			return knc_pmu_init();
> +			ret = knc_pmu_init();
> +			break;
>  		case 0xf:
> -			return p4_pmu_init();
> +			ret = p4_pmu_init();
> +			break;
>  		}
> -		return -ENODEV;
> +		if (ret)
> +			pr_cont("CPU does not support PMU: ");
> +		return ret;
>  	}
>  
>  	/*
> diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c
> index d32c0eed38ca..fb5e8576d9ac 100644
> --- a/arch/x86/events/intel/p4.c
> +++ b/arch/x86/events/intel/p4.c
> @@ -1345,11 +1345,8 @@ __init int p4_pmu_init(void)
>  	BUILD_BUG_ON(ARCH_P4_MAX_CCCR > INTEL_PMC_MAX_GENERIC);
>  
>  	rdmsr(MSR_IA32_MISC_ENABLE, low, high);
> -	if (!(low & (1 << 7))) {
> -		pr_cont("unsupported Netburst CPU model %d ",
> -			boot_cpu_data.x86_model);
> +	if (!(low & (1 << 7)))
>  		return -ENODEV;
> -	}
>  
>  	memcpy(hw_cache_event_ids, p4_hw_cache_event_ids,
>  		sizeof(hw_cache_event_ids));
> diff --git a/arch/x86/events/intel/p6.c b/arch/x86/events/intel/p6.c
> index 408879b0c0d4..e8e03e68b22f 100644
> --- a/arch/x86/events/intel/p6.c
> +++ b/arch/x86/events/intel/p6.c
> @@ -269,7 +269,6 @@ __init int p6_pmu_init(void)
>  		break;
>  
>  	default:
> -		pr_cont("unsupported p6 CPU model %d ", boot_cpu_data.x86_model);
>  		return -ENODEV;
>  	}
>  
> -- 
> 2.18.0
> 

-- 
All the best,
Eduardo Valentin