RE: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for ARM CoreSight PMU driver

[Besar Wicaksono]

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@xxxxxxx>; Mathieu Poirier
> <mathieu.poirier@xxxxxxxxxx>
> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>; catalin.marinas@xxxxxxx;
> will@xxxxxxxxxx; mark.rutland@xxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> tegra@xxxxxxxxxxxxxxx; sudeep.holla@xxxxxxx;
> thanu.rangarajan@xxxxxxx; Michael.Williams@xxxxxxx; Thierry Reding
> <treding@xxxxxxxxxx>; Jonathan Hunter <jonathanh@xxxxxxxxxx>; Vikram
> Sethi <vsethi@xxxxxxxxxx>; mike.leach@xxxxxxxxxx; leo.yan@xxxxxxxxxx
> Subject: RE: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for
> ARM CoreSight PMU driver
> 
> 
> 
> > -----Original Message-----
> > From: Robin Murphy <robin.murphy@xxxxxxx>
> > Sent: Wednesday, July 13, 2022 3:13 AM
> > To: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>; Besar Wicaksono
> > <bwicaksono@xxxxxxxxxx>
> > Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>;
> catalin.marinas@xxxxxxx;
> > will@xxxxxxxxxx; mark.rutland@xxxxxxx; linux-arm-
> > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > tegra@xxxxxxxxxxxxxxx; sudeep.holla@xxxxxxx;
> > thanu.rangarajan@xxxxxxx; Michael.Williams@xxxxxxx; Thierry Reding
> > <treding@xxxxxxxxxx>; Jonathan Hunter <jonathanh@xxxxxxxxxx>; Vikram
> > Sethi <vsethi@xxxxxxxxxx>; mike.leach@xxxxxxxxxx; leo.yan@xxxxxxxxxx
> > 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:
> > [...]
> > >>> 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()/
> > >>>
> > >>
> > >> Just want to confirm, is it just the public functions or do we need to
> > replace
> > >> all that has "coresight" naming ? Including the static functions, structs,
> > filename.
> > >
> > > I think all references to "coresight" should be changed to
> > "arm_system_pmu",
> > > 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
> > > 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
> > any
> > > of the other CoreSight IP blocks found in the CoreSight framework[1].
> > Using the
> > > "coresight" naming convention in this driver would be _extremely_
> > confusing,
> > > 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/
> 
> > 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.
> 
> > Cheers,
> > Robin.
> >
> > >
> > > Thanks,
> > > Steve
> > >
> > > [1]. drivers/hwtracing/coresight/
> > > [2]. https://developer.arm.com/documentation/ihi0091/latest