Date:   Thu, 21 Jul 2022 09:35:44 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Suzuki K Poulose <suzuki.poulose@....com>
Cc:     Besar Wicaksono <bwicaksono@...dia.com>,
        Robin Murphy <robin.murphy@....com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "sudeep.holla@....com" <sudeep.holla@....com>,
        "thanu.rangarajan@....com" <thanu.rangarajan@....com>,
        "Michael.Williams@....com" <Michael.Williams@....com>,
        Thierry Reding <treding@...dia.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Vikram Sethi <vsethi@...dia.com>,
        "mike.leach@...aro.org" <mike.leach@...aro.org>,
        "leo.yan@...aro.org" <leo.yan@...aro.org>
Subject: Re: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for ARM
 CoreSight PMU driver

On Thu, 21 Jul 2022 at 03:19, Suzuki K Poulose <suzuki.poulose@....com> wrote:
>
> Hi
>
> On 14/07/2022 05:47, Besar Wicaksono wrote:
> >
> >
> >> -----Original Message-----
> >> From: Robin Murphy <robin.murphy@....com>
> >> Sent: Wednesday, July 13, 2022 3:13 AM
> >> To: Mathieu Poirier <mathieu.poirier@...aro.org>; Besar Wicaksono
> >> <bwicaksono@...dia.com>
> >> Cc: Suzuki K Poulose <suzuki.poulose@....com>; catalin.marinas@....com;
> >> will@...nel.org; mark.rutland@....com; linux-arm-
> >> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
> >> tegra@...r.kernel.org; sudeep.holla@....com;
> >> thanu.rangarajan@....com; Michael.Williams@....com; Thierry Reding
> >> <treding@...dia.com>; Jonathan Hunter <jonathanh@...dia.com>; Vikram
> >> Sethi <vsethi@...dia.com>; mike.leach@...aro.org; leo.yan@...aro.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/
>
> 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
>

Powered by blists - more mailing lists