linux-kernel - Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZMPYYuwg7wHGXft4@FVFF77S0Q05N>
Date:   Fri, 28 Jul 2023 16:01:54 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Gowthami Thiagarajan <gthiagarajan@...vell.com>
Cc:     will@...nel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, sgoutham@...vell.com,
        bbhushan2@...vell.com, gcherian@...vell.com, lcherian@...vell.com
Subject: Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support

Hi,

On Fri, Jun 30, 2023 at 05:33:46PM +0530, Gowthami Thiagarajan wrote:
> PCI Express Interface PMU includes various performance counters to monitor
> the data that is transmitted over the PCIe link. The counters track various
> inbound and outbound transactions which includes separate counters for
> posted/non-posted/completion TLPs. Also, inbound and outbound memory read
> requests along with their latencies can also be monitored. Address
> Translation Services(ATS)events such as ATS Translation, ATS Page Request,
> ATS Invalidation along with their corresponding latencies are also
> supported.
> 
> The performance counters are 64 bits wide.
> 
> For instance,
> perf stat -e ib_tlp_pr <workload>
> tracks the inbound posted TLPs for the workload.
> 
> Signed-off-by: Linu Cherian <lcherian@...vell.com>
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@...vell.com>

This generally looks fine; I have a few comments below.

[...]

> diff --git a/drivers/perf/marvell_pem_pmu.c b/drivers/perf/marvell_pem_pmu.c
> new file mode 100644
> index 000000000000..fb27112aa7d4
> --- /dev/null
> +++ b/drivers/perf/marvell_pem_pmu.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell PEM(PCIe RC) Performance Monitor Driver
> + *
> + * Copyright (C) 2023 Marvell.
> + */

Nit: please follow the preferred coding style for comments. This should have a
newline immediately after the '/*', e.g.

	/*
	 * Marvell PEM(PCIe RC) Performance Monitor Driver
	 *
	 * Copyright (C) 2023 Marvell.
	 */

Likewise for all other multi-line comments.
	
> +#include <linux/acpi.h>
> +#include <linux/init.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/perf_event.h>
> +
> +/* Each of these events maps to a free running 64 bit counter
> + * with no event control, but can be reset.
> + *
> + */
> +enum pem_events {
> +	IB_TLP_NPR,
> +	IB_TLP_PR,
> +	IB_TLP_CPL,

> +static u64 eventid_to_offset_table[] = {
> +	0x0,
> +	0x8,
> +	0x10,

I assume the event IDs are the values in the pem_events enum, so please use
array initalizers here to make that clear, e.g.

	static u64 eventid_to_offset_table[] = {
		[IB_TLP_NPR]	= 0x0,
		[IB_TLP_PR]	= 0x8,
		[IB_TLP_CPL]	 0x10,
		...
	};

[...]

> +static int pem_perf_event_init(struct perf_event *event)
> +{
> +	struct pem_pmu *pmu = to_pem_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (is_sampling_event(event)) {

Don't we also need to check for:

	event->attach_state & PERF_ATTACH_TASK

> +		dev_info(pmu->dev, "Sampling not supported!\n");
> +		return -EOPNOTSUPP;
> +	}

Please delete this dev_info().

> +
> +	if (event->cpu < 0) {
> +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> +		return -EOPNOTSUPP;
> +	}

Likewise, please delete this dev_warn().

> +
> +	/*  We must NOT create groups containing mixed PMUs */
> +	if (event->group_leader->pmu != event->pmu &&
> +	    !is_software_event(event->group_leader))
> +		return -EINVAL;
> +
> +	/* Set ownership of event to one CPU, same event can not be observed
> +	 * on multiple cpus at same time.
> +	 */

Please fix this comment style (or delete the comment).

> +	event->cpu = pmu->cpu;
> +	hwc->idx = -1;
> +	return 0;
> +}

Thanks,
Mark.