[PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB

[Daniel Axtens]

> >>  
> >> -	if (!(ppmu->flags & PPMU_ARCH_207S)) {
> >> +	if (!(ppmu->flags & PPMU_ARCH_207S) || cpuhw->bhrb_users)
> 
> > You're using cpuhw->bhrb_users as a bool here, where it's an int. Could
> > you make the test more specific so that it's clear exactly what you're
> > expecting bhrb_users to contain?
> 
> Using cpuhw->bhrb_users as a bool just verifies whether it contains
> zero or non-zero value in it. The test seems to be doing that as
> expected. But yes, we can move it as a nested conditional block as
> well if that is better.
> 

What I meant was, should this read (cpuhw->bhrb_users != 0)? Because
bhrb_users in check_excludes() is a signed int, I also wanted to make
sure it shouldn't be a test for bhrb_users > 0 instead. (Also, if
bhrb_users is always positive, should it be an unsigned int?)

I don't think a nested conditional would be better. 



> >> -	if (check_excludes(ctrs, cflags, n, 1))
> >> +	cpuhw = &get_cpu_var(cpu_hw_events);
> > Should this be using a this_cpu_ptr rather than a get_cpu_var? (as with
> > the power_pmu_commit_txn case?)
> >> +	if (check_excludes(ctrs, cflags, n, 1, cpuhw->bhrb_users)) {
> >> +		put_cpu_var(cpu_hw_events);
> > Likewise with this?
> >>  		return -EINVAL;
> >> +	}
> >>  
> >> -	cpuhw = &get_cpu_var(cpu_hw_events);
> 
> This patch just moves the existing code couple of lines above without
> changing it in any manner.
> 
I see that, but I still think you should take this opportunity to
improve it.

Regards,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 860 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20150611/8ecf5364/attachment.sig>