Messages in this thread |  | | From | Besar Wicaksono <> | | Subject | RE: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for ARM CoreSight PMU driver | | Date | Thu, 21 Jul 2022 03:36:25 +0000 |
| |
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 > <mathieu.poirier@linaro.org> > 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 > <treding@nvidia.com>; Jonathan Hunter <jonathanh@nvidia.com>; Vikram > Sethi <vsethi@nvidia.com>; 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 > > > > > -----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 > > <bwicaksono@nvidia.com> > > 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 > > <treding@nvidia.com>; Jonathan Hunter <jonathanh@nvidia.com>; Vikram > > Sethi <vsethi@nvidia.com>; 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: > > [...] > > >>> 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
|  |