Received-SPF: pass (google.com: domain of linux-kernel+bounces-71204-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 19 Feb 2024 11:29:38 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Samuel Holland <samuel.holland@sifive.com>
CC: Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, "Eric
 Lin" <eric.lin@sifive.com>, Conor Dooley <conor@kernel.org>, Palmer Dabbelt
	<palmer@dabbelt.com>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Paul Walmsley <paul.walmsley@sifive.com>,
	<linux-riscv@lists.infradead.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v1 2/6] drivers/perf: Add SiFive Composable Cache PMU
 driver
Message-ID: <20240219112938.00002654@Huawei.com>
In-Reply-To: <20240216000837.1868917-3-samuel.holland@sifive.com>
References: <20240216000837.1868917-1-samuel.holland@sifive.com>
	<20240216000837.1868917-3-samuel.holland@sifive.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Thu, 15 Feb 2024 16:08:14 -0800
Samuel Holland <samuel.holland@sifive.com> wrote:

> From: Eric Lin <eric.lin@sifive.com>
> 
> Add a driver for the PMU found in the SiFive Composable Cache
> controller. This PMU provides a configurable number of counters and a
> variety of events. Events are grouped into sets. Each counter can count
> events from only one set at a time; however, it can count any number of
> events within that set simultaneously. The PMU hardware does not provide
> an overflow interrupt or a way to atomically control groups of counters.
> 
> Some events can be filtered further by client ID (e.g. CPU or external
> DMA master). That functionality is not supported by this driver.
> 
> This driver further assumes that a single Composable Cache instance is
> shared by all CPUs in the system.
> 
> Example usage:
> 
> $ perf stat -a -e sifive_ccache_pmu/inner_acquire_block_btot/,
> 		  sifive_ccache_pmu/inner_acquire_block_hit/,
> 		  sifive_ccache_pmu/inner_acquire_block_ntob/ ls
> 
>  Performance counter stats for 'system wide':
> 
>                542      sifive_ccache_pmu/inner_acquire_block_btot/
>              22081      sifive_ccache_pmu/inner_acquire_block_hit/
>              22006      sifive_ccache_pmu/inner_acquire_block_ntob/
> 
>        0.064672432 seconds time elapsed
> 
> Example using numeric event selectors:
> 
> $ perf stat -a -e sifive_ccache_pmu/event=0x10001/,
> 		  sifive_ccache_pmu/event=0x2002/,
> 		  sifive_ccache_pmu/event=0x4001/ ls
> 
>  Performance counter stats for 'system wide':
> 
>                478      sifive_ccache_pmu/event=0x10001/
>               4717      sifive_ccache_pmu/event=0x2002/
>              44966      sifive_ccache_pmu/event=0x4001/
> 
>        0.111027326 seconds time elapsed
> 
> Signed-off-by: Eric Lin <eric.lin@sifive.com>
> Co-developed-by: Samuel Holland <samuel.holland@sifive.com>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>


Hi Samuel,

A few comments inline.

> diff --git a/drivers/perf/sifive_ccache_pmu.c b/drivers/perf/sifive_ccache_pmu.c
> new file mode 100644
> index 000000000000..8c9ef0d09f48
> --- /dev/null
> +++ b/drivers/perf/sifive_ccache_pmu.c


> +
> +#define to_ccache_pmu(p) (container_of(p, struct sifive_ccache_pmu, pmu))
> +
> +#ifndef readq
> +static inline u64 readq(void __iomem *addr)
> +{
> +	return readl(addr) | (((u64)readl(addr + 4)) << 32);
> +}
> +#endif
> +
> +#ifndef writeq
> +static inline void writeq(u64 v, void __iomem *addr)
> +{
> +	writel(lower_32_bits(v), addr);
> +	writel(upper_32_bits(v), addr + 4);

Include io-64-nonatomic-lo-hi.h 
and you shouldn't need these.

> +}
> +#endif


> +
> +/*
> + * pmu->stop: stop the counter
> + */
> +static void sifive_ccache_pmu_stop(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (hwc->state & PERF_HES_STOPPED)
> +		return;
> +
> +	/* Disable this counter to count events */
> +	writeq(0, (void *)hwc->config_base);

Not going to give address space warnings as writeq expects
__iomem?

> +	sifive_ccache_pmu_read(event);
> +
> +	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +}


