Re: [PATCH V9 2/4] drivers/perf: imx_ddr: Add ddr performance counter support

[Will Deacon <will.deacon@arm.com>]

On Mon, Apr 29, 2019 at 04:44:32PM +0000, Frank Li wrote:
> diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> new file mode 100644
> index 0000000..087d75a
> --- /dev/null
> +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> @@ -0,0 +1,589 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2017 NXP
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/perf_event.h>
> +#include <linux/slab.h>
> +
> +#define COUNTER_CNTL		0x0
> +#define COUNTER_READ		0x20
> +
> +#define COUNTER_DPCR1		0x30
> +
> +#define CNTL_OVER		0x1
> +#define CNTL_CLEAR		0x2
> +#define CNTL_EN			0x4
> +#define CNTL_EN_MASK		0xFFFFFFFB
> +#define CNTL_CLEAR_MASK		0xFFFFFFFD
> +#define CNTL_OVER_MASK		0xFFFFFFFE
> +
> +#define CNTL_CSV_SHIFT		24
> +#define CNTL_CSV_MASK		(0xFF << CNTL_CSV_SHIFT)
> +
> +#define EVENT_CYCLES_ID		0
> +#define EVENT_CYCLES_COUNTER	0
> +#define NUM_COUNTERS		4
> +
> +#define to_ddr_pmu(p)		container_of(p, struct ddr_pmu, pmu)
> +
> +#define DDR_PERF_DEV_NAME	"ddr_perf"

This is far too generic. Please make it something like "imx8_ddr_perf_pmu".

> +static int ddr_perf_probe(struct platform_device *pdev)
> +{
> +	struct ddr_pmu *pmu;
> +	struct device_node *np;
> +	void __iomem *base;
> +	struct resource *iomem;
> +	char *name;
> +	char *hpname;
> +	int num;
> +	int ret;
> +	int irq;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, iomem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	np = pdev->dev.of_node;
> +
> +	pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> +	if (!pmu)
> +		return -ENOMEM;
> +
> +	num = ddr_perf_init(pmu, base, &pdev->dev);
> +
> +	platform_set_drvdata(pdev, pmu);
> +
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +				"perf/imx/ddr%d:online", num);
> +	if (!hpname)
> +		return -ENOMEM;
> +
> +	pmu->cpu = raw_smp_processor_id();
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> +					 ddr_perf_offline_cpu);

This doesn't seem right to me. My understanding of the cpuhp mechanism
is that you register a single multi-instance state as part of driver
initialisation, and then add an instance for each device you probe.
That means you don't need to kasprintf the callback name as you are above --
you can just use DDR_PERF_DEV_NAME instead.

Please see how other perf drivers manage this on my for-next/perf branch.

> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> +		goto ddr_perf_err;
> +	}
> +
> +	pmu->cpuhp_state = ret;
> +
> +	/* Register the pmu instance for cpu hotplug */
> +	cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +
> +	ret = perf_pmu_register(&pmu->pmu, name, -1);

Again, the string you're passing in here is too generic. I suggest taking
DDR_PERF_DEV_NAME and adding "_%d" to the end to paste in your 'num'
identifier.

Sorry if this feels like pedantry, but this gets exposed to userspace
via sysfs, so we need to be careful with the namespace.

Will