Re: [PATCH 03/10] coresight: etm-perf: configuring filters from perf core

[Suzuki K Poulose <Suzuki.Poulose@arm.com>]

On 18/07/16 20:51, Mathieu Poirier wrote:
> This patch implements the required API needed to access
> and retrieve range and start/stop filters from the perf core.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-etm-perf.c | 146 ++++++++++++++++++++---
>  drivers/hwtracing/coresight/coresight-etm-perf.h |  32 +++++
>  2 files changed, 162 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 78a1bc0013a2..fde7f42149c5 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -29,6 +29,7 @@
>  #include <linux/workqueue.h>
>
>  #include "coresight-priv.h"
> +#include "coresight-etm-perf.h"
>
>  static struct pmu etm_pmu;
>  static bool etm_perf_up;
> @@ -83,12 +84,44 @@ static const struct attribute_group *etm_pmu_attr_groups[] = {
>
>  static void etm_event_read(struct perf_event *event) {}
>
> +static int etm_addr_filters_alloc(struct perf_event *event)
> +{

...

> +	return 0;
> +}
> +


> +
>  static int etm_event_init(struct perf_event *event)
>  {
> +	int ret;
> +
>  	if (event->attr.type != etm_pmu.type)
>  		return -ENOENT;
>
> -	return 0;
> +	ret = etm_addr_filters_alloc(event);


>  }
>
>  static void free_event_data(struct work_struct *work)
> @@ -456,6 +489,85 @@ static void etm_free_drv_configs(struct perf_event *event)
>  	}
>  }
>
> +static int etm_addr_filters_validate(struct list_head *filters)
> +{

> +
> +	return 0;
> +}
> +
> +static void etm_addr_filters_sync(struct perf_event *event)
> +{
> +	struct perf_addr_filters_head *head = perf_event_addr_filters(event);
> +	unsigned long start, stop, *offs = event->addr_filters_offs;
> +	struct etm_filters *filters = event->hw.addr_filters;
> +	struct perf_addr_filter *filter;
> +	int i = 0;

Is it possible to delay the etm_addr_filters_alloc() until this point ?
I understand that this function cannot report back failures if we fail
to allocate memory. Or may be do a lazy allocation from addr_filters_validate(),
when we get the first filter added.

Of course this could be done as a follow up patch to improve things once
we get the initial framework in.



> +
> +	list_for_each_entry(filter, &head->list, entry) {
> +		start = filter->offset + offs[i];
> +		stop = start + filter->size;
> +
> +		if (filter->range == 1) {
> +			filters->filter[i].start_addr = start;
> +			filters->filter[i].stop_addr = stop;
> +			filters->filter[i].type = ETM_ADDR_TYPE_RANGE;
> +		} else {
> +			if (filter->filter == 1) {
> +				filters->filter[i].start_addr = start;
> +				filters->filter[i].type = ETM_ADDR_TYPE_START;
> +			} else {
> +				filters->filter[i].stop_addr = stop;
> +				filters->filter[i].type = ETM_ADDR_TYPE_STOP;
> +			}
> +		}
> +		i++;
> +	}
> +
> +	filters->nr_filters = i;
> +/**
> + * struct etm_filters - set of filters for a session
> + * @etm_filter:	All the filters for this session.
> + * @nr_filters:	Number of filters
> + * @ssstatus:	Status of the start/stop logic.
> + */
> +struct etm_filters {
> +	struct etm_filter	filter[ETM_ADDR_CMP_MAX];

nit: having the variable renamed to etm_filter will make the code a bit more readable
where we populate/validate the filters.

Otherwise looks good

Suzuki