Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gowthami Thiagarajan <gthiagarajan@marvell.com>
To:     Mark Rutland <mark.rutland@arm.com>
CC:     "will@kernel.org" <will@kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>,
        Bharat Bhushan <bbhushan2@marvell.com>,
        George Cherian <gcherian@marvell.com>,
        Linu Cherian <lcherian@marvell.com>
Subject: RE: [EXT] Re: [PATCH 1/6] perf/marvell: Marvell PEM performance
 monitor support
Thread-Topic: [EXT] Re: [PATCH 1/6] perf/marvell: Marvell PEM performance
 monitor support
Thread-Index: AQHZq0r5gOjLcxepVkO38KSbGmvNAq/Pck4AgBbTomA=
Date:   Sat, 12 Aug 2023 03:43:07 +0000
Message-ID: <SN1PR18MB2126DFE6FCC39375D9CE8944DB11A@SN1PR18MB2126.namprd18.prod.outlook.com>
References: <20230630120351.1143773-1-gthiagarajan@marvell.com>
 <20230630120351.1143773-2-gthiagarajan@marvell.com>
 <ZMPYYuwg7wHGXft4@FVFF77S0Q05N>
In-Reply-To: <ZMPYYuwg7wHGXft4@FVFF77S0Q05N>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?+yDyDFlE0DzRjTNOn/r/vBOCTscyr1HHsqoRtGZKiKOX+hqtYydezbgK/d4M?=
 =?us-ascii?Q?JsO71FDg41AtYFmZb3wZm1+4gAOEEnbMcpuCFzn28FlNB1hM9NgWWIrRB8kn?=
 =?us-ascii?Q?Yr9MW1uX/0zlQqPEF9YgwQJpx5iZz6SKhJz4X8vRN+Jl600kTGG0j6Ts6M5B?=
 =?us-ascii?Q?oGJRxxSCrILoiZYVdfNY3d79oKbuK3uervSAhaJfEDJDe5iR1440EObH0wSO?=
 =?us-ascii?Q?l5InCcG15/00urprqmJa0ryh0DH/Lcp4yCyfBoP6fP4tpYvDrUnmLdV304lt?=
 =?us-ascii?Q?n9aGQ/de8tVVF4P3UBITY2sE/5CRh5irmHS7cGzuUjebN76as0rKed8X+drE?=
 =?us-ascii?Q?U4pLdj3aFqnom9kEntYNNsnlZE0LihWAqa7FDTe52ZwRWGZFGxLeNiiZytzY?=
 =?us-ascii?Q?q+vxrYPu2nECXi07WiHsxOajAxnb9mbGNBpTwbZYDWXytvvJ0pqz1UzuMxgU?=
 =?us-ascii?Q?+nsAkk2l9ofV7Ij9ULCFahndDzYjLd4Pa4ILAqzCLJ2B3VkMF3L1NNUuZS40?=
 =?us-ascii?Q?Biq+EwCRHMKLUn9JlOG+JbRabTIlbdfFDAR8lhn5dqiGdU2u2LwJ4hlG8tDO?=
 =?us-ascii?Q?0QRZ1ZIAl+S93ewzRE+a2oOE/AFqRFA74bIQ3t15JtdILBuQ5k12QFHrsspN?=
 =?us-ascii?Q?C2qTNG7L6cR1/KgqcWcx0ykHK4/GCDio9iBYCJyvDWs8q7MvmiFxSijVVc/X?=
 =?us-ascii?Q?X/na2se8SnkBegHiHqIz5bGi8P0A1EKX3eGYzEww4Y7L0Q0SvD3VyKelJJd/?=
 =?us-ascii?Q?kfGwZourE8LZx/N1pp/7/SKpgDeBmeaLi3IYZyHsLXQgaBxcnMqcqDAaUh2w?=
 =?us-ascii?Q?EDSJmhcBgG1WodeVQqAnutBPnHYMjmJt9cnZaKwXALuG0TTDlPPDRw1zr4xB?=
 =?us-ascii?Q?FUXRubuFKlK3/2ubv6JfU/qlflcfJGFRfN5dLAk+o7hemXXxhKZmpAWXST76?=
 =?us-ascii?Q?ZpCWCyFs1gxRyd0wUY4JRilXSeEkP/PLEMZIkz+8oVjQCgEIKYB6GszljYFW?=
 =?us-ascii?Q?70UZS0ixjp3AYHPuP1mv2g4jjCL5ClGGLPL+bVyMy7p4cGvSNHy58zBCVDE/?=
 =?us-ascii?Q?bC/KCb92oQv44Kbd3xhtNbvQNqE9w226X0Etu4/8IOswxTb8LNb5/YvDEqF8?=
 =?us-ascii?Q?lPGMScfQ7ddSQqn6UW0yHSCkLTQXk3Uef+Do1V3YK3xZnk8aUjWdMSFKHPOv?=
 =?us-ascii?Q?AoJbWB2SBAQwqwRF+RbiW/Ul8CPXFi3+aicds6DHnN9ZqLtK9BojfY/O10LZ?=
 =?us-ascii?Q?WyRrIfY7GeCoqQ9iqzAlOBIY/2+/MOieSp+QfN/xfOfCvATswJvQ5rs/ZnGr?=
 =?us-ascii?Q?7H9fDvo/QFEOb+EYo9/qnumoWG6DHar6VLBh59zPo3VO4c6RLn20jTGDrHiU?=
 =?us-ascii?Q?5RuJPwohpQHnay++28BTSKS/czbttDvnjZf6vH1XqrJGpNqLC2TA2kHzrtTr?=
 =?us-ascii?Q?iADwgd2f+gMy9DT+O8qYfUxl4VJTXbb3wTWuEJxzlftYLhBXfs2zY5Wf1o1A?=
 =?us-ascii?Q?C9bi4zIW1fiRAJPXhh5uOP1dyksaYWVncY+n5DPm5MWLdkvns2MsgFNj0KnX?=
 =?us-ascii?Q?0qQ11QY5PJSaRrWefezF30CJtnJXoLU/eu4fQfDG?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN1PR18MB2126.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 40d754c8-ef31-4138-4b0e-08db9ae63df1
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Aug 2023 03:43:07.5034
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: RHeEc2FiA93T/pCqKWftrIh6XxPPWBQearyACULYvyW7X3j4Pf5PNoaS8tJftUO5NpLRFyx8/g8uBsxuYJfHL97bd6keu/lQnjJwLQyvh2I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR18MB3616
X-Proofpoint-ORIG-GUID: VIKQdha-WeCNkEHO6IXfXeTifidOCNcW
X-Proofpoint-GUID: VIKQdha-WeCNkEHO6IXfXeTifidOCNcW
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-08-12_01,2023-08-10_01,2023-05-22_02
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Friday, July 28, 2023 8:32 PM
> To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@v=
ger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; G=
eorge Cherian
> <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> Subject: [EXT] Re: [PATCH 1/6] perf/marvell: Marvell PEM performance moni=
tor support
>=20
> External Email
>=20
> ----------------------------------------------------------------------
> Hi,
>=20
> On Fri, Jun 30, 2023 at 05:33:46PM +0530, Gowthami Thiagarajan wrote:
> > PCI Express Interface PMU includes various performance counters to moni=
tor
> > the data that is transmitted over the PCIe link. The counters track var=
ious
> > inbound and outbound transactions which includes separate counters for
> > posted/non-posted/completion TLPs. Also, inbound and outbound memory re=
ad
> > requests along with their latencies can also be monitored. Address
> > Translation Services(ATS)events such as ATS Translation, ATS Page Reque=
st,
> > 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@marvell.com>
> > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
>=20
> This generally looks fine; I have a few comments below.
>=20
> [...]
>=20
> > 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.
> > + */
>=20
> Nit: please follow the preferred coding style for comments. This should h=
ave a
> newline immediately after the '/*', e.g.
>=20
> 	/*
> 	 * Marvell PEM(PCIe RC) Performance Monitor Driver
> 	 *
> 	 * Copyright (C) 2023 Marvell.
> 	 */
>=20
> Likewise for all other multi-line comments.

