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

[Borislav Petkov <bp@suse.de>]

On Mon, Oct 26, 2015 at 11:37:23AM +0800, Huang Rui wrote:
> On Fri, Oct 23, 2015 at 06:59:19AM -0700, Guenter Roeck wrote:
> > 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(+)

...

> > >@@ -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.
> > 
> 
> I am OK with removing WARN_ON here. Boris, if you also agree with it,

The real question should be: are those MSR accesses behind a CPUID flag check
which guarantees their existence?

If so, you don't really need WARN_ONs. And the MSR accesses better be
behind a CPUID flag anyway because reading non-existent MSRs is pretty
much pure waste of energy.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--