From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752959AbbJZEgw (ORCPT ); Mon, 26 Oct 2015 00:36:52 -0400 Received: from mx2.suse.de ([195.135.220.15]:46144 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750721AbbJZEgv (ORCPT ); Mon, 26 Oct 2015 00:36:51 -0400 Date: Mon, 26 Oct 2015 05:36:45 +0100 From: Borislav Petkov To: Huang Rui Cc: Guenter Roeck , Peter Zijlstra , Jean Delvare , Andy Lutomirski , Andreas Herrmann , Thomas Gleixner , Ingo Molnar , "Rafael J. Wysocki" , Len Brown , John Stultz , =?utf-8?B?RnLvv71k77+9cmlj?= Weisbecker , lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org, x86@kernel.org, Andreas Herrmann , Aravind Gopalakrishnan , Fengguang Wu , Aaron Lu , Tony Li Subject: Re: [PATCH v2 06/10] hwmon: (fam15h_power) Add ptsc counter value for accumulated power Message-ID: <20151026043645.GC9328@nazgul.tnic> References: <1445308109-17970-1-git-send-email-ray.huang@amd.com> <1445308109-17970-7-git-send-email-ray.huang@amd.com> <562A3D37.9080400@roeck-us.net> <20151026033722.GD8036@hr-amur2> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20151026033722.GD8036@hr-amur2> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > >Cc: Borislav Petkov > > >Cc: Guenter Roeck > > >Cc: Peter Zijlstra > > >Cc: Ingo Molnar > > >--- > > > 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) --