> +/*
> + * pmu->del: delete the event from the PMU
Why use multi line comments?
> + */
> +static void sifive_ccache_pmu_del(struct perf_event *event, int flags)
> +{
> +	struct sifive_ccache_pmu *ccache_pmu = to_ccache_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx = hwc->idx;
> +
> +	/* Stop and release this counter */
> +	sifive_ccache_pmu_stop(event, PERF_EF_UPDATE);
> +
> +	ccache_pmu->events[idx] = NULL;
> +	clear_bit(idx, ccache_pmu->used_mask);
> +
> +	perf_event_update_userpage(event);
> +}
> +
> +/*
> + * Driver initialization

Probably drop generic code organization comments like this.
They just rot over time and provide little benefit.

> + */

..

> +
> +static int sifive_ccache_pmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sifive_ccache_pmu *ccache_pmu;
> +	u32 n_counters;
> +	int ret;
> +
> +	/* Instances without a sifive,perfmon-counters property do not contain a PMU */
> +	ret = device_property_read_u32(dev, "sifive,perfmon-counters", &n_counters);
> +	if (ret || !n_counters)
> +		return -ENODEV;
if (ret)
	return ret;

	if (!n_counters)
		return -ENODEV;

In general don't eat potentially useful return codes.

> +
> +	ccache_pmu = devm_kzalloc(dev, struct_size(ccache_pmu, events, n_counters), GFP_KERNEL);
> +	if (!ccache_pmu)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ccache_pmu);
> +
> +	ccache_pmu->pmu = (struct pmu) {
> +		.parent		= dev,
> +		.attr_groups	= sifive_ccache_pmu_attr_grps,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE | PERF_PMU_CAP_NO_INTERRUPT,
> +		.task_ctx_nr	= perf_invalid_context,
> +		.event_init	= sifive_ccache_pmu_event_init,
> +		.add		= sifive_ccache_pmu_add,
> +		.del		= sifive_ccache_pmu_del,
> +		.start		= sifive_ccache_pmu_start,
> +		.stop		= sifive_ccache_pmu_stop,
> +		.read		= sifive_ccache_pmu_read,
> +	};
> +	ccache_pmu->cpu = nr_cpu_ids;
> +	ccache_pmu->n_counters = n_counters;
> +
> +	ccache_pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ccache_pmu->base))
> +		return PTR_ERR(ccache_pmu->base);
> +
> +	sifive_ccache_pmu_hw_init(ccache_pmu);
> +
> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_SIFIVE_CCACHE_ONLINE, &ccache_pmu->node);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add CPU hotplug instance\n");

you could use devm_add_action_or_reset() and trivial callback to unwind this in remove + error
paths automatically.  Slight simplification of code, though may end up a line or two longer.

Do the same for perf_pmu_unregister() and you can get rid of remove entirely.

> +
> +	ret = perf_pmu_register(&ccache_pmu->pmu, "sifive_ccache_pmu", -1);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to register PMU\n");
> +		goto err_remove_instance;
> +	}
> +
> +	return 0;
> +
> +err_remove_instance:
> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_SIFIVE_CCACHE_ONLINE, &ccache_pmu->node);
> +
> +	return ret;
> +}
> +
> +static void sifive_ccache_pmu_remove(struct platform_device *pdev)
> +{
> +	struct sifive_ccache_pmu *ccache_pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&ccache_pmu->pmu);
> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_SIFIVE_CCACHE_ONLINE, &ccache_pmu->node);
> +}
> +
> +static const struct of_device_id sifive_ccache_pmu_of_match[] = {
> +	{ .compatible = "sifive,ccache0" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sifive_ccache_pmu_of_match);
> +
> +static struct platform_driver sifive_ccache_pmu_driver = {
> +	.probe	= sifive_ccache_pmu_probe,
> +	.remove_new	= sifive_ccache_pmu_remove,

Is this actually aligning anything in a useful fashion?
I'd just use a single space instead and not bother. The alignment tends to
just end up broken and provides little readability advantage.


> +	.driver	= {
> +		.name		= "sifive_ccache_pmu",
> +		.of_match_table	= sifive_ccache_pmu_of_match,
> +	},
> +};

> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 172d0a743e5d..be6361fdc8ba 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -230,6 +230,7 @@ enum cpuhp_state {
>  	CPUHP_AP_PERF_POWERPC_TRACE_IMC_ONLINE,
>  	CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE,
>  	CPUHP_AP_PERF_POWERPC_HV_GPCI_ONLINE,
> +	CPUHP_AP_PERF_RISCV_SIFIVE_CCACHE_ONLINE,

Not sure, but can you get away with CPUHP_AP_ONLINE_DYN ?
Nicer to avoid adding more entries to this list if that is suitable here.

>  	CPUHP_AP_PERF_CSKY_ONLINE,
>  	CPUHP_AP_WATCHDOG_ONLINE,
>  	CPUHP_AP_WORKQUEUE_ONLINE,