Re: [PATCH v2 06/10] hwmon: (fam15h_power) Add ptsc counter value for accumulated power

From: Guenter Roeck <linux@roeck-us.net>
To: "Huang Rui" <ray.huang@amd.com>, "Borislav Petkov" <bp@suse.de>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Jean Delvare" <jdelvare@suse.de>,
	"Andy Lutomirski" <luto@amacapital.net>,
	"Andreas Herrmann" <herrmann.der.user@gmail.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"Len Brown" <lenb@kernel.org>,
	"John Stultz" <john.stultz@linaro.org>,
	"Frédéric Weisbecker" <fweisbec@gmail.com>
Cc: lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org,
	x86@kernel.org,
	Andreas Herrmann <herrmann.der.user@googlemail.com>,
	Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>,
	Borislav Petkov <bp@alien8.de>,
	Fengguang Wu <fengguang.wu@intel.com>,
	Aaron Lu <aaron.lu@intel.com>, Tony Li <tony.li@amd.com>
Subject: Re: [PATCH v2 06/10] hwmon: (fam15h_power) Add ptsc counter value for accumulated power
Date: Fri, 23 Oct 2015 06:59:19 -0700	[thread overview]
Message-ID: <562A3D37.9080400@roeck-us.net> (raw)
In-Reply-To: <1445308109-17970-7-git-send-email-ray.huang@amd.com>

On 10/19/2015 07:28 PM, Huang Rui wrote:
> PTSC is the performance timestamp counter value in a cpu core and the
> cores in one compute unit have the fixed frequency. So it picks up the
> performance timestamp counter value of the first core per compute unit
> to measure the interval for average power per compute unit.
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>   arch/x86/include/asm/msr-index.h | 1 +
>   drivers/hwmon/fam15h_power.c     | 5 +++++
>   2 files changed, 6 insertions(+)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index c1c0a1c..3686eaa 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -313,6 +313,7 @@
>   #define MSR_F15H_PERF_CTR		0xc0010201
>   #define MSR_F15H_NB_PERF_CTL		0xc0010240
>   #define MSR_F15H_NB_PERF_CTR		0xc0010241
> +#define MSR_F15H_PTSC			0xc0010280
>
>   /* Fam 10h MSRs */
>   #define MSR_FAM10H_MMIO_CONF_BASE	0xc0010058
> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> index 88e4f3e..6321f73 100644
> --- a/drivers/hwmon/fam15h_power.c
> +++ b/drivers/hwmon/fam15h_power.c
> @@ -62,6 +62,8 @@ struct fam15h_power_data {
>   	u64 max_cu_acc_power;
>   	/* accumulated power of the compute units */
>   	u64 cu_acc_power[MAX_CUS];
> +	/* performance timestamp counter */
> +	u64 cpu_sw_pwr_ptsc[MAX_CUS];
>   };
>
>   static ssize_t show_power(struct device *dev,
> @@ -132,6 +134,9 @@ static void do_read_registers_on_cu(void *_data)
>
>   	WARN_ON(rdmsrl_safe(MSR_F15H_CU_PWR_ACCUMULATOR,
>   			    &data->cu_acc_power[cu]));
> +
> +	WARN_ON(rdmsrl_safe(MSR_F15H_PTSC,
> +			    &data->cpu_sw_pwr_ptsc[cu]));
>   }

I am not really happy with those WARN_ON, or even an error message.
If the error is seen, it may be persistent.

If an error check is really needed here, it might make more sense to store
the read error and return it to user space if the respective sysfs attribute
is read.

Guenter

>
>   static int read_registers(struct fam15h_power_data *data)
>