* Huang Rui <ray.hu...@amd.com> wrote:
> +/*
> + * Acc power status counters
> + */
> +#define AMD_POWER_PKG_ID 0
> +#define AMD_POWER_EVENTSEL_PKG 1
> +/*
> + * the ratio of compute unit power accumulator sample period to the
> + * PTSC period
> + */
> +/*
> + * Accumulated power is to measure the sum of each compute unit's
> + * power consumption. So it picks only one core from each compute unit
> + * to get the power with MSR_F15H_CU_PWR_ACCUMULATOR. The cpu_mask
> + * represents CPU bit map of all cores which are picked to measure the
> + * power for the compute units that they belong to.
> + */
> +static cpumask_t cpu_mask;
> + /*
> + * calculate the power comsumption for each compute unit over
> + * a time period, the unit of final value (delta) is
> + * micro-Watts. Then add it into event count.
> + */
Please capitalize sentences consistently - half of the comments you added start
lower-case.
> +static void __pmu_event_start(struct power_pmu *pmu,
> + struct perf_event *event)
So this looks better either on a single line, or as:
static void
__pmu_event_start(struct power_pmu *pmu, struct perf_event *event)
Breaking the argument list combines the worst of the two worlds.
> + if ((mode & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
> + /*
> + * Drain the remaining delta count out of a event
> + * that we are disabling:
s/an event
(Please re-read all the comments - I saw a few other typos and spelling
mistakes
as well.)
> + if (cfg == AMD_POWER_EVENTSEL_PKG)
> + bit = AMD_POWER_PKG_ID;
> + else
> + return -EINVAL;
> +
> + event->hw.event_base = MSR_F15H_CU_PWR_ACCUMULATOR;
> + event->hw.config = cfg;
> + event->hw.idx = bit;
> +
> + return ret;
so this control flow looks pretty weird. Why not:
> + if (cfg != AMD_POWER_EVENTSEL_PKG)
> + return -EINVAL;
> +
> + event->hw.event_base = MSR_F15H_CU_PWR_ACCUMULATOR;
> + event->hw.config = cfg;
> + event->hw.idx = AMD_POWER_PKG_ID;
> +
> + return ret;
?
> +static struct attribute_group pmu_attr_group = {
> + .attrs = pmu_attrs,
> +};
> +
> +
> +/*
> + * at current, it only supports to report the power of each
s/currently it only supports power reporting of each
> + * processor/package
> + */
> +EVENT_ATTR_STR(power-pkg, power_pkg, "event=0x01");
> +
> +EVENT_ATTR_STR(power-pkg.unit, power_pkg_unit, "mWatts");
> +
> +/* convert the count from micro-Watts to milli-Watts */
> +EVENT_ATTR_STR(power-pkg.scale, power_pkg_scale, "1.000000e-3");
> +
> +
> +static struct attribute *events_attr[] = {
> + EVENT_PTR(power_pkg),
> + EVENT_PTR(power_pkg_unit),
> + EVENT_PTR(power_pkg_scale),
> + NULL,
> +};
> +
> +static struct attribute_group pmu_events_group = {
> + .name = "events",
> + .attrs = events_attr,
> +static struct attribute_group pmu_format_group = {
> + .name = "format",
> + .attrs = formats_attr,
> +};
Please initialize structures in a vertically aligned manner, like you did it
later
on:
> +static struct pmu pmu_class = {
> + .attr_groups = attr_groups,
> + .task_ctx_nr = perf_invalid_context, /* system-wide only */
> + .event_init = pmu_event_init,
> + .add = pmu_event_add, /* must have */
> + .del = pmu_event_del, /* must have */
> + .start = pmu_event_start,
> + .stop = pmu_event_stop,
> + .read = pmu_event_read,
> +};
Btw., I don't think the 'must have' comment adds any important information
needed
to understand this new PMU driver.
> +static int power_cpu_init(int cpu)
> +{
> + struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu);
> +
> + if (pmu)
> + return 0;
> +
> + if (!cpumask_and(pmu->mask, topology_sibling_cpumask(cpu),
> + &cpu_mask))
> + cpumask_set_cpu(cpu, &cpu_mask);
> +
> + return 0;
> +}
Hm, has this function ever been runtime tested? This function either does
nothing
(contrary to the clear intention of twiddling the cpu_mask), or crashes on a
NULL
pointer.
( Also, the code has an annoying line-break. Don't pacify checkpatch by making
the
code harder to read. )
Thanks,
Ingo
