On Thu, Nov 03, 2016 at 01:42:03AM -0400, Anurup M wrote:
> + do {
> + /* Get count from individual L3C banks and sum them up */
> + for (i = 0; i < num_banks; i++) {
> + total_raw_count += hisi_read_l3c_counter(l3c_hwmod_data,
> + idx, i);
> + }
> + prev_raw_count = local64_read(&hwc->prev_count);
> +
> + /*
> + * As prev_raw_count is updated with average value of
> + * L3 cache banks, we multiply it by no of banks and
> + * compute the delta
> + */
> + delta = (total_raw_count - (prev_raw_count * num_banks)) &
> + HISI_MAX_PERIOD;
> +
> + local64_add(delta, &event->count);
> +
> + /*
> + * Divide by num of banks to get average count and
> + * update prev_count with this value
> + */
> + avg_raw_count = total_raw_count / num_banks;
> + } while (local64_cmpxchg(
> + &hwc->prev_count, prev_raw_count, avg_raw_count) !=
> + prev_raw_count);
Please don't aggregate like this; expose separate PMUs instead.
This is racy, and by averaging and multiplying we're making up and/or
throwing away data.
[...]
> + event_value = (val -
> + HISI_HWEVENT_L3C_READ_ALLOCATE);
> +
> + /* Select the appropriate Event select register */
> + if (idx > 3)
> + reg_offset += 4;
> +
> + /* Value to write to event type register */
> + val = event_value << (8 * idx);
> +
Please add helpers for these, and explain *why* the transformations are
necessary.
> + /* Find the djtag Identifier of the Unit */
> + client = l3c_hwmod_data->client;
> +
> + /*
> + * Set the event in L3C_EVENT_TYPEx Register
> + * for all L3C banks
> + */
As above, it seems like you should expose a separate PMU per bank
instead. That applies for all the other instances where you iterate over
banks.
[...]
> + for (i = 0; i < l3c_hwmod_data->l3c_hwcfg.num_banks; i++) {
> + module_id = l3c_hwmod_data->l3c_hwcfg.module_id[i];
> + cfg_en = l3c_hwmod_data->l3c_hwcfg.bank_cfgen[i];
> + ret = hisi_djtag_writereg(module_id,
> + cfg_en,
> + reg_offset,
> + value,
> + client);
> + if (!ret)
> + ret = value;
> + }
This is impossible to read. Please factor this into helpers such that
you don't need this amount of indentation.
Please do similarly elsewhere when you see this indentation pattern.
[...]
> +static int hisi_l3c_get_event_idx(struct hisi_pmu *pl3c_pmu)
> +{
> + struct hisi_l3c_data *l3c_hwmod_data = pl3c_pmu->hwmod_data;
> + int event_idx;
> +
> + event_idx =
> + find_first_zero_bit(
> + l3c_hwmod_data->hisi_l3c_event_used_mask,
> + pl3c_pmu->num_counters);
> +
> + if (event_idx == HISI_MAX_CFG_L3C_CNTR)
> + return -EAGAIN;
> +
> + __set_bit(event_idx,
> + l3c_hwmod_data->hisi_l3c_event_used_mask);
> +
> + return event_idx;
> +}
Please get rid of the weird hungarian notation (i.e. don't use 'p' as a
prefix for pointers), and use temporary variables consistently, e.g.
static int hisi_l3c_get_event_idx(struct hisi_pmu *l3c_pmu)
{
struct hisi_l3c_data *l3c_hwmod_data = l3c_pmu->hwmod_data;
unsigned long *used_mask = l3c_hwmod_data->hisi_l3c_event_used_mask;
int num_counters = pl3c_pmu->num_counters
int idx;
idx = find_first_zero_bit(used_mask, num_counters);
if (idx == num_counters)
return -EAGAIN;
set_bit(idx, used_mask);
return idx;
}
[...]
> + if (of_property_read_u32(node, "counter-reg",
> + &pl3c_hwcfg->counter_reg0_off)) {
> + dev_err(dev, "DT:Couldnot read counter-reg!\n");
> + return -EINVAL;
> + }
Please use spaces in these messages.
Otherwise, my comments on the binding apply here.
[...]
> +static int init_hisi_l3c_data(struct device *dev,
> + struct hisi_pmu *pl3c_pmu,
> + struct hisi_djtag_client *client)
> +{
> + struct hisi_l3c_data *l3c_hwmod_data = NULL;
> + int ret;
> +
> + l3c_hwmod_data = kzalloc(sizeof(struct hisi_l3c_data),
> + GFP_KERNEL);
Use:
l3c_hwmod_data = kzalloc(sizeof(*l3c_hwmod_data, GFP_KERNEL):
[...]
> +static int hisi_pmu_l3c_dev_probe(struct hisi_djtag_client *client)
> +{
> + struct hisi_pmu *pl3c_pmu = NULL;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + pl3c_pmu = hisi_pmu_alloc(dev);
> + if (IS_ERR(pl3c_pmu))
> + return PTR_ERR(pl3c_pmu);
Why use error pointers for this?
hisi_pmu_alloc() only ever returns ERR_PTR(-ENOMEM) if it failed to
allocate.
It's far simpler to have it pass on NULL there, and here do:
pl3c_pmu = hisi_pmu_alloc(dev);
if (!pl3c_pmu)
return -ENOMEM;
Please also s/pl3c_pmu/l3c_pmu/ here, and elsewhere throughout the
driver. The 'p' only serves to make this harder to read.
[...]
> + /* Register with perf PMU */
> + pl3c_pmu->pmu = (struct pmu) {
> + .name = pl3c_pmu->name,
> + .task_ctx_nr = perf_invalid_context,
> + .event_init = hisi_uncore_pmu_event_init,
> + .add = hisi_uncore_pmu_add,
> + .del = hisi_uncore_pmu_del,
> + .start = hisi_uncore_pmu_start,
> + .stop = hisi_uncore_pmu_stop,
> + .read = hisi_uncore_pmu_read,
> + };
Please remove the comment above this.
[...]
> +int hisi_uncore_pmu_event_init(struct perf_event *event)
> +{
> + int err;
> + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
This is undefined behaviour. This must be done *after* we check the
event->pmu->type.
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + /* we do not support sampling as the counters are all
> + * shared by all CPU cores in a CPU die(SCCL). Also we
> + * donot support attach to a task(per-process mode)
> + */
> + if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> + return -EOPNOTSUPP;
> +
> + /* counters do not have these bits */
> + if (event->attr.exclude_user ||
> + event->attr.exclude_kernel ||
> + event->attr.exclude_host ||
> + event->attr.exclude_guest ||
> + event->attr.exclude_hv ||
> + event->attr.exclude_idle)
> + return -EINVAL;
> +
> + if (event->cpu < 0)
> + return -EINVAL;
> +
> + event->cpu = cpumask_first(&phisi_pmu->cpu);
You should also check the event grouping.
Take a look at what we do in arch/arm/mm/cache-l2x0-pmu.c.
[...]
> +/*
> + * Enable counter and set the counter to count
> + * the event that we're interested in.
> + */
> +void hisi_uncore_pmu_enable_event(struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> +
> + /* Disable the hardware event counting */
> + if (phisi_pmu->ops->disable_counter)
> + phisi_pmu->ops->disable_counter(phisi_pmu, GET_CNTR_IDX(hwc));
Why isn't the counter already disabled?
> + /*
> + * Set event (if destined for Hisilicon SoC counters).
> + */
> + if (phisi_pmu->ops->set_evtype)
> + phisi_pmu->ops->set_evtype(phisi_pmu, GET_CNTR_IDX(hwc),
> + hwc->config_base);
Why isn't this done in the pmu::event_add callback?
> +
> + /* Enable the hardware event counting */
> + if (phisi_pmu->ops->enable_counter)
> + phisi_pmu->ops->enable_counter(phisi_pmu, GET_CNTR_IDX(hwc));
This should be the only necessary part of this function.
> +}
> +
> +void hisi_pmu_set_event_period(struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> +
> + /*
> + * The Hisilicon PMU counters have a period of 2^32. To account for the
> + * possiblity of extreme interrupt latency we program for a period of
> + * half that. Hopefully we can handle the interrupt before another 2^31
> + * events occur and the counter overtakes its previous value.
> + */
> + u64 val = 1ULL << 31;
> +
> + local64_set(&hwc->prev_count, val);
> +
> + /* Write to the hardware event counter */
> + phisi_pmu->ops->write_counter(phisi_pmu, hwc, val);
> +}
> +
> +void hisi_uncore_pmu_start(struct perf_event *event, int flags)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> + struct hisi_pmu_hw_events *hw_events;
> +
> + hw_events = &phisi_pmu->hw_events;
> +
> + if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> + return;
> +
> + WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
> + hwc->state = 0;
> +
> + if (phisi_pmu->ops->set_event_period)
> + phisi_pmu->ops->set_event_period(event);
When will this differ from hisi_pmu_set_event_period() above?
> + if (flags & PERF_EF_RELOAD) {
> + u64 prev_raw_count = local64_read(&hwc->prev_count);
> +
> + phisi_pmu->ops->write_counter(phisi_pmu, hwc,
> + (u32)prev_raw_count);
> + }
If we always go through hisi_pmu_set_event_period(), this looks
redundant.
> +
> + hisi_uncore_pmu_enable_event(event);
There's no matching disable_event() call in this function, so this looks
suspicious.
> + perf_event_update_userpage(event);
> +}
> +
> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> +
> + if (hwc->state & PERF_HES_UPTODATE)
> + return;
Why?
[...]
> +int hisi_uncore_common_fwprop_read(struct device *dev,
> + struct hisi_pmu *phisi_pmu)
> +{
> + if (device_property_read_u32(dev, "num-events",
> + &phisi_pmu->num_events)) {
> + dev_err(dev, "Cant read num-events from DT!\n");
> + return -EINVAL;
> + }
For consistency with the rest of the driver, and given there is no ACPI
support, please use the of_property_* API here.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
