On Thu, 2015-07-23 at 12:14 +0530, Madhavan Srinivasan wrote:

On Wednesday 22 July 2015 10:26 AM, Daniel Axtens wrote:

quoted

quoted

+static void p8_nest_read_counter(struct perf_event *event)
+{
+	uint64_t *addr;
+	u64 data = 0;

You've got a u64 and a uint64_t, and then...

quoted

+
+	addr = (u64 *)event->hw.event_base;

... you cast to event_base to a u64 pointer, which you assign to a
uint64_t pointer.

quoted

+	data = __be64_to_cpu(*addr);

And now you dereference the pointer.
Could you just have:
    data = __be64_to_cpu(*event->hw.event_base);

quoted

+	local64_set(&event->hw.prev_count, data);
+}
+
+static void p8_nest_perf_event_update(struct perf_event *event)
+{
+	u64 counter_prev, counter_new, final_count;
+	uint64_t *addr;
+
+	addr = (uint64_t *)event->hw.event_base;

Here at least the cast type is the same as the type of addr, but again,
why do you need the different types, and why local variable?

Damn sorry, copy paste errors. When I added debug prints i messed
the type case in both the functions. I will make them as uint64_t.

No please use u64/u32 etc. Most code in powerpc does and I prefer them.

cheers