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

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

On Tue, Apr 30, 2019 at 11:30:17AM -0500, Zhi Li wrote:
> On Tue, Apr 30, 2019 at 5:51 AM Will Deacon <will.deacon@arm.com> wrote:
> >
> > 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.
> 
> imx8_ddr_perf_pmu is too long to use.  how about imx_ddr%d?

Too long for what? Drop the "perf" bit if you want, but I think we need
"pmu" in there.

Will