From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753735AbcIIR7p (ORCPT ); Fri, 9 Sep 2016 13:59:45 -0400 Received: from mga11.intel.com ([192.55.52.93]:25003 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752151AbcIIR7n (ORCPT ); Fri, 9 Sep 2016 13:59:43 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,306,1470726000"; d="scan'208";a="1047971193" From: "Pan, Harry" To: "tglx@linutronix.de" CC: "linux-kernel@vger.kernel.org" , "peterz@infradead.org" , "ray.huang@amd.com" , "x86@kernel.org" , "hpa@zytor.com" , "srinivas.pandruvada@linux.intel.com" , "mingo@redhat.com" , "bp@alien8.de" Subject: Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support Thread-Topic: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support Thread-Index: AQHSCqwO70QFBqa9eUG61lxaD6zHCaBwvUoAgAAvFAA= Date: Fri, 9 Sep 2016 17:59:37 +0000 Message-ID: <1473443970.3685.20.camel@intel.com> References: <1473433267-10153-1-git-send-email-harry.pan@intel.com> <1473433267-10153-2-git-send-email-harry.pan@intel.com> In-Reply-To: Accept-Language: zh-TW, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.252.184.248] Content-Type: text/plain; charset="utf-8" Content-ID: <8D5619AD318FD3448A75BC7BED2B2BB6@intel.com> MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u89HxnuY003165 I refined/uploaded again, kindly advise. On Fri, 2016-09-09 at 17:11 +0200, Thomas Gleixner wrote: > On Fri, 9 Sep 2016, Harry Pan wrote: > > - if (apply_quirk) > > + if (apply_quirk == RAPL_HSX_QUIRK) > > rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16; > > > > /* > > + * Some Atom processors (BYT/BSW) have 2^ESU microjoules increment, > > + * refer to Software Developers' Manual, Vol. 3C, Order No. 325384, > > + * Table 35-8 of MSR_RAPL_POWER_UNIT > > + */ > > + if (apply_quirk == RAPL_BYT_QUIRK) { > > + for (i = 0; i < NR_RAPL_DOMAINS; i++) > > + rapl_hw_unit[i] = 32 - rapl_hw_unit[i]; > > + } > > switch(quirk) if at all, but see below. Yes, v3 I refined as switch. > > > + /* > > * Calculate the timer rate: > > * Use reference of 200W for scaling the timeout to avoid counter > > * overflows. 200W = 200 Joules/sec > > @@ -702,47 +742,53 @@ static int __init init_rapl_pmus(void) > > { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)&init } > > > > struct intel_rapl_init_fun { > > - bool apply_quirk; > > + enum rapl_quirk apply_quirk; > > This is silly. Make apply_quirk a function pointer and provide functions > for the different quirks. I read the rapl_check_hw_unit() as: read MSR_RAPL_POWER_UNIT, apply quirk if need, then estimate timer rate. In case to refine struct intel_rapl_init_fun adding callback, then either the quirk moving outside the rapl_check_hw_unit(), or replace input parameter as whole rapl_init in order to assess quirk callback, by far it looks to me centralize these two quirks inside this function more easily to maintain. > > > int cntr_mask; > > struct attribute **attrs; > > }; > > > > static const struct intel_rapl_init_fun snb_rapl_init __initconst = { > > - .apply_quirk = false, > > + .apply_quirk = RAPL_NO_QUIRK, > > Zero ininitalization has no real value other than consuming state space. To enable more than one quirk I extended bool to enum, I thought the __initconst space would be freed after kernel initialized, is there more detail concern I missed? Sincerely, Harry