> >>
> >> - 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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list [email protected] https://lists.ozlabs.org/listinfo/linuxppc-dev
