Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752106AbbLQFwS (ORCPT ); Thu, 17 Dec 2015 00:52:18 -0500 Received: from mgwkm04.jp.fujitsu.com ([202.219.69.171]:12897 "EHLO mgwkm04.jp.fujitsu.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750766AbbLQFwR (ORCPT ); Thu, 17 Dec 2015 00:52:17 -0500 Subject: Re: [PATCH 1/3] powercap, intel_rapl, implement get_max_time_window To: Prarit Bhargava References: <1450184532-21150-1-git-send-email-prarit@redhat.com> <1450184532-21150-2-git-send-email-prarit@redhat.com> CC: , "Rafael J. Wysocki" , Radivoje Jovanovic , Mathias Krause , Ajay Thomas From: Seiichi Ikarashi Organization: Fujitsu Limited Message-ID: <56724C0A.8000101@jp.fujitsu.com> Date: Thu, 17 Dec 2015 14:45:46 +0900 User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1450184532-21150-2-git-send-email-prarit@redhat.com> Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: 7bit X-SecurityPolicyCheck-GC: OK by FENCE-Mail X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3812 Lines: 127 On 2015-12-15 22:02, Prarit Bhargava wrote: > The MSR_PKG_POWER_INFO register (Intel ASDM, section 14.9.3 > "Package RAPL Domain") provides a maximum time window which the > system can support. This window is read-only and is currently > not examined when setting the time windows for the package. I have been having a question here long time. Maximum Time Window (bits 53:48) in MSR_PKG_POWER_INFO is only 6-bit length even though Time Window for Power Limit #1 (bits 23:17) and Time Window for Power Limit #2 (bits 55:49) in MSR_PKG_POWER_LIMIT are both 7-bit length, not 6. Do Intel guys have an answer for it? The patch itself looks good to me. Just minor comments below: > diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c > index cc97f08..f765b2c 100644 > --- a/drivers/powercap/intel_rapl.c > +++ b/drivers/powercap/intel_rapl.c > @@ -493,13 +493,42 @@ static int get_current_power_limit(struct powercap_zone *power_zone, int id, > return ret; > } > > +static int get_max_time_window(struct powercap_zone *power_zone, int id, The 2nd arg "id" is not necessary. > + u64 *data) > +{ > + struct rapl_domain *rd; > + int ret = 0; > + u64 val; > + > + get_online_cpus(); > + rd = power_zone_to_rapl_domain(power_zone); > + > + if (rapl_read_data_raw(rd, MAX_TIME_WINDOW, true, &val)) rapl_read_data_raw() can return -EINVAL and -ENODEV other than -EIO. > + ret = -EIO; Is it OK to limit ret to -EIO here? > + else > + *data = val; > + > + put_online_cpus(); > + return ret; > +} > + > static int set_time_window(struct powercap_zone *power_zone, int id, > u64 window) > { > struct rapl_domain *rd; > int ret = 0; > + u64 max_window; > > get_online_cpus(); > + ret = get_max_time_window(power_zone, id, &max_window); > + if (ret < 0) > + goto out; > + > + if (window > max_window) { > + ret = -EINVAL; > + goto out; > + } > + > rd = power_zone_to_rapl_domain(power_zone); > switch (rd->rpl[id].prim_id) { > case PL1_ENABLE: > @@ -511,6 +540,7 @@ static int set_time_window(struct powercap_zone *power_zone, int id, > default: > ret = -EINVAL; > } > +out: > put_online_cpus(); > return ret; > } > @@ -590,6 +620,7 @@ static struct powercap_zone_constraint_ops constraint_ops = { > .set_time_window_us = set_time_window, > .get_time_window_us = get_time_window, > .get_max_power_uw = get_max_power, > + .get_max_time_window_us = get_max_time_window, > .get_name = get_constraint_name, > }; > > diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c > index 84419af..7d77b83 100644 > --- a/drivers/powercap/powercap_sys.c > +++ b/drivers/powercap/powercap_sys.c > @@ -101,7 +101,7 @@ static ssize_t store_constraint_##_attr(struct device *dev,\ > int err; \ > u64 value; \ > struct powercap_zone *power_zone = to_powercap_zone(dev); \ > - int id; \ > + int id, ret; \ > struct powercap_zone_constraint *pconst;\ > \ > if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id)) \ > @@ -113,8 +113,10 @@ static ssize_t store_constraint_##_attr(struct device *dev,\ > if (err) \ > return -EINVAL; \ > if (pconst && pconst->ops && pconst->ops->set_##_attr) { \ > - if (!pconst->ops->set_##_attr(power_zone, id, value)) \ > + ret = pconst->ops->set_##_attr(power_zone, id, value); \ > + if (!ret) \ > return count; \ > + return ret; \ An opposite question to above. Is it OK not to limit the return value to -EINVAL here? Do you want this function to return -EIO or something? > } \ > \ > return -ENODATA; \ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/