Messages in this thread | | | Subject | Re: [PATCH 1/3] powercap, intel_rapl, implement get_max_time_window | From | Seiichi Ikarashi <> | Date | Thu, 17 Dec 2015 14:45:46 +0900 |
| |
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; \ >
| |