Re: [PATCH 2/2] perf/imx_ddr: Dump AXI ID filter info to userspace

[Will Deacon <will@kernel.org>]

On Mon, Nov 04, 2019 at 07:09:24AM +0000, Joakim Zhang wrote:
> caps/filter indicates whether HW supports AXI ID filter or not.
> caps/enhanced_filter indicates whether HW supports enhanced AXI ID filter
> or not.
> 
> Users can check filter features from userspace with these attributions.
> 
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/perf/fsl_imx8_ddr_perf.c | 59 ++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> index 3bbf806209a6..6db484251950 100644
> --- a/drivers/perf/fsl_imx8_ddr_perf.c
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -84,6 +84,64 @@ struct ddr_pmu {
>  	int id;
>  };
>  
> +enum ddr_perf_filter_capabilities {
> +	PERF_CAP_AXI_ID_FILTER = 0,
> +	PERF_CAP_AXI_ID_FILTER_ENHANCED,
> +	PERF_CAP_AXI_ID_FEAT_MAX,
> +};
> +
> +static int ddr_perf_filter_feat_caps[PERF_CAP_AXI_ID_FEAT_MAX] = {
> +	[PERF_CAP_AXI_ID_FILTER] = DDR_CAP_AXI_ID_FILTER,
> +	[PERF_CAP_AXI_ID_FILTER_ENHANCED] = DDR_CAP_AXI_ID_FILTER_ENHANCED,
> +};
> +
> +static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap)
> +{
> +	switch (cap) {
> +	case PERF_CAP_AXI_ID_FILTER:
> +		return !!(pmu->devtype_data->quirks);
> +	case PERF_CAP_AXI_ID_FILTER_ENHANCED:
> +		return (pmu->devtype_data->quirks ==
> +			ddr_perf_filter_feat_caps[cap]);
> +	}

I think this is a bit error-prone if you add additional caps in future,
since you'll need to remember to go back and update this case. I rewrote
it as follows to try to prevent this:


-static int ddr_perf_filter_feat_caps[PERF_CAP_AXI_ID_FEAT_MAX] = {
-	[PERF_CAP_AXI_ID_FILTER] = DDR_CAP_AXI_ID_FILTER,
-	[PERF_CAP_AXI_ID_FILTER_ENHANCED] = DDR_CAP_AXI_ID_FILTER_ENHANCED,
-};
-
 static u32 ddr_perf_filter_cap_get(struct ddr_pmu *pmu, int cap)
 {
+	u32 quirks = pmu->devtype_data->quirks;
+
 	switch (cap) {
 	case PERF_CAP_AXI_ID_FILTER:
-		return !!(pmu->devtype_data->quirks);
+		return quirks & DDR_CAP_AXI_ID_FILTER;
 	case PERF_CAP_AXI_ID_FILTER_ENHANCED:
-		return (pmu->devtype_data->quirks ==
-			ddr_perf_filter_feat_caps[cap]);
+		quirks &= DDR_CAP_AXI_ID_FILTER_ENHANCED;
+		return quirks == DDR_CAP_AXI_ID_FILTER_ENHANCED;
+	default:
+		WARN(1, "unknown filter cap %d\n", cap);
 	}
 
-	WARN(1, "unknown filter cap %d\n", cap);
-
 	return 0;
 }


which also means we can drop ddr_perf_filter_feat_caps[] altogether.

> +#define PERF_EXT_ATTR_ENTRY(_name, _func, _var)				\
> +	(&((struct dev_ext_attribute[]) {				\
> +		{ __ATTR(_name, 0444, _func, NULL), (void *)_var }	\
> +	})[0].attr.attr)

In another thread, we recently realised that the array syntax is not needed
here, so I've updated this to be:


 #define PERF_EXT_ATTR_ENTRY(_name, _func, _var)				\
-	(&((struct dev_ext_attribute[]) {				\
-		{ __ATTR(_name, 0444, _func, NULL), (void *)_var }	\
-	})[0].attr.attr)
+	(&((struct dev_ext_attribute) {					\
+		__ATTR(_name, 0444, _func, NULL), (void *)_var		\
+	}).attr.attr)


I've made those two changes, so I'll queue this up for 5.5. Thanks.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel