On Thu, 21 Jul 2022 at 03:19, Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote: > > Hi > > On 14/07/2022 05:47, Besar Wicaksono wrote: > > > > > >> -----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/ > > I think we all understand the state of affairs. > > - We have an architecutre specification for PMUs, Arm CoreSight PMU > Architecutre, which has absolutely no relationship with : > > either CoreSight Self-Hosted Tracing (handled by "coresight" > subsystem in the kernel under drivers/hwtracing/coresight/, with a user > visible pmu as "cs_etm") > > or the CoreSight Architecture (except for the name). This is of less > significance in general. But has a significant impact on the "name" > users might expect for the driver/Kconfig etc. > > - We want to be able to make it easier for the users/developers to > choose what they want without causing confusion. > > For an end-user: Having the PMU instance named after the "System IP" > (as implememented in the driver solves the problem and falling back to > arm_system_pmu is a good enough choice. So let us stick with that) > > Kconfig: May be we can choose > CONFIG_ARM_CORESIGHT_PMU_ARCH_PMU > or even > CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU > > with appropriate help text to ensure there is enough stress about what > this is and what this is not would be sufficient. > > Now the remaining contention is about the name of the "subsystem" and > also the dir/files. This may sound insignificant. But it is also > important to get this right. e.g., helps the reviewers unambiguously > identify the change or maintainers accepting pull requests (remember > these two PMUs (cs_etm and this one) go via different trees.). Not > everyone who deals with this in the community may be aware of how > these are different. > > We could choose arm_cspmu_ or simply cspmu. Given that only the > "normal" users care about the "association" with the "architecture" > and more advanced users (e.g, developers) can easily map "Kconfig" > to driver files, may be we could even stick to the "arm_syspmu" > (from "arm system pmu") ? > +1 on "arm_syspmu" > Suzuki > > > > > >> 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 >