Ack.
>=20
> > +#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,
>=20
> > +static u64 eventid_to_offset_table[] =3D {
> > +	0x0,
> > +	0x8,
> > +	0x10,
>=20
> I assume the event IDs are the values in the pem_events enum, so please u=
se
> array initalizers here to make that clear, e.g.
>=20
> 	static u64 eventid_to_offset_table[] =3D {
> 		[IB_TLP_NPR]	=3D 0x0,
> 		[IB_TLP_PR]	=3D 0x8,
> 		[IB_TLP_CPL]	 0x10,
> 		...
> 	};
>=20
> [...]

Ack. Yes. IDs refer to the pem_events.=20

>=20
> > +static int pem_perf_event_init(struct perf_event *event)
> > +{
> > +	struct pem_pmu *pmu =3D to_pem_pmu(event->pmu);
> > +	struct hw_perf_event *hwc =3D &event->hw;
> > +
> > +	if (event->attr.type !=3D event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (is_sampling_event(event)) {
>=20
> Don't we also need to check for:
>=20
> 	event->attach_state & PERF_ATTACH_TASK

Ack. Will add this check.

>=20
> > +		dev_info(pmu->dev, "Sampling not supported!\n");
> > +		return -EOPNOTSUPP;
> > +	}
>=20
> Please delete this dev_info().

Ack.
>=20
> > +
> > +	if (event->cpu < 0) {
> > +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> > +		return -EOPNOTSUPP;
> > +	}
>=20
> Likewise, please delete this dev_warn().
Ack.
>=20
> > +
> > +	/*  We must NOT create groups containing mixed PMUs */
> > +	if (event->group_leader->pmu !=3D 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.
> > +	 */
>=20
> Please fix this comment style (or delete the comment).
Ack.

-Thanks,
Gowthami
>=20
> > +	event->cpu =3D pmu->cpu;
> > +	hwc->idx =3D -1;
> > +	return 0;
> > +}
>=20
> Thanks,
> Mark.