On 20 July 2016 at 09:34, Suzuki K Poulose <[email protected]> wrote: > On 18/07/16 20:51, Mathieu Poirier wrote: >> >> With this commit [1] address range filter information is now found >> in the struct hw_perf_event::addr_filters. As such pass the event >> itself to the coresight_source::enable/disable() functions so that >> both event attribute and filter can be accessible for configuration. >> >> [1] 'commit 375637bc5249 ("perf/core: Introduce address range filtering")' > > >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h >> index 385d62e64abb..2a5982c37dfb 100644 >> --- a/include/linux/coresight.h >> +++ b/include/linux/coresight.h >> @@ -232,8 +232,9 @@ struct coresight_ops_source { >> int (*cpu_id)(struct coresight_device *csdev); >> int (*trace_id)(struct coresight_device *csdev); >> int (*enable)(struct coresight_device *csdev, >> - struct perf_event_attr *attr, u32 mode); >> - void (*disable)(struct coresight_device *csdev); >> + struct perf_event *event, u32 mode); >> + void (*disable)(struct coresight_device *csdev, >> + struct perf_event *event); > > > nit: > > Should we make this a a bit more generic API rather than hard coding > the perf stuff in there ? i.e, > > how about : > > int (*enable)(struct coresight_device *csdev, void *data, u32 mode) > > void (*disable)(struct coresight_device *csdev, void *data, u32 mode) > > where data is specific to the mode of operation. That way the API is > cleaner and each mode could pass their own data (even though sysfs > doesn't use any at the moment).
We've been faced with the dilemma of writing code that may cater to future extensions or stick with the right code for the current situation a couple of times already. Each time we've opted for the latter, something I would be inclined to continue doing here. If need be further decoupling of the perf and sysFS access methods could be achieved by using specific enable/disable ops for each mode, i.e enable/disable_perf() and enable/disable_sysfs() but we are not there yet. Thanks, Mathieu > > Suzuki
