Message-ID: <20170321165252.GA29116@leverpostej>
Date:   Tue, 21 Mar 2017 16:52:52 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Anurup M <anurupvasu@...il.com>
Cc:     will.deacon@....com, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, anurup.m@...wei.com,
        zhangshaokun@...ilicon.com, tanxiaojun@...wei.com,
        xuwei5@...ilicon.com, sanil.kumar@...ilicon.com,
        john.garry@...wei.com, gabriele.paoloni@...wei.com,
        shiju.jose@...wei.com, huangdaode@...ilicon.com,
        linuxarm@...wei.com, dikshit.n@...wei.com, shyju.pv@...wei.com
Subject: Re: [PATCH v6 07/11] drivers: perf: hisi: Add support for Hisilicon
 SoC event counters

On Fri, Mar 10, 2017 at 01:28:31AM -0500, Anurup M wrote:
> + * This code is based on the uncore PMU's like arm-cci and
> + * arm-ccn.

Nit: s/PMU's/PMUs/

[...]

> +struct hisi_l3c_hwcfg {
> +	u32 module_id;
> +	u32 bank_select;
> +	u32 bank_id;
> +};

> +/* hip05/06 chips L3C bank identifier */
> +static u32 l3c_bankid_map_v1[MAX_BANKS] = {
> +	0x02, 0x04, 0x01, 0x08,
> +};
> +
> +/* hip07 chip L3C bank identifier */
> +static u32 l3c_bankid_map_v2[MAX_BANKS] = {
> +	0x01, 0x02, 0x03, 0x04,
> +};

What exactly are these?

Why do they differ like this, and why is htis not described in the DT?

> +/* Return the L3C bank index to use in PMU name */
> +static u32 get_l3c_bank_v1(u32 module_id, u32 bank_select)
> +{
> +	u32 i;
> +
> +	/*
> +	 * For v1 chip (hip05/06) the index of bank_select
> +	 * in the bankid_map gives the bank index.
> +	 */
> +	for (i = 0 ; i < MAX_BANKS; i++)
> +		if (l3c_bankid_map_v1[i] == bank_select)
> +			break;
> +
> +	return i;
> +}
> +
> +/* Return the L3C bank index to use in PMU name */
> +static u32 get_l3c_bank_v2(u32 module_id, u32 bank_select)
> +{
> +	u32 i;
> +
> +	/*
> +	 * For v2 chip (hip07) each bank has different
> +	 * module ID. So index of module ID in the
> +	 * bankid_map gives the bank index.
> +	 */
> +	for (i = 0 ; i < MAX_BANKS; i++)
> +		if (l3c_bankid_map_v2[i] == module_id)
> +			break;
> +
> +	return i;
> +}

Can you please elaborate on the relationship between the index, its
bank, and its module?

It's not clear to me what's going on above.

[...]

> +static u32 hisi_l3c_read_counter(struct hisi_l3c_data *l3c_data, int cntr_idx)
> +{
> +	struct hisi_djtag_client *client = l3c_data->client;
> +	u32 module_id = GET_MODULE_ID(l3c_data);
> +	u32 bank_sel = GET_BANK_SEL(l3c_data);
> +	u32 reg_off, value;
> +
> +	reg_off = get_counter_reg_off(cntr_idx);
> +	hisi_djtag_readreg(module_id, bank_sel, reg_off, client, &value);

This function can fail. If it fails, it doesn't initialise value, so
that's full of junk.

It is not ok to ignore that and return junk.

[...]

> +	do {
> +		/* Get count from the L3C bank / submodule */
> +		new_raw_count += hisi_l3c_read_counter(l3c_data, idx);
> +		prev_raw_count = local64_read(&hwc->prev_count);
> +
> +		/*
> +		 *  compute the delta
> +		 */
> +		delta = (new_raw_count - prev_raw_count) & HISI_MAX_PERIOD;
> +
> +		local64_add(delta, &event->count);
> +	} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
> +				 new_raw_count) != prev_raw_count);

