Hi Reviewers,

We still have disagreement on the naming, how do we resolve it and move forward ?

Thanks,
Besar

-----Original Message-----
From: Besar Wicaksono
Sent: Thursday, July 14, 2022 11:47 AM
To: Robin Murphy <robin.murphy@arm.com>; Mathieu Poirier
[off-list ref]
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>; catalin.marinas@arm.com;
will@kernel.org; mark.rutland@arm.com; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
tegra@vger.kernel.org; sudeep.holla@arm.com;
thanu.rangarajan@arm.com; Michael.Williams@arm.com; Thierry Reding
[off-list ref]; Jonathan Hunter [off-list ref]; Vikram
Sethi [off-list ref]; mike.leach@linaro.org; leo.yan@linaro.org
Subject: RE: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for
ARM CoreSight PMU driver

quoted

-----Original Message-----
From: Robin Murphy <robin.murphy@arm.com>
Sent: Wednesday, July 13, 2022 3:13 AM
To: Mathieu Poirier <mathieu.poirier@linaro.org>; Besar Wicaksono
[off-list ref]
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>;

catalin.marinas@arm.com;

quoted

will@kernel.org; mark.rutland@arm.com; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
tegra@vger.kernel.org; sudeep.holla@arm.com;
thanu.rangarajan@arm.com; Michael.Williams@arm.com; Thierry Reding
[off-list ref]; Jonathan Hunter [off-list ref]; Vikram
Sethi [off-list ref]; mike.leach@linaro.org; leo.yan@linaro.org
Subject: Re: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for
ARM CoreSight PMU driver

External email: Use caution opening links or attachments


On 2022-07-12 17:36, Mathieu Poirier wrote:
[...]

quoted

quoted

quoted

If we have decied to call this arm_system_pmu, (which I am perfectly
happy with), could we please stick to that name for functions that we
export ?

e.g,

s/coresight_pmu_sysfs_event_show/arm_system_pmu_event_show()/

quoted

quoted

quoted

Just want to confirm, is it just the public functions or do we need to

replace

quoted

quoted

all that has "coresight" naming ? Including the static functions, structs,

filename.

quoted

I think all references to "coresight" should be changed to

"arm_system_pmu",

quoted

including filenames.  That way there is no doubt this IP block is not
related, and does not interoperate, with the any of the "coresight" IP

blocks

quoted

already supported[1] in the kernel.

I have looked at the documentation[2] in the cover letter and I agree
with an earlier comment from Sudeep that this IP has very little to do

with

quoted

any

quoted

of the other CoreSight IP blocks found in the CoreSight framework[1].

Using the

quoted

"coresight" naming convention in this driver would be _extremely_

confusing,

quoted

especially when it comes to exported functions.

But conversely, how is it not confusing to make up completely different
names for things than what they're actually called? The CoreSight
Performance Monitoring Unit is a part of the Arm CoreSight architecture,
it says it right there on page 1. What if I instinctively associate the
name Mathieu with someone more familiar to me, so to avoid confusion I'd
prefer to call you Steve? Is that OK?

What is the naming convention for modules under drivers/perf ?
In my observation, the names there correspond to the part monitored by
the PMU. The confusion on using "coresight_pmu" naming could be that
people may think the PMU monitors coresight system, i.e the trace system
under hwtracing.
However, the driver in this patch is for a new PMU standard that monitors
uncore
parts. Uncore was considered as terminology from Intel, so "system" was
picked instead.
Please see this thread for reference:
  https://lore.kernel.org/linux-arm-kernel/20220510111318.GD27557@willie-
the-truck/

quoted

As it happens, Steve, I do actually agree with you that "coresight_" is
a bad prefix here, but only for the reason that it's too general. TBH I
think that's true of the existing Linux subsystem too, but that damage
is already done, and I'd concur that there's little value in trying to
unpick that now, despite the clear existence of products like CoreSight
DAP and CoreSight ELA which don't have all that much to do with program
trace either.

However, hindsight and inertia are hardly good reasons to double down on
poor decisions, so if I was going to vote for anything here it would be
"cspmu_", which is about as
obviously-related-to-the-thing-it-actually-is as we can get while also
being pleasantly concise.

[ And no, this isn't bikeshedding. Naming things right is *important* ]

I agree having the correct name is important, especially at this early stage.
A direction of what the naming should describe would be very helpful here.

quoted

Cheers,
Robin.

quoted

Thanks,
Steve

[1]. drivers/hwtracing/coresight/
[2]. https://developer.arm.com/documentation/ihi0091/latest

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel