Re: [PATCH V8 09/10] powerpc, perf: Enable privilege mode SW branch filters

[Daniel Axtens <dja@axtens.net>]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

> 	if (sw_filter & PERF_SAMPLE_BRANCH_PLM_ALL) {
> +		flag = false;
Would it be possible to use a more meaningful name than flag? Perhaps
indicating what is it flagging?
> +
> +		if (sw_filter & PERF_SAMPLE_BRANCH_USER) {
> +			if (to_plm == POWER_ADDR_USER)
> +				flag = true;
> +		}
> +
> +		if (sw_filter & PERF_SAMPLE_BRANCH_KERNEL) {
> +			if (to_plm == POWER_ADDR_KERNEL)
> +				flag = true;
> +		}
> +
> +		if (sw_filter & PERF_SAMPLE_BRANCH_HV) {
> +			if (cpu_has_feature(CPU_FTR_HVMODE)) {
> +				if (to_plm == POWER_ADDR_KERNEL)
> +					flag = true;
> +			}
> +		}

Is there any reason these are nested ifs rather than &&s?

> +
> +		if (!flag)
> +			return false;
> +	}
> +

> @@ -700,7 +710,6 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type, u64 *bhrb_filter)
>  			if (branch_sample_type) {
>  				/* Multiple filters will be processed in SW */
>  				pmu_bhrb_filter = 0;
> -				*bhrb_filter = 0;
>  				return pmu_bhrb_filter;
>  			} else {
>  				/* Individual filter will be processed in HW */
What's the justification for the removal of this line? You added it in
the previous patch...

Regards,
Daniel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]