This is wrong. We shouldn't += the new_raw_count, and we should
accumulate the delta outside of the loop. e.g.

	do {
		new_raw_count = hisi_l3c_read_counter(l3c_data, idx);
		prev_raw_count = local64_read(&hwc->prev_count);
	} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
				 new_raw_count) != prev_raw_count);
	
	delta = (new_raw_count - prev_raw_count) & HISI_MAX_PERIOD;
	local64_add(delta, &event->count);

[...]

> +static void hisi_l3c_set_evtype(struct hisi_pmu *l3c_pmu, int idx, u32 val)
> +{
> +	struct hisi_l3c_data *l3c_data = l3c_pmu->hwmod_data;
> +	struct hisi_djtag_client *client = l3c_data->client;
> +	u32 module_id = GET_MODULE_ID(l3c_data);
> +	u32 bank_sel = GET_BANK_SEL(l3c_data);
> +	u32 reg_off = L3C_EVTYPE_REG_OFF;
> +	u32 event_value, value = 0;
> +
> +	event_value = (val - HISI_HWEVENT_L3C_READ_ALLOCATE);
> +
> +	/*
> +	 * Select the appropriate Event select register(L3C_EVENT_TYPEx).
> +	 * There are 2 Event Select registers for the 8 hardware counters.
> +	 * For the first 4 hardware counters, the L3C_EVTYPE_REG_OFF is chosen.
> +	 * For the next 4 hardware counters, the second register is chosen.
> +	 */
> +	if (idx > 3)
> +		reg_off += 4;

Please factor this logic out into a macro, e.g.

#define L3C_EVTYPE_REG(idx)	(L3C_EVTYPE_REG_OFF + (idx <= 3 ? 0 : 4))

... then you can use it above to initialise reg_off.

> +
> +	/*
> +	 * Value to write to event select register
> +	 * Each byte in the 32 bit select register is used to
> +	 * configure the event code. Each byte correspond to a
> +	 * counter register to use.
> +	 */
> +	val = event_value << (8 * idx);
> +
> +	/*
> +	 * Set the event in L3C_EVENT_TYPEx Register
> +	 * for all L3C banks
> +	 */
> +	hisi_djtag_readreg(module_id, bank_sel, reg_off, client, &value);
> +	value &= ~(0xff << (8 * idx));
> +	value |= val;
> +	hisi_djtag_writereg(module_id, bank_sel, reg_off, value, client);

This is a recurring pattern. Please factor it out.

What happens when either of these fail?

> +static void hisi_l3c_clear_evtype(struct hisi_pmu *l3c_pmu, int idx)
> +{
> +	struct hisi_l3c_data *l3c_data = l3c_pmu->hwmod_data;
> +	struct hisi_djtag_client *client = l3c_data->client;
> +	u32 module_id = GET_MODULE_ID(l3c_data);
> +	u32 bank_sel = GET_BANK_SEL(l3c_data);
> +	u32 reg_off = L3C_EVTYPE_REG_OFF;
> +	u32 value;
> +
> +	if (!hisi_l3c_counter_valid(idx)) {
> +		dev_err(l3c_pmu->dev,
> +			"Unsupported event index:%d!\n", idx);
> +		return;
> +	}
> +
> +	/*
> +	 * Clear Counting in L3C event config register.
> +	 * Select the appropriate Event select register(L3C_EVENT_TYPEx).
> +	 * There are 2 Event Select registers for the 8 hardware counters.
> +	 * For the first 4 hardware counters, the L3C_EVTYPE_REG_OFF is chosen.
> +	 * For the next 4 hardware counters, the second register is chosen.
> +	 */
> +	if (idx > 3)
> +		reg_off += 4;
> +
> +	/*
> +	 * Clear the event in L3C_EVENT_TYPEx Register
> +	 */
> +	hisi_djtag_readreg(module_id, bank_sel, reg_off, client, &value);
> +	value &= ~(0xff << (8 * idx));
> +	value |= (0xff << (8 * idx));
> +	hisi_djtag_writereg(module_id, bank_sel, reg_off, value, client);
> +}

Same comments as for hisi_l3c_set_evtype().

> +static u32 hisi_l3c_write_counter(struct hisi_pmu *l3c_pmu,
> +				  struct hw_perf_event *hwc, u32 value)
> +{
> +	struct hisi_l3c_data *l3c_data = l3c_pmu->hwmod_data;
> +	struct hisi_djtag_client *client = l3c_data->client;
> +	u32 module_id = GET_MODULE_ID(l3c_data);
> +	u32 bank_sel = GET_BANK_SEL(l3c_data);
> +	u32 reg_off;
> +	int idx = GET_CNTR_IDX(hwc);
> +	int ret;
> +
> +	if (!hisi_l3c_counter_valid(idx)) {
> +		dev_err(l3c_pmu->dev,
> +			"Unsupported event index:%d!\n", idx);
> +		return -EINVAL;
> +	}
> +
> +	reg_off = get_counter_reg_off(idx);
> +	ret = hisi_djtag_writereg(module_id, bank_sel, reg_off, value, client);
> +	if (!ret)
> +		ret = value;
> +
> +	return ret;
> +}

This does not make any sense.

Why do we return the value upon a write?

How is the caller supposed to distinguish that from an error code, given
the return type is a u32 that cannot encode a negative error code?

What happens if the access times out?

> +static void hisi_l3c_start_counters(struct hisi_pmu *l3c_pmu)
> +{
> +	struct hisi_l3c_data *l3c_data = l3c_pmu->hwmod_data;
> +	struct hisi_djtag_client *client = l3c_data->client;
> +	unsigned long *used_mask = l3c_data->event_used_mask;
> +	u32 module_id = GET_MODULE_ID(l3c_data);
> +	u32 bank_sel = GET_BANK_SEL(l3c_data);
> +	u32 num_counters = l3c_pmu->num_counters;
> +	u32 value;
> +	int enabled = bitmap_weight(used_mask, num_counters);
> +
> +	if (!enabled)
> +		return;
> +
> +	/*
> +	 * Set the event_bus_en bit in L3C AUCNTRL to start counting
> +	 * for the L3C bank
> +	 */
> +	hisi_djtag_readreg(module_id, bank_sel, L3C_EVCTRL_REG_OFF,
> +			   client, &value);
> +	value |= L3C_EVENT_EN;
> +	hisi_djtag_writereg(module_id, bank_sel, L3C_EVCTRL_REG_OFF,
> +			    value, client);
> +}

What happens if these accesses time out?

Why are we not proagating the error, or handling it somehow?

> +static void hisi_l3c_stop_counters(struct hisi_pmu *l3c_pmu)
> +{
> +	struct hisi_l3c_data *l3c_data = l3c_pmu->hwmod_data;
> +	struct hisi_djtag_client *client = l3c_data->client;
> +	u32 module_id = GET_MODULE_ID(l3c_data);
> +	u32 bank_sel = GET_BANK_SEL(l3c_data);
> +	u32 value;
> +
> +	/*
> +	 * Clear the event_bus_en bit in L3C AUCNTRL
> +	 */
> +	hisi_djtag_readreg(module_id, bank_sel, L3C_EVCTRL_REG_OFF,
> +			   client, &value);
> +	value &= ~(L3C_EVENT_EN);
> +	hisi_djtag_writereg(module_id, bank_sel, L3C_EVCTRL_REG_OFF,
> +			    value, client);
> +}

Likewise.

> +static void hisi_l3c_clear_event_idx(struct hisi_pmu *l3c_pmu, int idx)
> +{
> +	struct hisi_l3c_data *l3c_data = l3c_pmu->hwmod_data;
> +	void *bitmap_addr;
> +
> +	if (!hisi_l3c_counter_valid(idx)) {
> +		dev_err(l3c_pmu->dev, "Unsupported event index:%d!\n", idx);
> +		return;
> +	}
> +
> +	bitmap_addr = l3c_data->event_used_mask;
> +	clear_bit(idx, bitmap_addr);
> +}

Please either replace bitmap_addr with:

	unsigned long *used_mask = l3c_data->event_used_mask;

... or get rid of it entirely and do:

	clear_bit(idx, l3c_data->event_used_mask);

[...]

> +ssize_t hisi_event_sysfs_show(struct device *dev,
> +			      struct device_attribute *attr, char *page)
> +{
> +	struct perf_pmu_events_attr *pmu_attr =
> +		container_of(attr, struct perf_pmu_events_attr, attr);
> +
> +	if (pmu_attr->event_str)
> +		return sprintf(page, "%s", pmu_attr->event_str);
> +
> +	return 0;
> +}

The absence of a string sounds like a bug.

When can this happen?

[...]

> +/* djtag read interface - Call djtag driver to access SoC registers */
> +int hisi_djtag_readreg(int module_id, int bank, u32 offset,
> +		       struct hisi_djtag_client *client, u32 *value)
> +{
> +	int ret;
> +	u32 chain_id = 0;
> +
> +	while (bank != 1) {
> +		bank = (bank >> 0x1);
> +		chain_id++;
> +	}

Surely you can do this with fls or ilog2?

A comment to explain why would be helpful.

> +/* djtag write interface - Call djtag driver  to access SoC registers */
> +int hisi_djtag_writereg(int module_id, int bank, u32 offset,
> +			u32 value, struct hisi_djtag_client *client)
> +{
> +	int ret;
> +	u32 chain_id = 0;
> +
> +	while (bank != 1) {
> +		bank = (bank >> 0x1);
> +		chain_id++;
> +	}

... please factor it out into a helper, regardless.

[...]

> +static int pmu_map_event(struct perf_event *event)
> +{
> +	return (int)(event->attr.config & HISI_EVTYPE_EVENT);
> +}
> +
> +static int hisi_hw_perf_event_init(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +	struct device *dev = hisi_pmu->dev;
> +	struct perf_event *sibling;
> +	int mapping;
> +
> +	mapping = pmu_map_event(event);
> +	if (mapping < 0) {
> +		dev_err(dev, "event %x:%llx not supported\n",
> +			event->attr.type, event->attr.config);
> +		return mapping;
> +	}

> +	/* For HiSilicon SoC update config_base based on event encoding */
> +	hwc->config_base = event->attr.config;

This is obviously not correct given the above, and the lack of other
calls to pmu_map_event().

> +
> +	/*
> +	 * We must NOT create groups containing mixed PMUs, although
> +	 * software events are acceptable
> +	 */
> +	if (event->group_leader->pmu != event->pmu &&
> +	    !is_software_event(event->group_leader))
> +		return -EINVAL;
> +
> +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
> +			    group_entry)
> +		if (sibling->pmu != event->pmu && !is_software_event(sibling))
> +			return -EINVAL;

Please also check the number of counters.

[...]

> +void hisi_uncore_pmu_enable(struct pmu *pmu)
> +{
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
> +
> +	if (hisi_pmu->ops->start_counters)
> +		hisi_pmu->ops->start_counters(hisi_pmu);
> +}
> +
> +void hisi_uncore_pmu_disable(struct pmu *pmu)
> +{
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
> +
> +	if (hisi_pmu->ops->stop_counters)
> +		hisi_pmu->ops->stop_counters(hisi_pmu);
> +}

When do you not have these?

In the absence of pmu::{enable,disable}, you must disallow groups, since
their events will be enabled for different periods of time.

Thanks,
Mark.

Powered by blists - more mailing lists